VersionedHashDBSource tests #9908
Conversation
| CustodyGroupCountChannel.class, | ||
| new CustodyGroupCountChannel() { | ||
| @Override | ||
| public void onGroupCountUpdate(int custodyGroupCount, int samplingGroupCount) { |
There was a problem hiding this comment.
a question here: do we receive this event here quick enough even when the node is a supernode due to number of ETH attached and not just due to explicit custody param?
I'm worried if we are able to set the flag quick enough and avoid loosing versioned hashed
There was a problem hiding this comment.
Yeah, I was thinking about this. We could have race here. We could try to pass supernode flag earlier at least.
There was a problem hiding this comment.
Don't we save how much we custody in a db var, so that it can go up only as per spec? I just remember this, maybe i'm wrong
There was a problem hiding this comment.
Yeah, we save, but we cannot put db read in constructor (or can?). I mean it's slow
| .getKZGCommitment()); | ||
| } else { | ||
| this.dataColumnSidecarToVersionedHash = pair -> VersionedHash.EMPTY; | ||
| } |
There was a problem hiding this comment.
I wander if we need to provide a noop version. Is there a situation where we call these ToVersionedHash functions and it is legit to have a NOOP one? would it be a bug?
There was a problem hiding this comment.
It's basically is fulu milestone supported. I don't understand where could it fail
There was a problem hiding this comment.
I mean we can provide the actual function without checking if supported. Cose if we end up calling those functions we are obviously in that milestone. If not is a bug (but in this version we will return empty instead of failing)
There was a problem hiding this comment.
Ahh yeah, you are right, until it's called it cannot fail, I will update the code
| private final Function<BlobSidecar, VersionedHash> blobSidecarToVersionedHash; | ||
| private final Function<Pair<DataColumnSidecar, UInt64>, VersionedHash> | ||
| dataColumnSidecarToVersionedHash; | ||
| private final AtomicBoolean storeSidecarHashes = new AtomicBoolean(false); |
There was a problem hiding this comment.
shouldn't we control the activation of the blobSidecar's versioned hashes storage too? not sure, we want to activate this for blobs by default...
There was a problem hiding this comment.
Yeah, blobSidecar is available by default.
But for dataColumnSidecars we need to have all sidecars (to be precise at least 64) to restore blob, so it makes sense there.
There was a problem hiding this comment.
But if you are not running an L2 you won't need that api and you wont need to store those indices. I thought that api call by versionedhashes was essentially optional
There was a problem hiding this comment.
It replaces getBlobSidecars, it's not in debug space ethereum/beacon-APIs#546
So by spec we should support it by default from my understanding
| Bytes.wrap(Longs.toByteArray(sidecar.getSlot().longValue())), | ||
| sidecar.getBlockRoot(), | ||
| Bytes.wrap(Longs.toByteArray(blobIndex.longValue()))); | ||
| } |
There was a problem hiding this comment.
thinking about is it is wise to use DataColumnSlotAndIdentifier here too so we make sure we save data that is by design matches the key we use to lookup blobs
There was a problem hiding this comment.
Semantically it will be more correct to use SlotAndBlockRootAndBlobIndex for both but it's not SSZ and doesn't have handy serialization/deserealization
|
Finished |
zilm13
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
storage/src/test/java/tech/pegasys/teku/storage/server/ChainStorageTest.java
Outdated
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Outdated
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Outdated
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Outdated
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Outdated
Show resolved
Hide resolved
...test/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/VersionedHashDBSourceTest.java
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| dataColumnSidecar.getIndex().increment()), | ||
| updater); | ||
| verifyNoMoreInteractions(dao, updater, commitmentToVersionedHashFunction); | ||
| } |
There was a problem hiding this comment.
Bug: Sidecar Deduplication Test Fails Due to Random KZG Commitments
The addSidecarVersionedHashes and addNonCanonicalSidecarVersionedHashes tests create a second DataColumnSidecar with new random KZG commitments. This prevents correctly testing the deduplication logic for sidecars from the same block, as all data column sidecars for a given block should share the same KZG commitments.
Additional Locations (1)
3 participants
PR Description
Part of #9835
Start from
VersionedHashDBSourceKvStoreDatabasefor every actions withVersionedHash'esVersionedHashDBSourceFactorystarting fromVersionedDatabaseFactorygetSidecarIdentifierwill be used later to get correspondingBlobVersionedHash-es toStore, because it would be difficult to keep it update along with blobSidecars. Also DataColumnSidecars are not a part of theStoreand niche of VersionedHash usage is onlygetBlobsRPC.Fixed Issue(s)
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Add VersionedHash storage/lookup tests and integrate versioned-hash assertions for blob/data-column sidecars, plus minor KZG fixture tweak and small spec/util helpers.
VersionedHashDBSourceTestandVersionedHashDBSourceFactoryTestcovering activation, add/remove, and serialization of sidecar identifier metadata; wire viaCustodyGroupCountChannel.DatabaseTestto assertVersionedHashpresence/absence for canonical and non-canonicalBlobSidecarandDataColumnSidecar, including pruning flows; add helpers to activate and verify versioned-hash mappings and combined availability checks.ChainStorageTestto verifySidecarIdentifierretrieval byVersionedHashfor finalized historical blob sidecars.DataStructureUtil: addrandomVersionedHash(), refactorrandomVersionedHashes, and overloadrandomDataColumnSidecar(...)to accept existing commitments.BlobSidecar: addgetSlotAndBlockRootAndBlobIndex()helper.NoOpKZG.blobToKzgCommitmentnow derives commitment from the blob’s first 48 bytes instead of returning zeros.Written by Cursor Bugbot for commit 0e97154. This will update automatically on new commits. Configure here.