Validate v1 message field lengths and handle missing check points

[eval-exec]

This PR strengthens validation for V1 proof responses and avoids treating a local checkpoint-storage inconsistency as a peer error.

• Validate that blocks_uncles_hash and blocks_extension contain exactly one entry for each returned header in:
  • SendBlocksProofV1
  • SendTransactionsProofV1
• Perform this structural validation before empty-response and changed-last-state early returns.
• Return MalformedProtocolMessage for mismatched V1 field lengths.
• Replace the panic when an expected local checkpoint is missing with InternalError.

[eval-exec]

I think this PR is ready to merge. Request @chenyukang @quake to review.

[Copilot]

Pull request overview

This PR hardens the light-client and filter protocols against malformed inputs by validating v1 proof message field lengths before use, and by avoiding panics when expected local check points are missing.

Changes:

• Add v1 blocks_uncles_hash / blocks_extension length checks in SendBlocksProof and SendTransactionsProof before verifying/storing data.
• Refactor proof handlers to reuse parsed headers/filtered blocks (minor control-flow cleanup).
• Replace a .expect() on cached check point retrieval with a let ... else and an explicit InternalError status.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
light-client-lib/src/protocols/light_client/components/send_transactions_proof.rs	Validates v1 extra-field lengths against filtered block headers before verification/storage.
light-client-lib/src/protocols/light_client/components/send_blocks_proof.rs	Validates v1 extra-field lengths against headers before verification/storage.
light-client-lib/src/protocols/filter/components/block_filters_process.rs	Replaces a panic on missing cached check point with a graceful error status return.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

light-client-lib/src/protocols/light_client/components/send_transactions_proof.rs:73

• The v1 length-mismatch error message reports headers= but the value printed is filtered_blocks_len (count of filtered_blocks). This is confusing when diagnosing malformed messages. Consider renaming the label to match what’s being counted (or rename the variable to headers_len).

                let error_message = format!(
                    "SendTransactionsProof v1 field length mismatch: \
                     headers={}, uncle_hashes={}, extensions={}",
                    filtered_blocks_len, uncle_hashes_len, extensions_len
                );

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

light-client-lib/src/tests/protocols/light_client/send_blocks_proof.rs:633

• returned_uncles_hash is a vector field; consider renaming it to returned_uncles_hashes for clarity/consistency with other Vec-typed fields.

    returned_uncles_hash: Option<Vec<packed::Byte32>>,

light-client-lib/src/tests/protocols/light_client/send_blocks_proof.rs:733

• This block uses the singular names returned_uncles_hash / uncles_hash even though the value is a collection; plural naming would be clearer and match the field’s actual type.

            if let Some(uncles_hash) = &param.returned_uncles_hash {
                let content = packed::SendBlocksProofV1::new_builder()
                    .last_header(last_header)
                    .proof(proof)
                    .headers(headers.pack())
                    .missing_block_hashes(param.returned_missing_block_hashes.clone().pack())
                    .blocks_uncles_hash(uncles_hash.to_owned().pack())
                    .build();

light-client-lib/src/tests/protocols/light_client/send_transactions_proof.rs:465

• GetTransactionsProof is built here without tx_hashes, which makes this test case unrealistic compared to production behavior (the protocol always sets tx_hashes when sending requests). This can reduce the test’s ability to catch regressions in request/response matching, since an empty response only passes check_tx_hashes when the request is also empty.

        let txs_proof_request = packed::GetTransactionsProof::new_builder()
            .last_hash(if last_state_changed {
                packed::Byte32::default()
            } else {
                last_header.header().calc_header_hash()
            })
            .build();

light-client-lib/src/tests/protocols/light_client/send_blocks_proof.rs:455

• The field name returned_uncles_hash is a Vec and is used as a collection of hashes; using a plural name would be clearer and consistent with other vector fields in this test setup.

This issue also appears in the following locations of the same file:

• line 633
• line 726

        returned_uncles_hash: Some(vec![packed::Byte32::default()]),
        expected_status: Some(StatusCode::MalformedProtocolMessage),

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

light-client-lib/src/protocols/light_client/components/send_transactions_proof.rs:76

• count_extra_fields() >= 2 will treat any message with 2+ extra fields as V1, and will silently treat a message with exactly 1 extra field as a legacy message (skipping V1 structural validation + extra-hash verification). That allows structurally malformed/unknown extra-field layouts to bypass the intended validation. Consider rejecting any count_extra_fields() value other than 0 or 2 as MalformedProtocolMessage, and derive is_v1 from an exact match.

        let filtered_blocks_len = self.message.filtered_blocks().len();
        let is_v1 = self.message.count_extra_fields() >= 2;
        if is_v1 {

light-client-lib/src/protocols/light_client/components/send_blocks_proof.rs:76

• count_extra_fields() >= 2 will treat any message with 2+ extra fields as V1, and will silently treat a message with exactly 1 extra field as a legacy message (skipping V1 structural validation + extra-hash verification). That allows structurally malformed/unknown extra-field layouts to bypass the intended validation. Consider rejecting any count_extra_fields() value other than 0 or 2 as MalformedProtocolMessage, and derive is_v1 from an exact match.

        let headers_len = self.message.headers().len();
        let is_v1 = self.message.count_extra_fields() >= 2;
        if is_v1 {

[eval-exec]

@chenyukang @quake I think this PR is ready to merge.

[knoal]

Hey @eval-exec — clean security fix. The placement of the new validation BEFORE the empty-response and changed-last-state early returns is exactly right: a malicious peer can't bypass it by sending headers=[] with populated blocks_uncles_hash / blocks_extension arrays. Three things I noted while reviewing:

LGTM with minor nits

1. Right placement. Length check fires before if all blocks missing and if last state changed early returns — closes a potential DoS vector (peer forcing the receiver to allocate large arrays even when "no blocks need verification").

2. Symmetric application. Same pattern in both send_blocks_proof.rs and send_transactions_proof.rs — uncle_hashes.len() != headers_len || extensions.len() != headers_len.

3. DRY refactor. headers_len / filtered_blocks_len and is_v1 are hoisted to the top of each method (computed once, used by both the new length check and the existing post-validation path). Removes a duplicated is_v1 computation.

4. InternalError over panic! for missing local checkpoint is the right pattern — a local storage inconsistency is not a peer-protocol error and shouldn't crash the process.

Minor nits (not blocking)

• new_unchecked + as_slice() is appropriate for the V0/V1 split (decided by count_extra_fields() >= 2 before the cast), but it's worth noting that the read happens outside molc's bounds-checked path. A more defensive read could use new_opt() + explicit error handling. NIT only.

• Refactor noise: the rename of self.message.headers().is_empty() → headers.is_empty() (and the filtered_blocks equivalent) expands the diff beyond the security fix. Necessary for the hoist, but reviewers will need to scan more lines. NIT only.

• Positive-case test: the new tests cover the reject path. An accepts_v1_fields_when_all_blocks_match test would document the invariant that valid V1 messages with matching lengths still pass. Existing tests likely cover this implicitly via the ..Default::default() updates, but explicit positive coverage would be nice. Optional follow-up — happy to send a PR adding it if useful.

What I'm offering to take on

None required for this PR — it's focused and well-executed. If you'd like the optional positive-case test as a follow-up, I'm happy to send a small PR.

B-side MCE review (B1 MEDIUM p=0.65, B2-B3 LOW) is at /agents/onyx_bot/nervos-r1-2026-07-28/b-side/pr-288/b-review-pr-288.md for the longer-form analysis.

— Onyx