-
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
Introduce BlobsSidecar
storage
#6587
Conversation
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.
Some comments and question on usage
ethereum/spec/src/main/java/tech/pegasys/teku/spec/datastructures/blocks/SlotAndBlockRoot.java
Outdated
Show resolved
Hide resolved
storage/api/src/main/java/tech/pegasys/teku/storage/api/StorageUpdateChannel.java
Outdated
Show resolved
Hide resolved
storage/src/integration-test/java/tech/pegasys/teku/storage/server/kvstore/DatabaseTest.java
Outdated
Show resolved
Hide resolved
storage/src/integration-test/java/tech/pegasys/teku/storage/server/kvstore/DatabaseTest.java
Outdated
Show resolved
Hide resolved
...ge/src/main/java/tech/pegasys/teku/storage/server/kvstore/dataaccess/CombinedKvStoreDao.java
Outdated
Show resolved
Hide resolved
import tech.pegasys.teku.infrastructure.unsigned.UInt64; | ||
import tech.pegasys.teku.spec.datastructures.blocks.SlotAndBlockRoot; | ||
|
||
class SlotAndBlockRootKeySerializer implements KvStoreSerializer<SlotAndBlockRoot> { |
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.
A bit unclear what does this mean. Maybe BigEndianSlotAndBlockRootSerializer
?
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.
I understand that core idea is to have a strream of it by slots, but it's not clear by this
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.
I added a comment on the class. I don't likeBigEndianSlotAndBlockRootSerializer
:)
|
||
void removeBlobsSidecar(SlotAndBlockRoot slotAndBlockRoot); | ||
|
||
void pruneOldestBlobsSidecar(UInt64 endSlot, int pruneLimit); |
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.
I'm confused with this interface. What is a direction? What is start slot? Why do we need pruneLimit?
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.
it always start from the oldest, prunes up to endSlot
but limits the pruning to maximum pruneLimit
entries.
Naming endSlot
differently might improve readability?
|
||
void confirmBlobsSidecar(SlotAndBlockRoot slotAndBlockRoot); | ||
|
||
Optional<BlobsSidecar> getBlobsSidecar(SlotAndBlockRoot slotAndBlockRoot); |
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.
So to answer, for example, on BeaconBlockAndBlobsSidecarByRoot
we will need
- Query DB for slot for each root
- Query unconfirmed blobsSidecar for each pair to avoid it
- Get BlobsSidecar for remaining pairs using this method
not sure that such complex flow is needed.
What are current cases we have with BlobsSidecar:
write:
- gossip receive (coupled, goes to import)
- sync receive (probably coupled too, in discussion, goes to import)
read:
- BeaconBlockAndBlobsSidecarByRoot (root list)
- BlobsSidecarsByRange (slot range)
delete:
- fork pruning (drop BlobsSidecars from noncanonical chain)
- every epoch remove 1 epoch blobsSidecars
Do I miss something or wrong with it? What potential future flows could we have with it?
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.
So to answer, for example, on BeaconBlockAndBlobsSidecarByRoot we will need
since we have to respond with a coupled block and blobs, we will start looking up blocks first. Once we have an existing blockRoot we can lookup in blobsSidecar without checking the unconfirmed column.
delete:
- fork pruning (drop BlobsSidecars from noncanonical chain)
- every epoch remove 1 epoch blobsSidecars
there is one more. The sequence will be:
- prune all blobs that has not being confirmed (related to blocks that remain in pending\future until discarded - ie never actually imported (successfully or with error))
- fork pruning (pruning blobs for noncanonical blocks up to the most recent finalized slot)
- prune blobs outside the data availability window (regardless finalization or not, just to avoid db overload when chain is not finalizing)
e1f7621
to
d0c2fea
Compare
9dd99d3
to
60b4c62
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.
LGTM
Introduces two new columns:
blobsSidecar column: store blobs for the entire lifecycle
<Slot,BlockRoot> -> BlobsSidecar
unconfirmed blobsSidecar column: tracks which blobsSidecar conteined in the previous column is yet to be confirmed (which happens when the block is fully validated and imported)
<Slot,BlockRoot> -> Void
Allows blobs to be streamed ordered by slot (ascending)
Introduces two pruning methods.
MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS
)Fixed Issue(s)
fixes #6586
Documentation
doc-change-required
label to this PR if updates are required.Changelog