-
Notifications
You must be signed in to change notification settings - Fork 304
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
[EIP-4844] Implement BlobsManager #6636
Conversation
@@ -101,8 +99,7 @@ | |||
} | |||
|
|||
return validateWeakSubjectivityPeriod() | |||
.thenCompose( | |||
__ -> forkChoice.onBlock(block, blobsSidecar, blockImportPerformance, executionLayer)) | |||
.thenCompose(__ -> forkChoice.onBlock(block, blockImportPerformance, executionLayer)) |
Check notice
Code scanning / CodeQL
Useless parameter
8d86c8a
to
cf52a22
Compare
2855fd0
to
6e3f902
Compare
6e719ba
to
4a697bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks definitely like an improvement. Just a few things:
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
Outdated
Show resolved
Hide resolved
...ransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java
Outdated
Show resolved
Hide resolved
...ransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java
Outdated
Show resolved
Hide resolved
...ransition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlobsSidecarManagerImpl.java
Outdated
Show resolved
Hide resolved
...reum/statetransition/src/main/java/tech/pegasys/teku/statetransition/block/BlockManager.java
Show resolved
Hide resolved
...atetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java
Outdated
Show resolved
Hide resolved
...atetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java
Show resolved
Hide resolved
...atetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java
Outdated
Show resolved
Hide resolved
...atetransition/src/test/java/tech/pegasys/teku/statetransition/forkchoice/ForkChoiceTest.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void storeUnconfirmedBlobs(final BlobsSidecar blobsSidecar) { | ||
// remove blobs from the already validated cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? We couldn't unvalidate blobs, but we could get, say, IGNORE result on the second import try and remove validation in this case. I don't understand why it's needed. From my point of view, it could only damage us in some case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it isn't required.. it is a strange case, that should never occur, so the idea was to be conservative and force a revalidation of the sidecar against commitments during subsequent block import. But since it is strictly coupled with the block via blockroot I can remove it.
spotlessly continue reverting blobs cache data availability check if commitments are empty implement ForkChoice UT implement BlockManager UT revert blobs cache in ChainStorage fix block validator UT renaming remove getBlobsSidecar in manager add blobs sidecar caching rename to blobsSidecarManager add tests postponing blobs storing after known\invalid block checks expose cause add gossip validation and fix Availability Checker first tmp commit
d18b324
to
5bc09d9
Compare
Introduces
BlobsSidecarManager
which is responsible for:BlockManager
BlockManager
blobsSidecarAvailabilityChecker
used inForkChoice
ForkCoiche
for blocks\sidecar coming from gossip.Under kvstore and storage:
removeUnconfirmedBlobsSidecar
->confirmBlobsSidecar
ChainStorage
and related classesChainStorage
fro void returning blobs methodsIn
StorageUpdate
andStoreTransactionUpdates
:Set<blockRoot>
toMap<blockRoot,slot>
to support blobs cleanup when hot blocks are prunedIn
BlockValidator
(gossip):In
BlockManager
:handleBlobs
method in thedoImportBlock
pipeline to store blobsSidecar viaBlobsSidecarManager
. This is done after block validity check (handleInvalidBlock
) and after checking it is actually a new block not seen before (handleKnownBlock
). Gossip validation result is considered to allowBlobsSidecarManager
to know if the availability check needs to be done or not.Additional notes
we could discard blobs sending them to the BlobsPruner which will gradually delete them from BD without queue the deletions in the storageUpdateChannel which can cause slowdown if many blobs are discarded in sequence (various pending blocks on a fork that turns out to be invalid). We could implement a BlobsDeletionChannel and block BlockPruner will consume it
having unconfirmed affects how we handle BlobsSidecarByRange rpc method:
containsBlock
)for the unconfirmed column I found an interesting approach that could make the confirmed-unconfirmed flag more efficient to store and update: https://groups.google.com/g/leveldb/c/qOMWvp7gyxg but would require some changes in our kvstore layer
id
is our current prefix andattributeId
s is a new postfix byte from 0 to number of attributes defined + the actual value we store. The column will have multiple value serializers one for each attributeId. In our case we can have:<blobsSidecarId, <SlotAndBlockRoot>, 0>
->KvStoreSerializer<Boolean>
(confirmed flag)<blobsSidecarId, <SlotAndBlockRoot>, 1>
->KvStoreSerializer<Bytes>
(BlobsSidecar ssz data)so when streaming\lookingup blobs we can efficiently filter by unconfirmed flag without even try to deserialize the object if doesn't match the filter.
This need further investigation for rocksDB.
fixes #6629
fixes #6473
Documentation
doc-change-required
label to this PR if updates are required.Changelog