Introduce bitcoin-consensus-encoding 1.0 and implement traits for Transaction and subtypes#288
Draft
apoelstra wants to merge 14 commits into
Draft
Introduce bitcoin-consensus-encoding 1.0 and implement traits for Transaction and subtypes#288apoelstra wants to merge 14 commits into
bitcoin-consensus-encoding 1.0 and implement traits for Transaction and subtypes#288apoelstra wants to merge 14 commits into
Conversation
Member
Author
|
Also, I renamed a few files, which is contributing to the huge diffstat. It's a big PR but it's not as big as it looks :). |
tcharding
reviewed
Jul 16, 2026
| /// Ok(TxOut { asset, value, nonce, script_pubkey, witness: TxOutWitness::empty() }) | ||
| /// } | ||
| /// } | ||
| /// } |
There was a problem hiding this comment.
Since you went to the trouble of writing such extensive docs is it worth adding dummy Display and Error impls here to remind folk?
tcharding
reviewed
Jul 16, 2026
Comment on lines
+8
to
+9
| // lol should these be accessible from a more convenient path? | ||
| use bitcoin::blockdata::block::BlockHashDecoder as GenesisHashDecoder; |
There was a problem hiding this comment.
You could drop the blockdata. Do you still think the path is too nested?
Member
Author
There was a problem hiding this comment.
Without blockdata I think it's okay. Really I'd like to write bitcoin::BlockHashDecoder but I'm unsure if that's a sane thing to do (reexporting every single encoder, decoder and decodererror at the root). It's what I did here in rust-elements and we can see how badly it pollutes the docs.
|
FWIW this looks sane to me. I just tabbed through quickly to see how you did it. |
Member
|
Looks good, will review ASAP. In the meantime a few small CI issues |
The 0.32.100 series has a higher MSRV (but still lower than ours) and introduces the bitcoin-consensus-encoding crate. Using this, we can eliminate all the bitcoin-io junk from this crate, eliminate our own Encoder trait, eliminate its passthrough impls on bitcoin types, etc. Since bitcoin depends on `encoding 1.0` but we will need 1.1, also add a separate explict encoding dependency. By updating bitcoin and hashes together, we can eliminate the duplicate `bitcoin-internals` dependency; everything is on 0.6 now.
This macro provided very little savings in lines of code, and wasn't worth the obfuscation.
This commit introduces the `decoder_state_machine` macro which generates (a lot of) the boilerplate needed to implement the encoding::Decoder trait. This includes internal enums, wrapper types to hide the internal enums, the state machine accounting, impls of std::error::Error, etc etc. I got Claude 4 to write a detailed doccomment, from which I deleted a bunch of fluff. But all the code in the macro was hand-written based on my implementing this decoder state machine a dozen times across the codebase. (My original implementations are thrown away and not included in this PR.)
Introduces the decoder_newtype! macro. As with the other macro, this one was hand-written based on multiple examples of hand-written code, but I let Claude write the doccomment (which I made some minor touchups to). There is a similarly-named macro in rust-bitcoin `primitives` but this is *not* that macro. A later PR will elaborate this macro, but this is all we need for now.
Leave the impl for BlockHeader (which is used by the functionary in a couple of places) (though I'd also like to delete this), the impls for the hash types (which are manually implemented and have regression tests), and the confidential stuff (which are also manually implemented and fairly straightforward). But for Transaction and PSET these impls are untenable. They are: * based on serde-derives (though 'manually' through the serde_struct_impl macro) of extremely complicated and fragile types, exposing implementation details and probably bypassing invariants on deserialization * not specified or documented anywhere * have very poor error messages on deserialization * not tested anywhere (not a single test broke with this change!!) If people are depending on these, we should provide some sort of compat crate/module. We can wait until they are trying to upgrade rust-elements and file complaints to do this. I'm skeptical that anybody *is*, given the above problems, at least not on purpose. If people want serde impls but don't need backward compatibility, we can provide that via the bitcoin-consensus-encoding crate, which will have general-purpose "serde-encode consensus objects in hex/bytes" adaptors in an upcoming version.
Just copy witness.rs from rust-bitcoin aeadac41b45a170beddbdbea8ada3d1621f09e7c, remove the alloc/std gates, and remove the LowerHex/UpperHex impls for which we need the private `HexPrimitive` type from bitcoin-primitives. Also fix the witness encoder to be exact-sized; see rust-bitcoin/rust-bitcoin#6426
apoelstra
force-pushed
the
2026-07/tx-encode-decode
branch
from
4b92b8e to
c45cfc5
Compare
TBH I'm not sure how best to review this commit. I thought it was going to be easy but then it wound up being a ton of boilerplate. I did -not- use the decoder_newtype macro because I made this complicated error mapping function. Maybe I should drop that here, or maybe I should integrate it into the macro somehow. I don't know. There is some discussion on rust-bitcoin. See rust-bitcoin/rust-bitcoin#6435
apoelstra
force-pushed
the
2026-07/tx-encode-decode
branch
from
c45cfc5 to
95b271f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a big PR. Please forgive me.
The goal is to replace the
EncodableandDecodabletraits from rust-elements with the newEncodeandDecodetraits from bitcoin-consensus-encoding. There are many benefits to this:bitcoin-consensus-encodingships with functions for decoding/encoding tostd::iotypes, unbufferedstd::iotypes, Vecs/slices, and hex strings, whilebitcoin-hashesships with functions for encoding into hash engines, andbitcoin-iowith functions for decoding/encoding intobitcoin::iotypes.core2(deleted from crates.io) orbitcoin-io, whose types tend to pollute every codebase that touches them.Transactionwithout needing to copy any data into auxliary buffers, and some for decoding. If an object does not itself hold aBoxorVecit should be possible to encode/decode it completely allocator-free. Again, without shim crates likeArrayVecor whatever. Coupled with allocator-free encoders likerust-bech32, it is now possible to write arbitrary error correcting codes and encode bitcoin/elements types into them without needing any auxiliary buffersSequenceandLockTimewithbitcoin-units 1.0types; this may invite type confusion between "bitcoin locktimes" and "elements locktimes" but in exchange we get stable types that have had years of bugfixes and API improvements. (And locktimes in Bitcoin, and therefore Elements, are notoriously fickle and hand to use correctly. Every API improvement we shipped found bugs in rust-miniscript.)