feat: optimistically verify blocks even before all blobs available #6087
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
|
what's the current strategy now? |
current strategy is wait till all block and blobs show up and then start the import. Since now there is an additive delay observed on the blobs, its best to verify block as much while the blobs are still becoming available this PR seems to be working fine on devnet 11 lodestar-geth-1, just need to cleanup and finalize the PR |
g11tech
commented
Nov 9, 2023
packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts
Outdated
Show resolved
Hide resolved
g11tech
force-pushed
the
optimistic-verify
branch
from
3f2d2eb
to
6b7288a
Compare
g11tech
commented
Nov 11, 2023
|
|
||
| // proposer boost is not available post 3 sec so try pulling using unknown block hash | ||
| // post 3 sec after throwing the availability error | ||
| const BLOB_AVAILABILITY_TIMEOUT = 3_000; |
There was a problem hiding this comment.
should this (time to wait for all blobs to show up before firing unknown block event) be kept 3, or 4 or full 12 sec?
There was a problem hiding this comment.
Proposer boost is available up to 1/3 into the slot, so 4 sec with Ethereum spec. Why not fire the unknown block event immediately upon receiving the blob? AFAIK lighthouse publish strategy is to publish block first, then blobs
There was a problem hiding this comment.
right now most of the time unknownBlock always issue beacon_block_by_root due to unknown block root in beacon_attestation message, so should have no issue to fire unknown block event from blobs. It should issue single request per block root anyway.
there is an issue that unknownBlock sync issues beacon_block_root even we're processing gossip block, should avoid these redundant requests in #6105
There was a problem hiding this comment.
@g11tech do you mean to pull blobs or block?
There was a problem hiding this comment.
Why not fire the unknown block event immediately upon receiving the blob?
here block is available not all blobs, currently block processing is only started we see the block, blobs we keep caching till we see the block and thats something we can handle on the gossip handler of the blob (you have pointed this out there as well)
Also even if block is send before blobs, there is no guarantee one may see block before blobs, so it would be a good idea to wait for some latency in that case as well, but more of this disucssion/resolution there.
Here we have the block but not all blobs and we are verifying the other aspects of the block (state transition, execution payload, signatures) while we wait for blobs to become available. so question is what is the right threshold to wait till we trigger unknownblock/blobs even to pull them by root.
Looking from the devnet12 data, there could be 500ms to 1sec latency so i guess we shouldn't wait for more than a sec defintely.
There was a problem hiding this comment.
added this issue to tracker (#5279) to tackle on top of blob inclusion proof PR
g11tech
force-pushed
the
optimistic-verify
branch
2 times, most recently
from
f63a770
to
1c1f667
Compare
dapplion
reviewed
Nov 20, 2023
|
|
||
| // proposer boost is not available post 3 sec so try pulling using unknown block hash | ||
| // post 3 sec after throwing the availability error | ||
| const BLOB_AVAILABILITY_TIMEOUT = 3_000; |
There was a problem hiding this comment.
Proposer boost is available up to 1/3 into the slot, so 4 sec with Ethereum spec. Why not fire the unknown block event immediately upon receiving the blob? AFAIK lighthouse publish strategy is to publish block first, then blobs
| // and should have resolved the availability promise, however we could track if the block processing | ||
| // was halted and requeue it | ||
| // | ||
| // handleValidBeaconBlock(blockInput, peerIdStr, seenTimestampSec); |
There was a problem hiding this comment.
So the data received through the blob_sidecar topic is discarded? Both if branches do nothing
There was a problem hiding this comment.
Looking into the blob validation function noticed that there's an unnecessary call to regen here
lodestar/packages/beacon-node/src/chain/validation/blobSidecar.ts
Lines 103 to 107 in fa30bcf
you only need the global pubkey cache and a config, no need to calculate a specific state.
There was a problem hiding this comment.
So the data received through the blob_sidecar topic is discarded? Both if branches do nothing
yes currently only seeing block only we trigger processing doesn't matter if all blobs are available or not (we concurrently wait for availability while other aspects of block are getting verified) and if the data doesn't become available in a threshold time we throw error which triggers pulling the block/blobs by root.
We can also threshold here the delay from the starting of the slot to throw the unknown block event. if the block was there (then we get non null blockinput) it must already be in the block processing queue waiting for blobs to become available which this blob would have resolved the promise if this was the last blob needed.
But if that was not the case then we can throw unknown block to pull by root.
There was a problem hiding this comment.
you only need the global pubkey cache and a config, no need to calculate a specific state.
ahh right, had this copied over from block, will see how to optimize.
There was a problem hiding this comment.
updated in the tracker (#5279) to tackle on top of blob inclusion proof PR
twoeths
reviewed
Nov 21, 2023
twoeths
reviewed
Nov 21, 2023
twoeths
reviewed
Nov 21, 2023
twoeths
reviewed
Nov 21, 2023
twoeths
reviewed
Nov 21, 2023
g11tech
force-pushed
the
optimistic-verify
branch
from
1c1f667
to
e8d10c1
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6087 +/- ##
=========================================
Coverage 90.35% 90.35%
=========================================
Files 78 78
Lines 8087 8087
Branches 490 490
=========================================
Hits 7307 7307
Misses 772 772
Partials 8 8 |
g11tech
force-pushed
the
optimistic-verify
branch
from
add164a
to
49c3713
Compare
twoeths
reviewed
Dec 18, 2023
twoeths
reviewed
Dec 18, 2023
packages/beacon-node/src/chain/seenCache/seenGossipBlockInput.ts
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Dec 18, 2023
twoeths
reviewed
Dec 18, 2023
twoeths
reviewed
Dec 18, 2023
twoeths
reviewed
Dec 18, 2023
twoeths
reviewed
Dec 18, 2023
| // Run state transition only | ||
| // TODO: Ensure it yields to allow flushing to workers and engine API | ||
| verifyBlocksStateTransitionOnly( | ||
| preState0, | ||
| blocksInput, | ||
| dataAvailabilityStatuses, | ||
| // hack availability for state transition eval as availability is separately determined | ||
| blocks.map(() => DataAvailableStatus.available), |
There was a problem hiding this comment.
should base on block.slot to determine DataAvailableStatus preDeneb vs available?
There was a problem hiding this comment.
internally in state transition it doesn't matter, but for correctness sake may be we can do it, although its inconsequential here as availability is actually verified as part of full block verification
nflaig
reviewed
Dec 19, 2023
cleanup pr and add metrics to track simplify improvements and type fixes increase bucket precision time fixes improve metrics collection improve metrics collection some comments improv fix the missing writing blobs for blobspromise rebase fixes rebase fixes remove artifact apply feedback add more meta info to error separate out the blockinput cache and attach to chain rename the cache apply more feedback add unittest for seengossipblockinput add comments
g11tech
force-pushed
the
optimistic-verify
branch
from
0e793db
to
f43984e
Compare
twoeths
approved these changes
Dec 19, 2023
|
🎉 This PR is included in v1.14.0 🎉 |
ensi321
pushed a commit
to ensi321/lodestar
that referenced
this pull request
Jan 22, 2024
…hainSafe#6087) * feat: optimistically verify blocks even before all blobs available cleanup pr and add metrics to track simplify improvements and type fixes increase bucket precision time fixes improve metrics collection improve metrics collection some comments improv fix the missing writing blobs for blobspromise rebase fixes rebase fixes remove artifact apply feedback add more meta info to error separate out the blockinput cache and attach to chain rename the cache apply more feedback add unittest for seengossipblockinput add comments * check type fixes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(WIP)
while all blobs become available, block can be optimistically verified to save any delays on the import times
this PR implements blockInput with blobsPromise that need to resolve before the block can be imported in a timebound manner else the unknownblock/blob search is initiated
TODO: