Simplify blob sidecar availability checker #8840
Conversation
...ence-tests/src/referenceTest/java/tech/pegasys/teku/reference/ManualReferenceTestRunner.java
Outdated
Show resolved
Hide resolved
| @@ -39,6 +39,9 @@ public class BlobSidecar | |||
| SignedBeaconBlockHeader, | |||
| SszBytes32Vector> { | |||
|
|
|||
| private volatile boolean kzgAndInclusionProofValidated = false; | |||
| private volatile boolean signatureValidated = false; | |||
There was a problem hiding this comment.
I find these validation steps a bit confusing and I'm wondering if we can't simply mark the blob sidecar as valid or not (merge them) or split the kzgAndInclusionProofValidated into two validation flags: kzgValidated and inclusionProofValidated. An and in a boolean variable name is red flag to me. Or do we really need this design for some reason I'm missing?!
There was a problem hiding this comment.
the reason I put kzg and inclusion proof together and signatureValidation as a separate boolean is because we can perform kzg and inclusion proof validation just having the blob itself (you don't need anything else). For the signature validation you need a prevalidated signed block OR the state to get the pubkey from and do the signature check.
| && !blobSidecar | ||
| .getSignedBeaconBlockHeader() | ||
| .hashTreeRoot() | ||
| .equals(block.hashTreeRoot())) { |
There was a problem hiding this comment.
Isn't this the same check done twice?!
There was a problem hiding this comment.
same check as which one?
There was a problem hiding this comment.
I meant same as blobSidecar.isSignatureValidated()
There was a problem hiding this comment.
ok but where am I doing this the second time?
I'm not sure this always checked. For instance the RPC lookups check blobs via BlobSidecarsByRootValidator.validate, which can only do kzg and inclusion proof.
So if we end up processing blobs coming from RPC source, we are not checking the block header in the blobSidecar (correct me if i'm wrong :) )
There was a problem hiding this comment.
oh. I actually forgot to do the marking in BlobSidecarsByRangeListenerValidatingProxy
There was a problem hiding this comment.
mmm maybe I can move those marking deeper in the stack and have them separated as you suggested
There was a problem hiding this comment.
So if we end up processing blobs coming from RPC source, we are not checking the block header in the blobSidecar (correct me if i'm wrong :) )
Ok I see what you mean. One should keep in mind the whole flow to understand that...
|
since i forgot to do this marking i think the way I did it is a bit error prone. I'll change it. |
|
I will join Mehdi feedback:
|
tbenr
force-pushed
the
simplify-BlobSidecarAvailabilityChecker
branch
2 times, most recently
from
ab8c9e8
to
36ef653
Compare
tbenr
force-pushed
the
simplify-BlobSidecarAvailabilityChecker
branch
from
0cfe5bb
to
39ecb99
Compare
blobs get from EL are marked as valid (trust assumption on EL)
TODO:
fixes #8740
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog