Encode primitives through an infallible Sink - #522
Conversation
ConsensusEncode writes into Sink instead of std::io::Write, so encoding cannot fail and callers stop discarding Ok. Headers hash the 80-byte layout directly, sizes are analytic, genesis blocks are compiled bytes, and decode reserves from compact-size counts. Co-authored-by: metaphorics <metaphorics@users.noreply.github.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_748267ef-219f-4cad-b94a-8acd3ee8f7d9) |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoEncode primitives through an infallible Sink
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1. Untrusted count amplifies allocation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| #[test] | ||
| fn analytic_tx_size_matches_encoded_length_with_witness() { |
There was a problem hiding this comment.
1. Size test lacks contract reference 📘 Rule violation ▣ Testability
The new permanent analytic_tx_size_matches_encoded_length_with_witness test does not identify the BIP or another named current contract that defines its expected behavior. This leaves the test tied only to the local encoder implementation it compares against.
Agent Prompt
## Issue description
The new analytic transaction-size test does not identify the external or documented contract it protects.
## Issue Context
Permanent tests must reference a named current contract in the test name, an adjacent comment, or supported metadata. Add the applicable Bitcoin specification or canonical vector/reference and ensure the assertions are grounded in it rather than solely in the local encoder.
## Fix Focus Areas
- crates/primitives/src/encode.rs[633-655]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for input in &mut inputs { | ||
| let item_count = read_compact(reader)?; | ||
| let mut witness = Vec::new(); | ||
| let mut witness = Vec::with_capacity(bounded_capacity(item_count, reader.len(), 1)); |
There was a problem hiding this comment.
2. Untrusted count amplifies allocation 🐞 Bug ⛨ Security
decode_tx reserves one Vec<u8> witness slot per remaining input byte when an attacker declares a huge item count, before validating even the first item, despite each slot consuming substantially more memory than its encoded empty-item byte. A validly framed peer transaction or block—up to 32 MiB under the current limit—can therefore trigger a hundreds-of-megabytes allocation and then fail decoding, enabling remote process OOM where the prior incremental Vec::new() path avoided the up-front allocation.
Agent Prompt
## Issue description
Witness-stack decoding eagerly preallocates a `Vec<Vec<u8>>` with one element slot per remaining serialized byte based on an untrusted witness count. Because an in-memory `Vec<u8>` slot is substantially larger than the one-byte encoding of an empty witness item, malformed input can cause a disproportionately large allocation before the first item's length check rejects it.
## Issue Context
Keep the serialized-input count bound and existing decoding behavior unchanged, but make witness-stack capacity allocation-aware by applying a conservative ceiling to eager reservations or growing the vector incrementally. The previous `Vec::new()` behavior did not perform this payload-proportional up-front allocation; add a regression test with a huge compact-size witness count and substantial trailing input, including a malformed oversized first item, to verify rejection occurs without reserving millions of witness slots. The decoder is reachable through accepted peer transaction and block payloads, whose size limit permits these multi-million-element reservation attempts.
## Fix Focus Areas
- crates/primitives/src/encode.rs[207-214]
- crates/primitives/src/encode.rs[395-403]
- crates/p2p/src/wire.rs[25-25]
- crates/p2p/src/wire.rs[390-402]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (2) 🔗 Fix PR: #531 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 2 fixed
|
2 participants
Stacked on #483. Breaking encode path for native primitives.
ConsensusEncodeno longer usesstd::io::Write. Encoding writes into an infallibleSink(Vec<u8>, hash engines, counters, and the apply-path byte-equality check). Callers stop discardingio::Result.Also:
Header::to_bytes/from_bytesand header hashing over the 80-byte layoutconsensus_size/base_size/stripped_size(no counting writer walk)Veccapacity bounded by remaining inputWorkspace call sites in p2p wire, UTXO undo, chainstate journal, checkpoints, and apply-path byte equality are updated.
Next stacked PR: native field newtypes in #597.