Light forward sync mechanism #6515
Conversation
cheatfate
force-pushed
the
hybrid-sync
branch
2 times, most recently
from
5be1b71
to
575d711
Compare
beacon_chain/beacon_chain_file.nim
Outdated
| header: ChainFileHeader, | ||
| data: openArray[byte] | ||
| ): Result[BlobSidecar, string] = | ||
| if header.plainSize > uint64(high(uint32)): |
There was a problem hiding this comment.
Can be more precise than this. Uncompressed blobs are 32 * 4096 = 131,072 bytes:
nimbus-eth2/beacon_chain/spec/datatypes/deneb.nim
Lines 53 to 54 in bf4abf8
Snappy might increase their size somewhat in pathological cases, but it has bounds on this.
There was a problem hiding this comment.
Such cases are not pathological because we here mostly working with high entropy sources (crypto hashes and or encrypted data).
Decreased to MaxChunkSize in 361bc0b
beacon_chain/beacon_chain_file.nim
Outdated
| header: ChainFileHeader, | ||
| data: openArray[byte] | ||
| ): Result[ForkedSignedBeaconBlock, string] = | ||
| if header.plainSize > uint64(high(uint32)): |
There was a problem hiding this comment.
Can bound more tightly: https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/specs/phase0/p2p-interface.md#configuration and https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/configs/mainnet.yaml and https://github.com/ethereum/consensus-specs/blob/v1.5.0-alpha.5/configs/minimal.yaml note that GOSSIP_MAX_SIZE is 10MiB (for "uncompressed gossip messages").
That limit applies to all gossiped SSZ objects, including blocks (and blobs, but blobs are simply fixed-size).
| buffer = Chunk.init(kind, uint64(slot), uint32(plainSize), data) | ||
| wrote = writeFile(chandle.handle, buffer).valueOr: | ||
| discard truncate(chandle.handle, origOffset) | ||
| discard fsync(chandle.handle) |
There was a problem hiding this comment.
Why are some fsync calls fine to discard/ignore the return value of but others not?
There was a problem hiding this comment.
So this is because of error in writeFile (initial error we going to report to user), so this is attempt to cleanup. In case where number of bytes written was less than we expect, it means that file become inconsistent, so we trying to truncate file to known good size and we trying to fsync it, but we still want to report original error message to the user.
cheatfate
force-pushed
the
hybrid-sync
branch
from
bc640a0
to
361bc0b
Compare
There was a problem hiding this comment.
This is a great improvement for the security of default sync compared to the existing genesis sync.
As this is timely to properly test, would be great to have at least a happy case test with a few epochs worth of minimal blocks to avoid accidental regressions.
If data size is a concern, one may consider syncing 1 (or N) sync committee period (~27 hrs / 8192 slots) at a time by treating each intermediate light client sync step as its own separate sync target, applying each of them separately before syncing to the next period. The hook to obtain intermediate results is LightClientUpdateObserver. It would add quite a bit of complexity, though, so maybe waiting for demand is better for now.
| @@ -92,6 +89,8 @@ proc initLightClient*( | |||
| .shouldSyncOptimistically(node.currentSlot): | |||
| return | |||
There was a problem hiding this comment.
The new sync mechanism works for all lcDataFork > LightClientDataFork.None, but here we have an additional >= Capella restriction, because that's required for the EL blockHash and setOptimisticHead.
All networks that we actively support have advanced past Capella, so letting this remain in the >= Capella section is also fine.
|
|
||
| let blockHandler: BlockAdded(consensusFork) = onBlockAddedHandler | ||
|
|
||
| discard addResolvedHeadBlock( |
There was a problem hiding this comment.
addResolvedHeadBlock calls dag.processNewBlockForLightClient(state, trustedBlock, parent.bid), and that may trigger a beacon state root computation.
If conf.lightClientDataImportMode is set to Full, this is not avoidable, as the user explicitly indicates interest in full historical data availability. However, if it is set to OnlyNew or OnDemand, the new sync overseer could gain some performance by setting dag.lcDataStore.cache.tailSlot to the LC sync target so that the state root computation can be avoided until the sync has caught up to the LC target.
There was a problem hiding this comment.
The general idea of downloading trust-minimized data into a separate ___location before processing it is great.
I wonder though, is there an overlap between this format and .era/.erb files?
And if no, is there an advantage of using a custom format over era?
There was a problem hiding this comment.
ERA files is more about static storage of limited number of blocks + state, without any need to read it from the tail.
Current file and format is made with sense that we downloading files from the head to tail and applying from tail to head.
cheatfate
force-pushed
the
hybrid-sync
branch
from
361bc0b
to
bbce4d3
Compare
Add more information when proposer verification failed.
…t after LC block received.
Remove some compilation warnings. Fix `CatchableError` leaks on Windows.
cheatfate
force-pushed
the
hybrid-sync
branch
from
bbce4d3
to
30e69cd
Compare
No description provided.