0.32.x: Add `consensus_encoding` to types present in units 1.0 by jamillambert · Pull Request #6184 · rust-bitcoin/rust-bitcoin

jamillambert · 2026-05-14T13:48:30Z

As a first step in implementing consensus_encoding in 0.32.x add impl for types that are in units 1.0 and in 0.32.x.

Add consensus_encoding dependency to units and bitcoin (some units 1.0 types are still in bitcoin in v0.32).
Add consensus_encoding to Amount, Sequence, CompactTarget and absolute::LockTime.
- These are a copy and paste from master with minimal changes required, described in the commit logs.
Bump the base58ck dependence in bitcoin to 0.1.100. Required to remove the dependence of 0.1.0 on internals.
Add fuzzing of the new vs old impl. The slices are trimmed to the length required by the decoder since the new encoding logic reads what it needs and ignores the rest, but the legacy fails if there are trailing bytes.

jamillambert · 2026-05-14T14:34:46Z

consensus_encoding depends on internals 0.5.0 but units still depends on 0.3.0.

I opened #6188 to update the dependence to 0.5.0 on 0.32.x

jamillambert · 2026-05-26T10:53:31Z

Problem with internals solved by #6200. Waiting on MSRV bump.

apoelstra · 2026-05-27T21:46:04Z

MSRV bump tagged and published in #6126

jamillambert · 2026-05-28T11:59:22Z

Now internals is gone and the MSRV bumped this is ready for review.

tcharding · 2026-05-29T00:00:41Z

+
+    if !remaining.is_empty() {
+        return Err(DecodeAllError::TrailingBytes);
+    }


Isnt' this going to error on every call? I thought bytes was essentially an endless slice of data.

Apparently not, I don't know why not though?

Oooh, maybe compare_amount never gets anywhere because both decode_all and deserialize error.

This was added because the first try failed fuzzing on cases with trailing bytes. As AI said (minus the half a page of nonsense) "the new path could accept a valid prefix with extra bytes while legacy rejects it". Fuzzing with extra bytes then causes false mismatches.

This is a behaviour difference, but I assume we want it because it matches master. And it is not a difference in the encoding itself.

I added a comment to try and explain why the check is there.

I thought bytes was essentially an endless slice of data.

Rust has no "endless slice" type so bytes is definitely not this.

Please cite which commit you're reviewing. Github does not show enough context to locate comments. For example, here you are complaining about a bytes object which does not even appear in the "context" that Github provides.

I changed the fuzzing to trim the slices since the old encoding logic failed on trailing bytes but the new just ignores them.

I learned a bit more. What I learned is: libfuzzer calls do_test repeatedly with different length slices starting with short ones then getting bigger.

I'm still unsure once the fuzzer finds a code path (ie a type decodes) if subsequent calls to do_test will change the slice length again or if the fuzzer gets 'stuck' using this length slice. Which leads back to the question is it the same to run the fuzzer for 1000 seconds compared to running it 10 times for 100 seconds?

tcharding · 2026-05-29T00:01:52Z

A few comments:

The Amount stuff looks spot on.
I don't think we should be doing the locktime types. It should be done only for LockTime in bitcoin.
I rekon this PR should be 'all types in units 1.0.0' not 'all types in units 0.1.3 (ie units/ here on this branch)
I rekon the include/ directory should go in the crate root not in units/.
Should we bike shed the feature name since it is going to be used in bitcoin as well? Is encoding what we want? (I think it is)
Perhaps we should add the feature to bitcoin in this PR and access the units types like bitcoin::Amount so the next PR (adding primitives types) is cleaner? Not sure though, up to you.
fuzz/generate-files.sh looks stale

jamillambert · 2026-05-29T15:50:56Z

A few comments:

The Amount stuff looks spot on.

It was relatively straightforward, I am hoping there are other types like this.

I don't think we should be doing the locktime types. It should be done only for LockTime in bitcoin.

I removed the changes to locktime.

I rekon this PR should be 'all types in units 1.0.0' not 'all types in units 0.1.3 (ie units/ here on this branch)

It is only Amount that has consensus_encoding in 1.0.0 and here.

I rekon the include/ directory should go in the crate root not in units/.

Master has it both at the repo root and in units. I assumed it was duplicated in units because it needed to be in the crate for publishing.

Should we bike shed the feature name since it is going to be used in bitcoin as well? Is encoding what we want? (I think it is)

Can easily change it if needed. I just matched it to master.

Perhaps we should add the feature to bitcoin in this PR and access the units types like bitcoin::Amount so the next PR (adding primitives types) is cleaner? Not sure though, up to you.

Makes sense to do it now so I did it.

fuzz/generate-files.sh looks stale

Updated.

apoelstra · 2026-05-29T18:58:15Z

Master has it both at the repo root and in units. I assumed it was duplicated in units because it needed to be in the crate for publishing.

There is only one copy, in the crate root. The other copies are symlinks.

I removed the changes to locktime.

I don't know how to interpret @tcharding's "don't do locktimes, do locktimes" comment, but we need to implement the traits for LockTime because this type appears in Transaction.

tcharding · 2026-05-29T21:57:31Z

I don't know how to interpret @tcharding's "don't do locktimes, do locktimes" comment

Sorry if I wasn't clear. This PR is (or was when I reviewed it) adding encoding to the types used by LockTime (what was Height and Time). Its those we don't want. We only want LockTime.

It is only Amount that has consensus_encoding in 1.0.0 and here.

That is my point. There are other types in units 1.0 that are in bitcoin on this branch i.e., at this stage of dev they hadn't been moved over to units. I think this PR should do all of them as well. I realized that #6259 should have the same reasoning applied to it. So, for example, this PR would include code in bitcoin::blockdata::locktime::absolute for the LockTime.

jamillambert · 2026-05-30T08:58:43Z

Ok thanks, I'll address those comments next week. Going to draft it for now.

tcharding · 2026-06-03T22:37:46Z

Full disclosure, I was braindead yesterday, I had it in mind that all the types from units all implemented Encode so wondered why they weren't here. FTR this PR is correct, I was wrong.

tcharding

ACK d16f97e

tcharding · 2026-06-03T22:38:39Z

BOOM! Thanks for your patience and for sticking with this Jamil.

tcharding · 2026-06-08T21:47:20Z

Can you prioritise this one please @apoelstra

apoelstra · 2026-06-09T21:15:58Z

FYI we may wind up yanking this, reducing the MSRV, and re-releasing. But I don't see any reason to hold up this PR on that. Will definitely prioritize this.

Bump all of the dependencies of the crates on other crates in the workspace to .100 which have an MSRV of 1.74 now that they are published.

Add the dependency and reexport to units and bitcoin. Add path section to bitcoin manifest since it uses local changes to units. Add the decoder newtype macro for consensus encoding and sym links in units and bitcoin. Update api and lock files.

Copy the code from master directly. Then remove ``.map_err(AmountDecoderError::out_of_range)` from `Amount::from_sat(a)`` since it is infallible. Remove AmountDecoderErrorInner and refactor now it is not needed. Update api files.

Copy the code from master directly. Then update the import of `internals` to `internal_macros` now that `internals` is not used. Gate the whole error module on `encoding` instead of every individual part to keep it out of the api when `encoding` is not enabled. Update api files.

jamillambert · 2026-06-11T09:39:52Z

Rebased and removed merge conflict in lock files.

Also removed all the error modules and inlined the errors.

Copy the code from master directly. Then update the import of `internals` to `internal_macros` now that `internals` is not used. Gate the whole error module on `encoding` instead of every individual part to keep it out of the api when `encoding` is not enabled. And update a doc link to LockTime. Update api files.

Add a fuzz target that compares the legacy to the new consensus encoding implementations for units 1.0 types. The slices created by the fuzzer are trimmed to the correct length to increase coverage instead of erroring on most slices that are too long. Update lockfiles. Assisted-by: Claude Sonnet 4.6 (So it says, with the amount of edits I had to make I'm not sure it was an "assistance")

tcharding

ACK 2e1f9a0

apoelstra · 2026-06-12T16:53:30Z

My local CI system is complaining about the include link. Lemme investigate.

apoelstra · 2026-06-12T18:10:42Z

I think I've got it. Should be resolved and an ACK posted tonight.

apoelstra

ACK 2e1f9a0; successfully ran local tests

tcharding · 2026-06-13T23:26:52Z

~~Is the plan merge this then add the transition-1.0 feature @apoelstra?~~

I think we should revert all the patches that went in with the MSRV bump first.

tcharding · 2026-06-13T23:47:08Z

MSRV bump revert in #6354

Then we can rebase this and see if we can get it to work based on #6349

apoelstra · 2026-06-14T12:42:54Z

This one is bigger, already has review, and doesn't have any version bumps or anything in it. So I merged it now, and we can rebase #6354.

tcharding · 2026-06-14T22:46:04Z

Bother, you made #6354 way harder now. I don't know exactly how to revert and keep each commit building and still keep the encoding stuff.

tcharding · 2026-06-14T22:48:24Z

EDIT: Perhaps skip reading this and just see #6359

I had hoped to do this:

revert back to a clean state (remove one by one going backwards all the msrv patches) (as in 0.32.x: Revert MSRV bump #6354)
add the transition feature and a dep on encoding and check that we have CI all working (ie prove cargor and cargo rbmt can handle the different MSRVs)
then rebase/merge this encoding stuff

github-actions Bot added ci C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate test doc labels May 14, 2026

jamillambert force-pushed the 0513-0.32.x-units-encoding branch 2 times, most recently from debebc4 to d5337ac Compare May 14, 2026 13:52

jamillambert force-pushed the 0513-0.32.x-units-encoding branch from d5337ac to 6493f3a Compare May 28, 2026 11:46

github-actions Bot removed ci C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate doc labels May 28, 2026

jamillambert marked this pull request as ready for review May 28, 2026 11:57

tcharding mentioned this pull request May 28, 2026

Remove SerializeBytesAsHex from internals #6273

Closed

tcharding reviewed May 28, 2026

View reviewed changes

Comment thread fuzz/Cargo.toml Outdated

tcharding reviewed May 29, 2026

View reviewed changes

jamillambert force-pushed the 0513-0.32.x-units-encoding branch 2 times, most recently from 7995ac4 to 6fe22fe Compare May 29, 2026 15:49

github-actions Bot added the C-bitcoin PRs modifying the bitcoin crate label May 29, 2026

jamillambert marked this pull request as draft May 30, 2026 08:58

tcharding approved these changes Jun 3, 2026

View reviewed changes

jamillambert mentioned this pull request Jun 8, 2026

0.32.x: Add consensus_encoding to types present in primitives 1.0 #6333

Merged

apoelstra mentioned this pull request Jun 10, 2026

Updating rust-bitcoin and supporting musig roadmap ElementsProject/rust-elements#277

Open

6 tasks

jamillambert added 5 commits June 11, 2026 10:38

Bump dependencies to .100

7b9851d

Bump all of the dependencies of the crates on other crates in the workspace to .100 which have an MSRV of 1.74 now that they are published.

Add dependency on consensus_encoding

d17cd34

Add the dependency and reexport to units and bitcoin. Add path section to bitcoin manifest since it uses local changes to units. Add the decoder newtype macro for consensus encoding and sym links in units and bitcoin. Update api and lock files.

Add consensus_encoding support to Amount

88f2c69

Copy the code from master directly. Then remove ``.map_err(AmountDecoderError::out_of_range)` from `Amount::from_sat(a)`` since it is infallible. Remove AmountDecoderErrorInner and refactor now it is not needed. Update api files.

jamillambert force-pushed the 0513-0.32.x-units-encoding branch from d16f97e to bfe8656 Compare June 11, 2026 09:38

jamillambert added 2 commits June 11, 2026 10:42

jamillambert force-pushed the 0513-0.32.x-units-encoding branch from bfe8656 to 2e1f9a0 Compare June 11, 2026 09:42

tcharding approved these changes Jun 12, 2026

View reviewed changes

apoelstra approved these changes Jun 13, 2026

View reviewed changes

apoelstra mentioned this pull request Jun 13, 2026

Return MSRV of 0.32.x to 1.63.0 and yank 0.32.100 #6349

Closed

4 tasks

apoelstra merged commit b481a4e into rust-bitcoin:0.32.x Jun 14, 2026
18 of 19 checks passed

This was referenced Jun 14, 2026

Revert all merge commits back to tag: bitcoin-0.32.10 #6359

Closed

Delete the old consensus encoding logic #5971

Closed

Uh oh!

Conversation

jamillambert commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented May 26, 2026

Uh oh!

apoelstra commented May 27, 2026

Uh oh!

jamillambert commented May 28, 2026

Uh oh!

Uh oh!

tcharding May 29, 2026 • 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!

tcharding May 29, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding May 29, 2026

Choose a reason for hiding this comment

Uh oh!

jamillambert May 29, 2026

Choose a reason for hiding this comment

Uh oh!

apoelstra Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

jamillambert Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented May 29, 2026

Uh oh!

apoelstra commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamillambert commented May 30, 2026

Uh oh!

tcharding commented Jun 3, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jun 3, 2026

Uh oh!

tcharding commented Jun 8, 2026

Uh oh!

apoelstra commented Jun 9, 2026

Uh oh!

jamillambert commented Jun 11, 2026

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jun 12, 2026

Uh oh!

apoelstra commented Jun 12, 2026

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jun 13, 2026

Uh oh!

Uh oh!

apoelstra commented Jun 14, 2026

jamillambert commented May 14, 2026 •

edited

Loading

jamillambert commented May 14, 2026 •

edited

Loading

tcharding May 29, 2026 •

edited

Loading

tcharding commented May 29, 2026 •

edited

Loading

apoelstra commented May 29, 2026 •

edited

Loading

tcharding commented May 29, 2026 •

edited

Loading

tcharding commented Jun 13, 2026 •

edited

Loading

tcharding commented Jun 14, 2026 •

edited

Loading