Introduces BlobSidecarPool with block-blobs tracking #7026
Conversation
tbenr
force-pushed
the
BlobSidecarPoolAndTrackers
branch
from
4fd019d to
ccb1e54
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...tatetransition/src/test/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolTest.java
Fixed
Show fixed
Hide fixed
...tatetransition/src/test/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolTest.java
Fixed
Show fixed
Hide fixed
tbenr
force-pushed
the
BlobSidecarPoolAndTrackers
branch
from
9444a29 to
b220675
Compare
...transition/src/test/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImplTest.java
Fixed
Show fixed
Hide fixed
tbenr
changed the title
.../main/java/tech/pegasys/teku/spec/datastructures/blobs/versions/deneb/SignedBlobSidecar.java
Show resolved
Hide resolved
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
Outdated
Show resolved
Hide resolved
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
Outdated
Show resolved
Hide resolved
...sync/src/main/java/tech/pegasys/teku/beacon/sync/gossip/blocks/FetchRecentBlocksService.java
Show resolved
Hide resolved
beacon/sync/src/testFixtures/java/tech/pegasys/teku/beacon/sync/SyncingNodeManager.java
Outdated
Show resolved
Hide resolved
...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImpl.java
Show resolved
Hide resolved
...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImpl.java
Outdated
Show resolved
Hide resolved
...tatetransition/src/main/java/tech/pegasys/teku/statetransition/util/BlobSidecarPoolImpl.java
Outdated
Show resolved
Hide resolved
| final UInt64 finalTime = | ||
| targetMillis.min(upperLimitRelativeToAttDue).max(nowMillis.plus(MIN_WAIT_MILLIS)); | ||
|
|
||
| return Optional.of(Duration.ofMillis(finalTime.minus(nowMillis).intValue())); |
There was a problem hiding this comment.
I'm not sure on all this logic if we take Gnosis. Do you know their plans for Blobs?
There was a problem hiding this comment.
good point... I think at the end it may hold even in gnosis timings.. i'll think about it
There was a problem hiding this comment.
I think for fast slot networks, we would end up being always on the MIN_WAIT_MILLIS which makes sense to me.
And I think it is acceptable that value being constant instead of being dependant by slot time, since that number is more related to network latency than actual slot time. We could argue that on fast nets, blocks (and blobs?) could be smaller so the expected latency could be lower on average, but for now seems like we can leave with a constant.
...ansition/src/main/java/tech/pegasys/teku/statetransition/blobs/BlockBlobSidecarsTracker.java
Outdated
Show resolved
Hide resolved
|
@zilm13 yes you're right, I promised my self to review synchronization. I'll do a second pass tomorrow |
tbenr
force-pushed
the
BlobSidecarPoolAndTrackers
branch
from
aa019f0 to
a691d5f
Compare
various simplifications and some bug fix
introduce fetching on all blobsidecars if block is unknown
tbenr
force-pushed
the
BlobSidecarPoolAndTrackers
branch
from
b09dee6 to
16670a5
Compare
| blockBody.getAndSet( | ||
| Optional.of(BeaconBlockBodyDeneb.required(block.getMessage().getBody()))); | ||
| if (oldBlock.isPresent()) { | ||
| LOG.debug("block was already set!"); |
There was a problem hiding this comment.
It's useless without any extra info (block info or whatever), and we could hit it very often. I'd remove it
There was a problem hiding this comment.
going to remove it for now, will put it in the debugging PR to show timings
The basic idea is to have a pool of trackers for each
blockRootwe see (coming from a block or from a sidecar)each tracker have the responsibility to signal when a block-blobs bundle is completed
the pool is currently responsible of signal to fetchers (block and blobs) to retrieve missing content. This logic is triggered only when dealing with object related to current slot (following the head, content coming from gossip)
the pool implements a fetch delay logic. There are three constants:
TARGET_WAIT_MILLIS: ideal delay before fetching (currently set to 1s)MIN_WAIT_MILLIS: minimum delay before fetching (currently set to 0.5s). The rationale is that we don't want to issue fetches if we are most probably still receiving block and blobs.MAX_WAIT_RELATIVE_TO_ATT_DUE_MILLIS: maximum "distance" from attestation due (4s) when we really want to have block and blobs to process (currently set to 1.5s). The rationale is that, if we are close to attestation due, even if we may still receive something from gossip, we are in rush and we can waste some bandwidth to have more chance to import before the 4s mark (and attest to correct head)currently if we try to fetch blobsidecars and we don't have the corresponding block yet, we try to fetch all possible sidecars (considering maxBlobsPerBlock). If we receive the block after fetch occurs, we drop the (eventually) non existing blobs.
fixes #6849
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog