← Watch feed
primitives: remove a bunch of panics from Transaction::decoder
Public commit record
85/100 · Strong
What the developer wrote
Authored by Andrew Poelstra
primitives: remove a bunch of panics from Transaction::decoder
The state machine logic in TransactionDecoder is a bit frustrating. We
need to obtain a mutable reference to the current sub-decoder to attempt
to feed data to it. If this fails, we then need to move the sub-decoder,
attempt to end() it, then replace it if this succeeds. (And the replacement
uses the return value from end() so we can't reorder this.)
Having the end() between the move and replacement means we need a dummy
Transitioning state, and having a mutable borrow before the move means
we need some borrowck trickery.
This commit separates the mutable-borrow logic from the move logic to
avoid the trickery. It retains the Transitioning dummy state which I can't
find a way to avoid.
Refactor only; no observable behavior changes.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit is a code cleanup inside the Bitcoin transaction decoder. It removes helper functions that could panic if called in the wrong state and rewrites the decoder's main loop to avoid needing those helpers. The author explicitly states this is a refactor with no observable behavior changes, and the diff supports that reading: the same state machine, transitions, and error paths remain, just reorganized.
Recommended action
No security action required. Treat as normal code-quality review; verify tests still pass and fuzzing coverage remains equivalent.
Security signals we found
01
Removal of internal panic paths (defensive hardening)
02
Refactor of state-machine borrow/move pattern (no functional change)
03
No new input validation, no new unsafe code, no cryptographic changes
Evidence from the diff
The patch refactors TransactionDecoder in primitives/src/transaction.rs. It deletes five state-transition helper methods (version_transition, inputs_transition, outputs_transition, witness_transition, lock_time_transition) that used mem::replace into a Transitioning dummy state and panicked on unexpected variants. The Decoder::push_bytes implementation is restructured into two consecutive match statements: first, attempt to feed bytes to the active sub-decoder and return early if more bytes are needed; second, end the current sub-decoder and advance to the next state. The Transitioning state is retained as a placeholder. No protocol parsing logic, bounds checks, or public API semantics appear to change.
Changed components
primitives/src/transaction.rsTransactionDecoderDecoder for TransactionDecoderInspect captured patch +39 / −81
diff --git a/primitives/src/transaction.rs b/primitives/src/transaction.rs
index 89df54aa..9dff5501 100644
--- a/primitives/src/transaction.rs
+++ b/primitives/src/transaction.rs
@@ -374,51 +374,6 @@ impl Default for TransactionDecoder {
fn default() -> Self { Self::new() }
}
-#[cfg(feature = "alloc")]
-impl TransactionDecoderState {
- #[track_caller]
- fn version_transition(&mut self) -> VersionDecoder {
- match mem::replace(self, TransactionDecoderState::Transitioning) {
- TransactionDecoderState::Version(decoder) => decoder,
- _ => panic!("transition called on invalid state"),
- }
- }
-
- #[track_caller]
- fn inputs_transition(&mut self) -> VecDecoder<TxIn> {
- match mem::replace(self, TransactionDecoderState::Transitioning) {
- TransactionDecoderState::Inputs(_, _, decoder) => decoder,
- _ => panic!("transition called on invalid state"),
- }
- }
-
- #[track_caller]
- fn outputs_transition(&mut self) -> (Vec<TxIn>, VecDecoder<TxOut>) {
- match mem::replace(self, TransactionDecoderState::Transitioning) {
- TransactionDecoderState::Outputs(_, inputs, _, decoder) => (inputs, decoder),
- _ => panic!("transition called on invalid state"),
- }
- }
-
- #[track_caller]
- fn witness_transition(&mut self) -> (Vec<TxIn>, Vec<TxOut>, WitnessDecoder) {
- match mem::replace(self, TransactionDecoderState::Transitioning) {
- TransactionDecoderState::Witnesses(_, inputs, outputs, _, decoder) =>
- (inputs, outputs, decoder),
- _ => panic!("transition called on invalid state"),
- }
- }
-
- #[track_caller]
- fn lock_time_transition(&mut self) -> (Vec<TxIn>, Vec<TxOut>, LockTimeDecoder) {
- match mem::replace(self, TransactionDecoderState::Transitioning) {
- TransactionDecoderState::LockTime(_, inputs, outputs, decoder) =>
- (inputs, outputs, decoder),
- _ => panic!("transition called on invalid state"),
- }
- }
-}
-
#[cfg(feature = "alloc")]
#[allow(clippy::too_many_lines)] // TODO: Can we clean this up?
impl Decoder for TransactionDecoder {
@@ -433,25 +388,52 @@ impl Decoder for TransactionDecoder {
};
loop {
+ // Attempt to push to the currently-active decoder and return early on success.
match &mut self.state {
State::Version(decoder) => {
if decoder.push_bytes(bytes)? {
// Still more bytes required.
return Ok(true);
}
- let decoder = self.state.version_transition();
- let version = decoder.end()?;
- self.state = State::Inputs(version, Attempt::First, VecDecoder::<TxIn>::new());
- }
- State::Inputs(version, attempt, decoder) => {
+ },
+ State::Inputs(_, _, decoder) => {
+ if decoder.push_bytes(bytes)? {
+ return Ok(true);
+ }
+ },
+ State::SegwitFlag(_) => {
+ if bytes.is_empty() {
+ return Ok(true);
+ }
+ },
+ State::Outputs(_, _, _, decoder) => {
+ if decoder.push_bytes(bytes)? {
+ return Ok(true);
+ }
+ },
+ State::Witnesses(_, _, _, _, decoder) => {
+ if decoder.push_bytes(bytes)? {
+ return Ok(true);
+ }
+ },
+ State::LockTime(_, _, _, decoder) => {
if decoder.push_bytes(bytes)? {
return Ok(true);
}
- // Copy the state because we need mutable access to self to transition.
- let version = *version;
- let attempt = *attempt;
+ },
+ State::Done(..) => return Ok(false),
+ State::Transitioning => {
+ panic!("use of decoder in transitioning state");
+ }
+ }
- let decoder = self.state.inputs_transition();
+ // If the above failed, end the current decoder and go to the next state.
+ match mem::replace(&mut self.state, State::Transitioning) {
+ State::Version(decoder) => {
+ let version = decoder.end()?;
+ self.state = State::Inputs(version, Attempt::First, VecDecoder::<TxIn>::new());
+ }
+ State::Inputs(version, attempt, decoder) => {
let inputs = decoder.end()?;
if Attempt::First == attempt {
@@ -475,11 +457,6 @@ impl Decoder for TransactionDecoder {
}
}
State::SegwitFlag(version) => {
- if bytes.is_empty() {
- return Ok(true);
- }
- let version = *version;
-
let segwit_flag = bytes[0];
*bytes = &bytes[1..];
@@ -488,17 +465,9 @@ impl Decoder for TransactionDecoder {
}
self.state = State::Inputs(version, Attempt::Second, VecDecoder::<TxIn>::new());
}
- State::Outputs(version, _, is_segwit, decoder) => {
- if decoder.push_bytes(bytes)? {
- return Ok(true);
- }
- // These types are Copy, so we can deref them but does not work for vectors.
- let version = *version;
- let is_segwit = *is_segwit;
-
+ State::Outputs(version, inputs, is_segwit, decoder) => {
// We get the inputs vector here instead of in the pattern match because I
// couldn't find another way to get it out of behind the mutable reference.
- let (inputs, decoder) = self.state.outputs_transition();
let outputs = decoder.end()?;
if is_segwit == IsSegwit::Yes {
self.state = State::Witnesses(
@@ -513,14 +482,9 @@ impl Decoder for TransactionDecoder {
State::LockTime(version, inputs, outputs, LockTimeDecoder::new());
}
}
- State::Witnesses(version, _, _, iteration, decoder) => {
- if decoder.push_bytes(bytes)? {
- return Ok(true);
- }
- let version = *version;
+ State::Witnesses(version, mut inputs, outputs, iteration, decoder) => {
let iteration = iteration.0;
- let (mut inputs, outputs, decoder) = self.state.witness_transition();
inputs[iteration].witness = decoder.end()?;
if iteration < inputs.len() - 1 {
self.state = State::Witnesses(
@@ -539,13 +503,7 @@ impl Decoder for TransactionDecoder {
State::LockTime(version, inputs, outputs, LockTimeDecoder::new());
}
}
- State::LockTime(version, _, _, decoder) => {
- if decoder.push_bytes(bytes)? {
- return Ok(true);
- }
- let version = *version;
-
- let (inputs, outputs, decoder) = self.state.lock_time_transition();
+ State::LockTime(version, inputs, outputs, decoder) => {
let lock_time = decoder.end()?;
self.state = State::Done(Transaction { version, lock_time, inputs, outputs });
return Ok(false);
Risk score
Our methodology →Why this scored 13/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.