Add BlobSidecarGossipValidator for new BlobSidecar with merkle proof validation #7687
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...c/test/java/tech/pegasys/teku/statetransition/validation/BlobSidecarGossipValidatorTest.java
Fixed
Show resolved
Hide resolved
...n/src/main/java/tech/pegasys/teku/statetransition/validation/BlobSidecarGossipValidator.java
Dismissed
Show dismissed
Hide dismissed
...rc/main/java/tech/pegasys/teku/statetransition/validation/BlobSidecarGossipValidatorOld.java
Dismissed
Show dismissed
Hide dismissed
| new ArrayList<>(Collections.singleton(GIndexUtil.SELF_G_INDEX)))); | ||
| int depth = 0; | ||
| final List<Long> path = new ArrayList<>(); | ||
| final int depth = Long.SIZE - Long.numberOfLeadingZeros(nodeIndex); |
There was a problem hiding this comment.
Great code reduction 👍
I'd suggest using GIndexUtil.gIdxGetDepth() here
There was a problem hiding this comment.
LGTM. Just some nits.
I'd like to get an approval from @Nashatyrev too wrt merkle proof part
| } | ||
|
|
||
| public String toLogString() { | ||
| return LogFormatter.formatBlobSidecar( | ||
| getSlot(), | ||
| getBlockRoot(), | ||
| getBlockBodyRoot(), |
There was a problem hiding this comment.
Seems odd to print the block body root instead of block root here
There was a problem hiding this comment.
Nice catch, which puts a focus on another issue: we cannot save it to DB since this change unless we get a block with corresponding body_root.
I have added both body_root and state_root to log string, since on most explorers we don't have body_root, but we may still want to see it to identify corresponding block.
There was a problem hiding this comment.
I'm stupid, we could do block root from block header. All fixed
| public SafeFuture<InternalValidationResult> validate( | ||
| final SignedBlobSidecarOld signedBlobSidecar) { | ||
| final BlobSidecarOld blobSidecar = signedBlobSidecar.getBlobSidecar(); | ||
| public SafeFuture<InternalValidationResult> validate(final BlobSidecar blobSidecar) { |
There was a problem hiding this comment.
I'd like to unify the validation of the block's field with the BlockGossipValidator. But let's do that on a separate PR (I might pick that up if I get the rid of my current task)
There was a problem hiding this comment.
I've modified previous flow to follow the spec in the order and steps (when it's possible). Also I've added to our plan a task to optimize this (to the bottom)
| depth++; | ||
| for (int i = 0; i <= depth; ++i) { | ||
| path.add(currentIndex); | ||
| currentIndex = currentIndex >> 1; |
There was a problem hiding this comment.
nit: I'd use gIdxGetParent here for better clarity
…lobsidecar-validator
PR Description
DeprecatedMerkleUtil.getPathToNodeis rewritten. Old was buggy.testPathNotFoundtest is not relevant, removed, because we have negative check in the beginning (and test for it), so was passed because of bug. (Correct me if I'm wrong)MiscHelpersDeneb.blobKzgCommitmentsGeneralizedIndex()in future fork MiscHelpers, becauseMAX_BLOB_COMMITMENTS_PER_BLOCKwas set to some big value to guarantee that it will not be changed if we change max blobs (Correct me if I'm wrong)Part of #7654
Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog