Repository navigation
Conversation
Two changes that together unblock real-mainnet sync through GSA. 1) Stream-bound entries are picked by (slot, hash), not position. `checkEntryMatch` was a positional pick (listToMaybe / last) followed by a hash check. That's wrong when multiple entries share a slot — the canonical case being the Byron EBB and the first main block of an epoch. A range request for the EBB alone landed on the main block, failed the hash check, and surfaced as `StreamBoundNotFound`; the symptom on a fresh-node mainnet sync was BlockFetch sending `MsgStartBatch` then going silent. Replaced with `dropUntilLowerBound` / `truncateAtUpperBound`, which scan by (slot, hash). Both remember the most recent same-slot non-match so `StreamBoundNotFound` still carries the near-miss entry (preserves behaviour expected by `prop_onDemandIteratorFromErrorsWhenStartingWithSlotNumberOnChainButWrongHeaderHash`). 2) ChainSync intersection candidates are probed against the on-disk chain. `cardano-node` 10.6.x emits a synthetic slot-0 hash in `MsgFindIntersect` that doesn't match the Byron EBB on disk. The old slot-only `withinTip` check accepted it, the iterator failed the hash equality, and the responder died. `findUsableIntersection` now opens an iterator on each candidate to confirm it's on our chain; non-matches are rejected and the loop tries the next candidate, so the bootstrap pattern of bogus-block-then-GenesisPoint correctly falls back to Origin. Same idiom as ouroboros-consensus's `Impl.Follower.forward`, which uses `streamAfterPoint` as its on-chain test.
34d228e to
6982de5
Compare
Add unit tests covering the Byron EBB / first-main-block collision: the scenario where two entries occupy the same slot and the bound helpers must distinguish them by hash rather than position. These would have failed against the previous positional 'checkEntryMatch' logic.
…goes beyond the sought bound
…he near-miss iteration unit tests
| (getE es) | ||
| dropUntilLowerBound ci fromSlot fromHash = go Nothing | ||
| where | ||
| -- 'mbNear' = most recent same-slot non-match, reported in StreamBoundNotFound. |
There was a problem hiding this comment.
| -- 'mbNear' = most recent same-slot non-match, reported in StreamBoundNotFound. | |
| -- 'mbNear' = most recently processed same-slot non-match, to report in StreamBoundNotFound. |
This way, it's clearer that the notion of recency applies to the algorithmic iteration, rather than somewhat suggesting that it's more recent on the blockchain (in the sense that a slot is an atomic unit in time, and therefore things in the same slot cannot be more or less recent than one another). The other small change here ("to report in") I think makes it clearer that the last slot of StreamBoundNotFound won't always be an entry from the requested slot, but rather could be something from another slot. Small points, but I think worth the precision in the wording. WDYT?
| Either (IllegalStreamOperation blk) [SizedEntry blk] | ||
| truncateAtUpperBound ci toSlot toHash = go [] Nothing | ||
| where | ||
| -- 'mbNear' = most recent same-slot non-match (see 'dropUntilLowerBound'). |
There was a problem hiding this comment.
| -- 'mbNear' = most recent same-slot non-match (see 'dropUntilLowerBound'). | |
| -- 'mbNear' = most recently processed same-slot non-match (see 'dropUntilLowerBound'). |
| sizedEntry :: SizedEntry blk -> Entry blk | ||
| sizedEntry (WithBlockSize _ e) = e |
There was a problem hiding this comment.
I'd call this something else, like sizedEntryToEntry or entryFromSizedEntry, since it feels off to read, e.g., sizedEntry e where e is already a SizedEntry blk.
|
|
||
| -- ChunkInfo where EBB epoch 0 maps to slot 0, sharing it with the first main block. | ||
| sharedSlotChunkInfo :: ChunkInfo | ||
| sharedSlotChunkInfo = UniformChunkSize $ ChunkSize True 10 |
There was a problem hiding this comment.
Maybe add a note that the 10 is entirely arbitrary, we remember later and don't get confused?
| unit_dropUntilLowerBound_picksMainBlockAtSharedSlot = | ||
| case dropUntilLowerBound sharedSlotChunkInfo (SlotNo 0) mainHash sharedSlotEntries of | ||
| Right (WithBlockSize _ e : _) -> headerHash e @?= mainHash | ||
| other -> fail $ "expected suffix starting at main block, got " ++ show other |
There was a problem hiding this comment.
"suffix" here feels odd, even if I know what you mean. Maybe just "result" or "iterable"?
| unit_truncateAtUpperBound_picksEBBAtSharedSlot = | ||
| case truncateAtUpperBound sharedSlotChunkInfo (SlotNo 0) ebbHash sharedSlotEntries of | ||
| Right entries -> map (headerHash . sizedEntry) entries @?= [ebbHash] | ||
| other -> fail $ "expected prefix [EBB], got " ++ show other |
There was a problem hiding this comment.
Analogous to the comment about "suffix", it feels a bit more natural to call "prefix" something like "result".
| unit_truncateAtUpperBound_picksMainBlockAtSharedSlot = | ||
| case truncateAtUpperBound sharedSlotChunkInfo (SlotNo 0) mainHash sharedSlotEntries of | ||
| Right entries -> map (headerHash . sizedEntry) entries @?= [ebbHash, mainHash] | ||
| other -> fail $ "expected prefix [EBB, main], got " ++ show other |
There was a problem hiding this comment.
Analogous to the comment about "suffix", it feels a bit more natural to call "prefix" something like "result".
Couple additional test cases + assorted tweaks to test cases
This PR introduces two fixes surfaced during mainnet benchmarking:
checkEntryMatchfirst picked a candidate block by position then checked the hash. The problem is that blocks can share a slot in the very special case of Byron EBBs, where the first slot of each epoch contains a two blocks. This lead toStreamBoundNotFounderrors when the block being picked was not the one requested.When syncing from an empty chain,
cardano-nodeactually sends two candidates:a) A synthetic anchor computed from Origin and
genesis.jsonfile.b) The Genesis origin point.
I am not exactly sure what the rationale is behind this, but we would previously accept the synthetic anchor without checking, which would later result in an error when the node tries to fetch that anchor.