Sync Loop NPT 25 26 - #956
Closed
aszepieniec wants to merge 2 commits into
Closed
Conversation
The `BlockValidationError` arm of the successors-task result match logged "Terminating sync loop" but did not break out of the event loop. Control fell through to the respawn check, which found the offending block still covered — it is only deleted after a successful send to the main loop — and spawned a new successors subtask against the same tip. That subtask re-read the same block, failed the same validation, and returned the same error, forever. Unlike the `Continue` path, the error path has no sleep, so this is a hot loop that re-runs `Block::is_valid` — proof verification included — on a pegged core while sync never completes. A peer can reach it: the sync loop's `receive_block` performs no validation, and the peer loop only runs `solo_validate` before forwarding, which deliberately leaves the block's place in the chain unchecked. A solo-valid block from another fork at a covered height is enough. Every other arm of that match breaks. Add the missing break. Test `invalid_tip_successor_terminates_sync_loop` asserts the sync loop task ends within 10 seconds when a tip-successor fails validation; it times out without this fix. The existing test-only `BlockValidator::Test` accepts everything, so this adds a `TestReject` variant and a way to inject it before the loop starts. Addresses NPT-25 in R8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`RapidBlockDownload::receive_block` stores whatever block a peer offers
and binds the height to it. Nothing about the block's place in the chain
is checked: upstream, the peer loop runs only `solo_validate`, which by
its own documentation does not establish that the block belongs to any
chain -- difficulty and proof-of-work relative to the parent fall outside
it. A block mined on a private low-difficulty fork passes.
Worse, the binding could not be undone. `SynchronizationBitMask` had no
`unset`, and `canonize` folds a newly covered height below `lower_bound`,
where bits are implicitly one forever. The tip-successor slot is the one
that gets folded immediately. Since block requests are drawn from the
complement of that bit mask, a bound height is never asked for again: one
peer winning the race for one height denied the whole sync, repeatedly
and at little cost.
Make the binding revocable:
- `SynchronizationBitMask::unset` walks `lower_bound` back down to the
index, materializing the limbs `canonize` dropped on the way up. The
limbs it reclaims lie entirely below the old bound, so they are all
ones. A complete bit mask keeps no limbs at all, so the limb holding
the bound is put back first, with only the bits below the bound set --
the bits at and beyond it are the implicit zeros and `pop_count`
reads them raw.
- `RapidBlockDownload::reject_block` deletes the stored block and
un-covers its height, putting it back on the to-do list.
- `BlockValidationError` and `BlockPowError` now carry the offending
height and the tip reached before the failure. The tip matters: the
subtask may have processed blocks before hitting the bad one, and
that progress used to be discarded.
- The sync loop discards the block and asks for the height again rather
than aborting, giving up only after `MAX_REJECTED_BLOCKS`. This
replaces the unconditional break added for NPT-25; eviction subsumes
it, and the retry bound still terminates the loop instead of spinning
on a block that can never validate.
Test `poisoned_height_slot_does_not_prevent_sync` has a peer answer one
height with a block the validator rejects, withheld until every other
height is downloaded, and then behave honestly. Sync must still complete;
before this change it aborted with the tip never advancing.
`rejected_block_frees_its_height_slot` covers the eviction itself, and
three tests cover `unset`, including the complete-bit-mask case where no
limbs exist to walk back into.
Addresses NPT-26 in R8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sword-Smith
reviewed
Jul 31, 2026
Comment on lines
+29
to
+37
| /// Rejects every block; for testing how the sync loop handles a | ||
| /// tip-successor that fails validation. | ||
| #[cfg(test)] | ||
| TestReject, | ||
| /// Rejects one specific block; for testing how the sync loop handles a | ||
| /// height slot poisoned with a block that does not belong to the chain | ||
| /// being synced. | ||
| #[cfg(test)] | ||
| TestRejectDigest(tasm_lib::prelude::Digest), |
Member
|
|
Member
I'll fix these two cosmetic problems. |
Member
|
Merged through 2142d4d |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Harden the sync loop against DoS attacks.