Skip to content

Conversation

anupsv
Copy link
Contributor

@anupsv anupsv commented Jul 22, 2025

Why are these changes needed?

We are removing the wiring of the blacklist feature. I.E we are removing the place where it is added to the list and the place where it was being checked. We are also removing the integration test which was testing these features.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@anupsv anupsv requested review from samlaf and litt3 July 22, 2025 19:30
Copy link

github-actions bot commented Jul 22, 2025

The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJul 22, 2025, 9:37 PM

@@ -134,13 +134,6 @@ func (s *ServerV2) StoreChunks(ctx context.Context, in *pb.StoreChunksRequest) (
return nil, api.NewErrorInvalidArg(fmt.Sprintf("failed to serialize batch header hash: %v", err))
}

// If the disperser is blacklisted and the blob authenticator is not nil, return an error
// we don't want to blacklist the disperser if the blob authenticator is nil since that indicated v1
if s.node.BlacklistStore.IsBlacklisted(ctx, in.GetDisperserID()) && s.config.EnableV2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's not longer used, consider also deleting the instantiation of the BlacklistStore inside node.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make the PR bigger, but sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@samlaf
Copy link
Collaborator

samlaf commented Jul 22, 2025

I think it LGTM. Super tired though and need some sleep so I'll let @litt3 and @dmanc review this one.

@samlaf samlaf requested a review from dmanc July 22, 2025 19:49
Copy link
Contributor

@dmanc dmanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deployed the latest commit and everything looks good.

@anupsv anupsv enabled auto-merge July 22, 2025 21:37
@anupsv anupsv added this pull request to the merge queue Jul 22, 2025
Merged via the queue into master with commit 378663d Jul 22, 2025
29 checks passed
@anupsv anupsv deleted the remove-blacklist-wiring branch July 22, 2025 21:58
dmanc pushed a commit that referenced this pull request Jul 22, 2025
* removing blacklist wiring and check

* removing blacklist test

* fixing unit tests

* removing node initialization

---------

Co-authored-by: anupsv <[email protected]>
dmanc added a commit that referenced this pull request Jul 22, 2025
fix: remove blacklist wiring from nodes (#1788)

* removing blacklist wiring and check

* removing blacklist test

* fixing unit tests

* removing node initialization

---------

Co-authored-by: anupsv <[email protected]>
Co-authored-by: anupsv <[email protected]>
pakim249CAL pushed a commit that referenced this pull request Jul 22, 2025
* removing blacklist wiring and check

* removing blacklist test

* fixing unit tests

* removing node initialization

---------

Co-authored-by: anupsv <[email protected]>
pakim249CAL pushed a commit that referenced this pull request Jul 23, 2025
* removing blacklist wiring and check

* removing blacklist test

* fixing unit tests

* removing node initialization

---------

Co-authored-by: anupsv <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants