fix(rpc): report oversized chain_length as error instead of panicking - #2504
fix(rpc): report oversized chain_length as error instead of panicking#2504erhnysr wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cf21e4843
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * [FIX][rust] The RPC retry policy is now endpoint-aware: `SubmitProvenTransaction` and `SubmitProvenBatch` retry only `ResourceExhausted` and let `Unavailable` propagate, while read endpoints keep retrying both. `Unavailable` does not say whether the node processed the request, so resubmitting could hit the nullifier consumed by an accepted copy and report a conflict indistinguishable from a genuine double spend, hiding the original success ([#2441](https://github.com/0xMiden/rust-sdk/issues/2441)). | ||
| * [FIX][cli] `-V`/`--version` now work when the binary is invoked under a different name, such as through the `miden client` shim installed by midenup ([#2486] https://github.com/0xMiden/rust-sdk/pull/2486)). | ||
| <!-- TODO: update the PR link below to the actual PR number once the PR is opened (currently a placeholder). --> | ||
| * [FIX][rust] `get_block_header_by_number` now reports a `chain_length` that does not fit in `usize` as `RpcError::InvalidResponse` instead of panicking. On a 32-bit target such as `wasm32`, a node returning a chain length above `u32::MAX` would abort the client; the conversion now mirrors the `map_err` already used on the adjacent forest-size check ([#XXXX](https://github.com/0xMiden/rust-sdk/pull/XXXX)). |
There was a problem hiding this comment.
Replace the placeholder changelog reference
This Unreleased entry will ship with a literal #XXXX link, which points to a nonexistent pull request and leaves released users without a valid reference for the behavioral fix. Replace it with the actual PR number (and remove the accompanying TODO) before merging.
Useful? React with 👍 / 👎.
|
Independent confirmation of this bug at real 32-bit pointer width, since the PR notes the failing path "cannot be exercised on the 64-bit host the suite runs on." I compiled the exact expression from // mirrors the pre-fix line: usize::try_from(forest).expect("u64 should fit in usize")
#[no_mangle]
pub extern "C" fn forest_size_from_node(hi: u32, lo: u32) -> u32 {
let forest: u64 = ((hi as u64) << 32) | (lo as u64);
let forest_size = usize::try_from(forest).expect("u64 should fit in usize");
forest_size as u32
}const { instance } = await WebAssembly.instantiate(bytes, {});
const f = instance.exports.forest_size_from_node;
console.log('chain_length=5:', f(0, 5)); // normal value
try { f(1, 0); } // 0x1_0000_0000 = 2^32 = u32::MAX + 1
catch (e) { console.log('chain_length=2^32 ->', e.constructor.name, '-', e.message); }Output: A normal chain length returns fine; |
get_block_header_by_number converted the node-supplied chain_length to a
Forest size with usize::try_from(forest).expect("u64 should fit in usize").
On a 32-bit target such as wasm32, which this crate supports, usize is 32
bits, so a chain_length above u32::MAX makes the conversion fail and the
expect abort the client on data the node controls.
Convert with map_err reporting RpcError::InvalidResponse instead, mirroring
the Forest::new check on the following line.
This is item 1 of 0xMiden#2378. 0xMiden#2379 closed that issue and its description covers
this conversion, but the merged change only touched rpc/domain/merkle.rs
(item 2, the MerklePath sibling bound); this line was never actually
changed.
45c1c3c to
4759ae2
Compare
Summary
get_block_header_by_numberin the tonic client converts the node-suppliedchain_lengthinto aForestsize with:chain_lengthis au64taken straight from the response. On a 32-bit target such aswasm32-unknown-unknown— which this crate supports (make build-wasm, the "Build wasm32 (no_std)" CI job, the wasm32 publish dry-run) —usizeis 32 bits, so anychain_lengthaboveu32::MAXmakes the conversion fail and theexpectabort the client on data the node controls. The message asserts something that is not true on a supported target.The very next line already handles the same value gracefully:
Change
Convert
chain_lengthwithmap_err, reportingRpcError::InvalidResponse, so the conversion mirrors the adjacentForest::newcheck and theMmrDeltaconversion inrpc/domain/merkle.rs. One line; no behavior change on 64-bit targets, where the conversion cannot fail.Relationship to #2378 / #2379
This is item 1 of #2378. #2378 documented two panics in this method; PR #2379 closed it and its description explicitly covers this exact conversion ("Convert
chain_lengthwithmap_err, reportingRpcError::InvalidResponse"). However, the merged diff for #2379 only touchedcrates/rust-client/src/rpc/domain/merkle.rs— item 2, theMerklePathsibling bound. Thetonic_client/mod.rschange described for item 1 was never actually committed, so theexpectis still onnext. This PR completes the fix #2378 asked for. (I've left a note on #2378.)Found while sweeping this crate for the same untrusted-input hardening class as #2419 / #2381 / #2386 / #2454.
Verification
make lint— clean (cargo fix,cargo +nightly fmt,taplo fmt,cargo clippy --workspace --features "testing std" --all-targets -- -D warnings,cargo shear), no warningsmake test— 457 tests run: 457 passed, 2 skippedNo dedicated test: the failing path is reachable only where
usizeis narrower thanu64(32-bit / wasm), so it cannot be exercised on the 64-bit host the suite runs on. #2379 shipped no test for this item for the same reason.Completes #2378