Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize BlobSidecarGossipValidator #7749

Merged
merged 5 commits into from
Nov 23, 2023

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Nov 22, 2023

PR Description

  • Cache size calculation looks a bit ugly but I don't want to create new constant for this, open for any improvements
  • At first I reused the checks moving most to dedicated method but copy-paste looks better from my side

Fixed Issue(s)

Fixes #7654

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@mehdi-aouadi mehdi-aouadi left a comment

Choose a reason for hiding this comment

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

I'd prefer extracting the check to avoid maintaining two code blocks 😄
I'd also add a test for the known block headers cache (make sure we check everything again if more than x epochs passed since we've verified a blob with a specific block header)...

@@ -63,14 +65,17 @@ public static BlobSidecarGossipValidator create(
final Optional<Integer> maybeMaxBlobsPerBlock = spec.getMaxBlobsPerBlock();

final int validInfoSize = VALID_BLOCK_SET_SIZE * maybeMaxBlobsPerBlock.orElse(1);
// It's not fatal if we miss something and we don't need finalized data
final int validSignedBlockHeadersSize = spec.getGenesisSpec().getSlotsPerEpoch() * 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather extract the 3 to a config constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and it could have a proper name, it's not a magic number, will do it

@zilm13
Copy link
Contributor Author

zilm13 commented Nov 22, 2023

@mehdi-aouadi I want to keep order the same as in the spec in long case and the checks we need in short case are not all one by one from it, I've tried to reuse it first time, but it actually looks not so good.
I will add a test, good idea

Copy link
Contributor

@mehdi-aouadi mehdi-aouadi left a comment

Choose a reason for hiding this comment

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

LGTM

blobSidecar.getIndex()))) {
return ignore(
"BlobSidecar is not the first valid for its slot and index. It will be dropped.");
synchronized (this) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why synchronised when using LimitedSet.createSynchronized ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"It is imperative that the user manually synchronize on the returned map when traversing any of its collection views via Iterator, Spliterator or Stream:"
Looks like we are safe on atomic operations, removed

@StefanBratanov
Copy link
Contributor

LGTM to me as well

@zilm13 zilm13 merged commit 82c578d into Consensys:master Nov 23, 2023
15 checks passed
@zilm13 zilm13 deleted the gossip-blobsidecar-optimize branch November 23, 2023 15:13
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.

Move to sidecar inclusion proof
3 participants