Skip to content

backport: consensus: Add CompactSize range check to deserialization#5921

Merged
apoelstra merged 3 commits into
rust-bitcoin:0.32.xfrom
tcharding:push-yxpsksqvkzss
Apr 17, 2026
Merged

backport: consensus: Add CompactSize range check to deserialization#5921
apoelstra merged 3 commits into
rust-bitcoin:0.32.xfrom
tcharding:push-yxpsksqvkzss

Conversation

@tcharding

Copy link
Copy Markdown
Member

Manually backport #5697. On master the VarInt type is gone but the deserialization bug fixed by 5697 exists here none the less.

Add the same range check to Decodable for VarInt.

@nymius

nymius commented Mar 30, 2026

Copy link
Copy Markdown

I've noticed these changes were applied on master in #5697, I should have commented there first.

@apoelstra

apoelstra commented Apr 1, 2026

Copy link
Copy Markdown
Member

Wait -- will this break PSBT on 0.32.x?

Otherwise utack 88722b0

@tcharding

tcharding commented Apr 1, 2026

Copy link
Copy Markdown
Member Author

PSBT on 0.32.x is borked anyways if anyone uses a key type_value that is more than one byte. We could backport all the bug fixes around type_value but really I rekon we just leave it, its been like this for years and no one has noticed, right?

@tcharding

Copy link
Copy Markdown
Member Author

@apoelstra I update to latest nightly and we get a single lint warning but this code is the same on master. Can you debug this for me please?

Using

at nightly-version                                                                                                                                                   ✘ 101 
nightly-2026-03-27
error: value assigned to `w` is never read
   --> hashes/src/sha256.rs:235:13
    |
235 |             $w = updates.2;
    |             ^^^^^^^^^^^^^^
...
803 |         round!(b, c, d, e, f, g, h, a, 0xc67178f2, w[15], w[13], w[8], w[0]);
    |         -------------------------------------------------------------------- in this macro invocation
    |
    = help: maybe it is overwritten before being read?
    = note: `-D unused-assignments` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_assignments)]`
    = note: this error originates in the macro `round` (in Nightly builds, run with -Z macro-backtrace for more info)

error: value assigned to `w` is never read
   --> hashes/src/sha512.rs:167:13
    |
167 |             $w = small_hash::later_round($a, $b, $c, &mut $d, $e, $f, $g, &mut $h, $k, $w, $w1, $w2, $w3)
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
293 |         round!(b, c, d, e, f, g, h, a, 0x6c44198c4a475817, w[15], w[13], w[8], w[0]);
    |         ---------------------------------------------------------------------------- in this macro invocation
    |
    = help: maybe it is overwritten before being read?
    = note: this error originates in the macro `round` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `bitcoin_hashes` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `bitcoin_hashes` (lib test) due to 2 previous errors
error: Recipe `lint` failed on line 14 with exit code 101

@apoelstra

Copy link
Copy Markdown
Member

@tcharding we have a macro round! that we call over and over, which is supposed to compute a value that gets fed into the next round. Well, the final round doesn't feed into anything, so Rust interprets this as an "unused assignment".

We should just whitelist the lint on any blocks that are affected.

@tcharding

Copy link
Copy Markdown
Member Author

Thanks. What confused me was why the lint fires here but not on master? I looked again and I'm guessing, but its not clear, and probably not important, because we use cargo rbmt on master with it's lint configuration and we do it differently on this branch.

I wrote up: #5957

@apoelstra

Copy link
Copy Markdown
Member

fires here but not on master

I'm pretty sure we whitelisted it

@apoelstra

Copy link
Copy Markdown
Member

in #5376

@tcharding tcharding force-pushed the push-yxpsksqvkzss branch from 88722b0 to 8a8498f Compare April 8, 2026 04:18
@github-actions github-actions Bot added the C-hashes PRs modifying the hashes crate label Apr 8, 2026
@tcharding

tcharding commented Apr 8, 2026

Copy link
Copy Markdown
Member Author

Nice digging. I added the patch from #5376 to this. I just cherry-picked it, the commit log is a bit odd now. You ok with that?

@tcharding tcharding force-pushed the push-yxpsksqvkzss branch from 8a8498f to e760a5d Compare April 8, 2026 04:23
@apoelstra

Copy link
Copy Markdown
Member

@tcharding can you put a patch at the front of this which updates the nightly version?

@tcharding tcharding force-pushed the push-yxpsksqvkzss branch from 80f688b to 829981b Compare April 9, 2026 06:16
@tcharding

Copy link
Copy Markdown
Member Author

Squashed all the lint fixes into the 'update nightly toolchain' patch. I forget, do you run the linter on each patch?

@tcharding tcharding force-pushed the push-yxpsksqvkzss branch from 829981b to 5c76a41 Compare April 9, 2026 06:24
@github-actions github-actions Bot added C-units PRs modifying the units crate C-internals PRs modifying the internals crate C-io PRs modifying the io crate C-base58 PRs modifying the base58 crate labels Apr 9, 2026
@apoelstra

Copy link
Copy Markdown
Member

I forget, do you run the linter on each patch?

I do not -- but I do attempt to use the nightly compiler on each patch. So I cannot test this PR unless the first commit fixes the nightly-version file.

Copy the version currently on master and fix the introduced lint
warnings.
Manually backport rust-bitcoin#5697. On `master` the `VarInt` type is gone but the
deserialization bug fixed by 5697 exists here none the less.

Add the same range check to `Decodable for VarInt`.
Like a cockroach that will just never fucking die ...
@tcharding

Copy link
Copy Markdown
Member Author

Sweet, re-ordered the patches. No other changes.

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4a71193; successfully ran local tests

@apoelstra apoelstra merged commit 9d5cae9 into rust-bitcoin:0.32.x Apr 17, 2026
17 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-base58 PRs modifying the base58 crate C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-internals PRs modifying the internals crate C-io PRs modifying the io crate C-units PRs modifying the units crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants