primitives: Implement decoding traits by tcharding · Pull Request #4998 · rust-bitcoin/rust-bitcoin

tcharding · 2025-09-17T06:06:17Z

Things that need careful attention please:

The state machine in TransactionDecoder
The WitnessDecoder::empty kludge

tcharding · 2025-09-17T06:08:23Z

cc @nyonson, here is a quick and dirty go at starting on primitives. You can see from this more or less how I envisage the errors stuff will look. This might help you if you want to keep thinking about removing Either. I don't mean to take over from you with the decoding stuff so if you push anything I'll give you preference. I'm just pumped to get this done

nyonson · 2025-09-22T17:42:17Z

I am a little confused by the commit message in 3adce0b for the error type wrappers. You said "If two encoders return the same error it is not currently possible to compose them with our decoders", but I don't think that is true? Couldn't a composed decoder define a new type which just maps UnexpectedEofError in this case?

tcharding · 2025-09-22T22:20:05Z

I was trying to describe the same problem we have in the composite error in your PR, if ADecoder and BDecoder both return FooError then one cannot do Decoder2<ADecoder, BDecoder>.

nyonson · 2025-09-22T22:32:31Z

Decoder2<ADecoder, BDecoder>

Can't you just do Decoder2<ADecoder, BDecoder, FooError> like this test kinda does:

rust-bitcoin/consensus_encoding/tests/composition.rs

Line 142 in b84245f

let mut decoder2: Decoder2<_, _, UnexpectedEof> =

I think this is enabled by a identity From blanket impl.

tcharding · 2025-09-23T02:34:24Z

I'm not sure what we disagree about. If two decoders return the same error then there is currently no way when using the Decoder2 to tell which one errored. That's all I meant to say.

nyonson · 2025-09-23T04:21:20Z

Ah, my bad man, misinterpreted. We are on the same page.

nyonson · 2025-09-23T20:44:04Z

consensus_encoding/src/decode/mod.rs

+/// # Errors
+///
+/// Errors if `slice` is empty or encoding is invalid.
+pub fn decode_compact_size(slice: &mut &[u8]) -> Result<u64, CompactSizeDecodeError> {


Kix wrote a pretty slick compact size decoder back in the day here: https://github.com/rust-bitcoin/rust-bitcoin/pull/2184/files#diff-97b1ea49ecd9b6b576f8f6dbd640e6d317da31348297d14cfbbdf47b909ae34bR12

Even though this is only used internally by a vec decoder, I think it makes sense to make it a full fledged decoder to handle weird cases when only a handful of bytes are read for some reason.

Ah yeah, my version does not correctly handle passing in a byte slice that is only part of the compact size. Nice reviewing Nick!

nyonson · 2025-09-23T20:52:20Z

consensus_encoding/src/decode/decoders.rs

+///
+/// The encoding is expected to start with the number of encoded bytes (length prefix).
+#[cfg(feature = "alloc")]
+pub struct VecDecoder {


Should this just be VecBytesDecoder to differentiate from VecDecoder?

Those are the names I briefly thought of also so that is a reasonable metric for their merit. @apoelstra want to opine on these names?

VecBytesDecoder

VecDecoder<T> where T: Decodable

Perhaps ByteVecDecoder is better since an AmountDecoder decodes an Amount and the ByteVecDecoder decodes a byte vector?

Or does it decode a vector of bytes? - drats.

If you want me to make a decision I like ByteVecDecoder best. But I don't really care and don't think it matters. Almost nobody will ever name this type.

Lets call it two votes for ByteVecDecoder and one for VecBytesDecoder. ByteVecDecoder in is. Thanks

tcharding · 2025-09-25T04:41:09Z

The test failure (name decode_segwit_transaction in primitives::transaction) is interesting because its the same test copied from blockdata::transaction. I.e., with the original consenus_deserialize we happily decode compact ints bigger than 4,000,000 but in the new code introduced in this PR we don't.

nyonson

ACK 15b4f4c

Some minor comments, but I was able to wrap my head around the transaction state machine. I was hoping to think up some alternative which handled the Inputs/SegwitFlag dance and then passed it off to some simpler Decoder4/6s. But handling the "backtracking" kinda lead me back to where you landed.

nyonson · 2025-10-07T19:28:38Z

primitives/src/transaction.rs

+        match &self.state {
+            State::Version(decoder) => decoder.min_bytes_needed(),
+            State::Inputs(_, _, decoder) => decoder.min_bytes_needed(),
+            State::SegwitFlag(_) => 2,


In 15b4f4c, given how the state machine flows, I believe this should only be 1 since the marker has already been consumed and we are only analyzing the flag in this state.

BOOM! Good reviewing bro, you are a useful bloke to have on the team, for real.

nyonson · 2025-10-07T19:29:15Z

primitives/src/transaction.rs

+
+/// The state of the transiting decoder.
+#[cfg(feature = "alloc")]
+pub enum TransactionDecoderState {


In 15b4f4c, do we have to expose TransactionDecoderState/Attempt/IsSegwit? Would be nice to give us the flexibility to tweak them in the future if we can hide them.

Oh BOOM! I thought I tried that before, that's nice!

nyonson · 2025-10-07T19:31:16Z

primitives/src/transaction.rs

+/// Boolean used to track whether or not this transaction uses segwit encoding.
+#[cfg(feature = "alloc")]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum IsSegwit {


In 15b4f4c, another minor clarity idea for the state machine is to fold Attempt into IsSegwit by adding some sort of initial Unknown variant. Might make the Inputs/SegwitFlag dance a little clearer.

So with all these types private, your idea (which I'm not exactly understanding right now) becomes an internal change, right? So I can punt it down the field?

Yep, can punt.

And said another way, instead of having the Attempt enum for the Inputs state, just use the IsSegwit state.

enum IsSegwit { /// Yes so uses segwit encoding. Yes, /// No segwit flag, marker, or witnesses. No, /// Initial state, inputs have not been decoded yet. Unknown, }

This isn't a massive change, just maps well in my mind to how there are really 3 Segwit states when decoding.

Ah yeah that looks good.

tcharding · 2025-10-07T22:17:09Z

Changes in force push:

Made auxiliary types private
Moved auxiliary types below where they are used
Rebase no master
Fixed bug in min_bytes_needed - props @nyonson

nyonson

ACK 24e7b35

6f78cf7 Use correct name in rustdocs (Tobin C. Harding) Pull request description: Cut'n'pasta error slipped in, notice during review of #4998. ACKs for top commit: apoelstra: ACK 6f78cf7; successfully ran local tests Tree-SHA512: a5b3a5d3f7a724fff51973c73cb89b6f42095950ab076422c524d00029d1b0ef4c9fe71fb4f7c0aaf4abfaac963a296a3cb01a1df25573ba857f8e25fe38269d

a4be941 Feature gate unused const (Jamil Lambert, PhD) 5e2eeb8 Fix clippy error: struct is never constructed (Jamil Lambert, PhD) fdce327 Fix explicit lifetime clippy lints (Jamil Lambert, PhD) f38221c Fix lint error: passing a unit value to a function (Jamil Lambert, PhD) Pull request description: In preparation for adding crate specific clippy runs in CI with no-default-features, fix all the current lint errors when running it locally in each crate. There is one clippy error locally in primitives from an unused import (only used with alloc), but it is fixed in #4998. ACKs for top commit: apoelstra: ACK a4be941; successfully ran local tests tcharding: ACK a4be941 Tree-SHA512: e5f601213782e7ccceecfbdd022020f830dc321d6e31d522a1b93cf56c15735bccb72f3987d8fc6f633dba7ec8b88369aa65429992d97a3c15883c4622c5fd90

apoelstra · 2025-10-10T11:47:59Z

Will need rebase because #5107 renamed min_bytes_needed.

apoelstra · 2025-10-11T13:36:03Z

Needs rebase again since #5108 (which rearranged some imports).

Stupid that we still need to manually resolve these kinds of conflicts..

tcharding · 2025-10-11T23:08:18Z

~~Rebased and added a trivial cleanup patch up front even though its unrelated, it showed up when I did a search-and-replace while rebasing this PR.~~` Done properly in #5119

This terminology is not that widely used, lets use `scriptPubkey` instead as is more common.

apoelstra · 2025-10-12T16:17:50Z

Followups:

See if we can't improve the transaction state machine to be less panicky and indirect and full of "copy this to get mutable access" patterns (I spent a bit of time on this but don't have a complete solution)
See if we can avoid the From impls on the errors (extra API surface and prevents us from doing DecoderN where multiple of the sub-decoders have the same error type) (annoyingly we need to resolve this pre-1.0 but I think we can at least cut a rc with it)

Also lmao Kix would hate the OutpointDecoder which currently reads into a 36-byte array then copies the bytes out. We should be decoding directly into the right places, and using uninitialized memory beforehand, and so on. But whatever, our API is closed enough that we can fix this later.

apoelstra · 2025-10-12T16:31:55Z

The impl of Decoder::end for Transaction is also wrong, but I'm willing to defer this to a followup. The docs say that this returns an error if you end too early but the implementation panics instead.

apoelstra

ACK 39bf86b; successfully ran local tests

tcharding · 2025-10-12T18:44:45Z

Nice! thanks. I split up your comments into a bunch of bite sized issues.

apoelstra · 2025-10-13T13:36:07Z

@nyonson in the interest of moving forward I'm gonna merge this, and any future review you do we can address in followups.

27fbdf9 primitives: add unit test for incomplete transaction decoding (Andrew Poelstra) 93db1e0 primitives: fix TransactionDecoder::end to not panic on early calls to end (Andrew Poelstra) e86a56c primitives: renamed TransactionDecoderError::Transitioning to Errored (Andrew Poelstra) 681af66 primitives: remove a bunch of panics from Transaction::decoder (Andrew Poelstra) Pull request description: Refactors the `TransactionDecoder` state machine in a couple ways, eliminating some unreachable panic paths and also eliminating most of the reachable panic paths in `end()` (which should have been error returns). Fixes two of the four followup issues to #4998 -- the other two are optimizing `OutpointDecoder` which is non-essential/low-priority, and eliminating the `From` impls required by the composite `Decoder` types (which is a somewhat-hard API problem and I don't think we want to hold up the rc on it). Fixes #5125 Fixes #5122 ACKs for top commit: nyonson: ACK 27fbdf9 tcharding: ACK 27fbdf9 Tree-SHA512: 1fa7556cc8f47f578ff1293a09c76b07ae2a2de53a76eb0b52b0846ca73b203878791fd8bad350590aa9197ec173c007857059f2991515c9224d35686094f1b3

github-actions bot added C-units PRs modifying the units crate test C-primitives C-consensus_encoding labels Sep 17, 2025

tcharding force-pushed the push-nprqntyknkup branch 2 times, most recently from f5cc627 to b2a6a6b Compare September 22, 2025 04:16

github-actions bot added the C-p2p label Sep 22, 2025

tcharding changed the title ~~primitives: Start implementing decoding traits~~ WIP: primitives: Start implementing decoding traits Sep 22, 2025

tcharding force-pushed the push-nprqntyknkup branch 2 times, most recently from 0fcd759 to 838adc0 Compare September 23, 2025 03:55

tcharding force-pushed the push-nprqntyknkup branch 2 times, most recently from f01fb47 to a2bfedf Compare September 23, 2025 04:45

tcharding changed the title ~~WIP: primitives: Start implementing decoding traits~~ primitives: Implement decoding traits Sep 23, 2025

tcharding force-pushed the push-nprqntyknkup branch from a2bfedf to 4331cc6 Compare September 23, 2025 04:48

nyonson reviewed Sep 23, 2025

View reviewed changes

tcharding mentioned this pull request Sep 23, 2025

Do we want to hide ecoder error internals #5022

Open

tcharding force-pushed the push-nprqntyknkup branch from 4331cc6 to 363d3ff Compare September 25, 2025 04:29

github-actions bot removed test C-p2p labels Sep 25, 2025

tcharding force-pushed the push-nprqntyknkup branch 3 times, most recently from 5b6fab8 to 64608f2 Compare September 25, 2025 04:39

nyonson approved these changes Oct 7, 2025

View reviewed changes

tcharding force-pushed the push-nprqntyknkup branch from 15b4f4c to 24e7b35 Compare October 7, 2025 22:15

nyonson approved these changes Oct 8, 2025

View reviewed changes

This was referenced Oct 8, 2025

Fix clippy lints with no-default-features #5105

Merged

Update rust-bitcoin-maintainer-tools version and add extra_lints.sh #5106

Merged

tcharding force-pushed the push-nprqntyknkup branch 2 times, most recently from 3400588 to 0e97331 Compare October 10, 2025 17:15

tcharding force-pushed the push-nprqntyknkup branch from 0e97331 to 28b7509 Compare October 11, 2025 23:07

tcharding added 2 commits October 12, 2025 10:11

Use scriptPubkey instead of script public key

63af008

This terminology is not that widely used, lets use `scriptPubkey` instead as is more common.

primitives: Implement decoders

39bf86b

tcharding force-pushed the push-nprqntyknkup branch from 28b7509 to 39bf86b Compare October 11, 2025 23:11

apoelstra approved these changes Oct 12, 2025

View reviewed changes

apoelstra merged commit 99544ff into rust-bitcoin:master Oct 13, 2025
25 checks passed

apoelstra mentioned this pull request Oct 13, 2025

Followups to #4998 (fix TransactionDecoder) #5129

Merged

apoelstra mentioned this pull request Oct 31, 2025

Delegate witness decoder #5224

Closed

Conversation

tcharding commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 17, 2025

Uh oh!

nyonson commented Sep 22, 2025

Uh oh!

tcharding commented Sep 22, 2025

Uh oh!

nyonson commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 23, 2025

Uh oh!

nyonson commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nyonson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nyonson Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 7, 2025

Uh oh!

nyonson left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 10, 2025

Uh oh!

apoelstra commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Oct 12, 2025

Uh oh!

apoelstra commented Oct 12, 2025

Uh oh!

apoelstra left a comment

tcharding commented Sep 17, 2025 •

edited

Loading

nyonson commented Sep 22, 2025 •

edited

Loading

tcharding Sep 23, 2025 •

edited

Loading

tcharding Sep 23, 2025 •

edited

Loading

tcharding commented Sep 25, 2025 •

edited

Loading

nyonson left a comment •

edited

Loading

nyonson Oct 7, 2025 •

edited

Loading

apoelstra commented Oct 11, 2025 •

edited

Loading

tcharding commented Oct 11, 2025 •

edited

Loading

tcharding commented Oct 12, 2025 •

edited

Loading