Skip to content

Improve slicing and error handling around slices#279

Merged
apoelstra merged 6 commits into
ElementsProject:masterfrom
apoelstra:2026-06/slicing-improvements
Jun 15, 2026
Merged

Improve slicing and error handling around slices#279
apoelstra merged 6 commits into
ElementsProject:masterfrom
apoelstra:2026-06/slicing-improvements

Conversation

@apoelstra

Copy link
Copy Markdown
Member

Uses the bitcoin-internals library which provides us a bunch of array/slice methods that aren't available in std in our MSRV. Eventually we will be able to drop the dep and get these features directly from std.

This replaces a bunch of slice accesses with arrays, cleaning up error paths and eliminating panics (though most are inaccessible assuming you use known-valid data from the blockchain).

Builds on #278 (which I will merge in the next hour).

Will do a followup that cleans up the error types; I deliberately avoided breaking the API to make this one easy to backport.

@apoelstra apoelstra force-pushed the 2026-06/slicing-improvements branch 6 times, most recently from 529130d to e97b79a Compare June 15, 2026 18:57
I would like the ArrayExt trait, which has a bunch of unsafe code, and
which implements stuff that is missing in stdlib. This is already in
our dep tree through rust-bitcoin so adding a direct dependency adds
no new code exposure.
These unwraps and indexing (which hide more panic paths) irritated me, and also
would need to be tweaked once we get rid of the Hash::from_slice methods. I
figured I'd preemptively get rid of them.
I don't *think* it's possible to create a rangeproof with a sidechannel smaller
than 64 bytes (if you create a 0-sized "proof of exact value" then unwinding
will fail entirely, and anything larger I think has at least one ring, so 128
bytes or more). Unsure.

But better not to assume this by indexing recklessly into the sidechannel
message.
We should redo the error types here as well at some point.
Currently we allow decoding segwit v0 programs which have uncompressed/hybrid
keys (not allowed) and I suspect that if you provide a too-short address then
you'll get a panic here.
Lol, this code was so close and yet so far.
@apoelstra apoelstra force-pushed the 2026-06/slicing-improvements branch from e97b79a to 5e0d577 Compare June 15, 2026 19:00
@philipr-za

philipr-za commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

ACK 5e0d577

The type inference that auto-magically figures out the sizes for split_array is neat!

@apoelstra

Copy link
Copy Markdown
Member Author

Yeah, these are great APIs and frustratingly they have been unstable for a long time and still not in stable https://doc.rust-lang.org/std/primitive.array.html#method.split_array_ref

@apoelstra

Copy link
Copy Markdown
Member Author

ACK 5e0d577; successfully ran local tests

@apoelstra apoelstra merged commit 9304937 into ElementsProject:master Jun 15, 2026
10 checks passed
apoelstra added a commit that referenced this pull request Jun 16, 2026
982a50f Disable fuzztest in CI (Philip Robinson)
ad00563 Bump version to 0.25.3 (Philip Robinson)
6275989 Fix docstring in blech32 (Philip Robinson)
675ca26 Pin deps for Fuzztest job (Philip Robinson)
ea42e11 add `--no-deps` to doc test (Philip Robinson)
70276cc pin bitcoin crates in test.sh (Philip Robinson)
2941a39 Remove `-- -D warnings` from test.sh (Philip Robinson)
250c4f6 pset: fix slicing in KeySource::deserialize (Philip Robinson)
b06dac9 address: do a better job slicing bech32 data (Philip Robinson)
2a8b46f transaction: use better error typing for pegin destructuring (Philip Robinson)
8b15649 blind: use array splitting in TxOut::unblind (fix potential DoS?) (Philip Robinson)
179c6da rewrite Address::from_base58 to eliminate all the unwraps (Philip Robinson)
8cd2a4d Add arrays.rs and slices.rs from bitcoin-internals 0.5 (Philip Robinson)

Pull request description:

  There are a number of useful improvements from #279 we would like to include in 0.25.x.

ACKs for top commit:
  apoelstra:
     utACK 982a50f

Tree-SHA512: 51f786c01982d107193f9e75be13a1087a24372664eb70fbbb0eb3ee3d2bfbf64b94ac573a22fe20d412c717f54d5c3a37a4010099fe6d56892fdd486e026c62
apoelstra added a commit that referenced this pull request Jun 16, 2026
6fee387 Bump version to 0.26.2 (Philip Robinson)
4428361 pset: fix slicing in KeySource::deserialize (Philip Robinson)
6e082a9 address: do a better job slicing bech32 data (Philip Robinson)
acf5bc8 transaction: use better error typing for pegin destructuring (Philip Robinson)
6659691 blind: use array splitting in TxOut::unblind (fix potential DoS?) (Philip Robinson)
6005720 rewrite Address::from_base58 to eliminate all the unwraps (Andrew Poelstra)
d216a7c Add arrays.rs and slices.rs from bitcoin-internals 0.5 (Philip Robinson)

Pull request description:

  There are a number of useful improvements from #279 we would like to include in 0.26.x.

ACKs for top commit:
  apoelstra:
    ACK 6fee387; successfully ran local tests

Tree-SHA512: dd5d142dd76cbe472c67823989f79ed1bb1024282b788cd840bd4cf71db95ef277aa4eeb416ebe829a01f4dd22ed672dbc293e181bddaa9ccb0814e47dd1cc19
@apoelstra apoelstra deleted the 2026-06/slicing-improvements branch June 16, 2026 19:57
apoelstra added a commit to apoelstra/rust-elements that referenced this pull request Jun 19, 2026
In ElementsProject#279 we fixed some slicing bugs in a fairly ugly way so that we could
easily backport the bugfixes without API breakage. However, the better
solution is to close the RangeProofMessage type, add a dedicated error
for parsing from an array, and update all callers.

This changes the CT validation logic to allow rangeproofs to contain
embedded asset IDs alongside explicit assets. This differs from Elements
which only allows unblinding an output if both its value and asset
commitment are confidential. However, if the confidential asset is
explicit, it seems like the "right thing to do" to embed the asset ID
and a zero blinding factor. This matches what we do in this library in
TxIn::blind_issuances_with_bfs for example.
apoelstra added a commit to apoelstra/rust-elements that referenced this pull request Jun 29, 2026
In ElementsProject#279 we fixed some slicing bugs in a fairly ugly way so that we could
easily backport the bugfixes without API breakage. However, the better
solution is to close the RangeProofMessage type, add a dedicated error
for parsing from an array, and update all callers.

This changes the CT validation logic to allow rangeproofs to contain
embedded asset IDs alongside explicit assets. This differs from Elements
which only allows unblinding an output if both its value and asset
commitment are confidential. However, if the confidential asset is
explicit, it seems like the "right thing to do" to embed the asset ID
and a zero blinding factor. This matches what we do in this library in
TxIn::blind_issuances_with_bfs for example.
delta1 added a commit that referenced this pull request Jun 29, 2026
9f47b74 use bitcoin_hashes 1.0 throughout the codebase (Andrew Poelstra)
be0c59d genesis: eliminate a bunch of allocations and panic paths (Andrew Poelstra)
400650d add 'hashes 1.0' dependency (Andrew Poelstra)
3c958e2 blind: encapsulate RangeProofMessage and improve error types (Andrew Poelstra)

Pull request description:

  As part of our project to modernize this library and its dependencies, upgrade to bitcoin_hashes 1.0. This has a number of API improvements:

  * replacing all the `Hash` trait methods with inherent methods so you don't need to import a trait anymore
  * replacing the fallible `from_slice` methods with infallible `from_byte_array` methods
  * removing the `hash` method and other "compute a hash from arbitrary data" methods on wrapper types like `Txid`; these should only be constructed in specific prescribed ways

  Also includes a followup commit to #279 to improve error typing.

ACKs for top commit:
  delta1:
    ACK 9f47b74; tested locally

Tree-SHA512: 5445b23158aa8379e22c8d43d922e8588ed5dd6a9a97c4bb39aeb6b37febc615407f8cb654d86c8f28959aebfb3e1611fc75b8900b700a9b7c4474e4fffaad7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants