Validate downloaded block data before indexing - #290
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens the light client synchronizer by validating that downloaded full-block bodies are consistent with the commitments in their already-proved headers before the blocks are accepted for indexing/caching. This closes a correctness gap where a proved header could otherwise be paired with an inconsistent block body response from a peer.
Changes:
- Add body-vs-header commitment validation for
SendBlockmessages when the block is already marked as proved. - Ban peers that send inconsistent proved-block bodies and keep the matched block pending for retry with other peers.
- Add synchronizer tests covering several body/commitment mismatch cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| light-client-lib/src/protocols/synchronizer.rs | Adds proved-block body validation during SendBlock handling. |
| light-client-lib/src/tests/protocols/synchronizer.rs | Adds tests asserting inconsistent block bodies are rejected and do not clear pending matched blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
light-client-lib/src/protocols/synchronizer.rs:112
- The write-lock recheck currently treats any proved entry as requiring validation (
is_proved && !body_validated). If the block is already cached, this can cause unnecessary loop churn (and, ifshould_validateis updated to checkblock.is_none(), can even create a spin loop). Mirror the same condition under the write lock: only force validation when proved and the cached block is stillNone.
let is_proved = matched_blocks
.get(&block_hash_key)
.is_some_and(|(proved, _)| *proved);
if is_proved && !body_validated {
drop(matched_blocks);
continue;
}
light-client-lib/src/protocols/synchronizer.rs:86
should_validatebecomes true for any proved matched block, even if the block body is already cached. This means a peer can repeatedly send the same proved block and force repeated (potentially expensive) root/commitment recomputation each time. Consider validating only when the matched-block entry is proved and still missing its cached block (OptionisNone).
This issue also appears on line 106 of the same file.
.matched_blocks()
.read()
.await
.get(&block_hash_key)
.is_some_and(|(proved, _)| *proved);
Summary
Why
A header proof authenticates the header. The downloaded full block must also match the commitments in that header before its transactions are indexed.
Behavior
Valid proved blocks continue through the existing indexing path. Validation runs only for matched blocks already marked as proved. Inconsistent responses are rejected before the block is cached or the pending queue is cleared.
Validation
cargo fmt --all --checkcargo clippy -p ckb-light-client-lib --locked -- --deny warningscargo clippy --target wasm32-unknown-unknown -p light-client-wasm -p ckb-light-client-lib -p light-client-db-common -p light-client-db-worker --locked -- --deny warningscargo nextest run -p ckb-light-client-lib synchronizer(6 passed)cargo nextest run --hide-progress-bar --success-output immediate --failure-output immediate -p ckb-light-client-lib(112 passed)The combined library and CLI test command could not start in this environment because
tikv-jemalloc-sysfailed to build against the local Nix/glibc toolchain.