refactor confidential.rs and clean up code#286
Open
apoelstra wants to merge 12 commits into
Open
Conversation
Copy the (nightly-only) rustfmt.toml from rust-bitcoin, and add an ignore
block which scopes it specifically to src/lib.rs. As I add new files, I
will add them to the format whitelist.
Because I am going to be adding a ton of new exports to src/lib.rs, add
that file for now.
To make this work with jj-fix, run
jj config edit --repo
and add
[fix.tools.cargo-fmt]
command = ["/usr/bin/env", "rustfmt", "+nightly", "--edition", "2018"]
patterns = [
"./src/lib.rs",
"./src/confidential/range_proof.rs",
"./src/confidential/surjection_proof.rs",
"./src/transaction/decoders.rs",
"./src/transaction/encoders.rs",
"./src/transaction/pegin_witness.rs",
]
…licKey These impls shouldn't really be in the public API; we don't ever consensus encode bare secp256k1-zkp objects. We only consensus-encode values and assets and nonces, which have their own dedicated types. Also, as a practical problem, we are going to remove our Encodable/Decodable trait in favor of the bitcoin-consensus-encoding Encode/Decode traits. Since we own neither the secp256k1-zkp objects nor the traits, we won't be able to retain these.
This commit (and the next) are a bit bigger than I'd expected. Essentially,
throughout the codebase we use Option<Box<secp256k1_zkp::RangeProof>> to
represent rangerproofs, and similar for surjection proofs (done in the next
commit). Aside from being very noisy, this causes a multitude of problems:
* we needed the aux traits `BlindValueProofs` and `BlindAssetProofs` to
add exact-value methods
* there was a type confusion throughout the PSET code where we did not
distinguish between empty rangeproofs and unset rangeproofs; we were
using None for both
* poor encapsulation in general; not only using literal None to mean "empty
rangeproof" but mucking around with Option::as_ref and boxing/unboxing
makes the code hard to read
* we had to impl our Encodable/Decodable traits directly on the secp-zkp
types, which we will be unable to do once we move to the new traits
This commit replaces the Option<Box<secp256k1_zkp::RangeProof>> with a new
rust-elements RangeProof type. This fixes all the above issues.
Also, FYI for new files I am quietly relicensing from CC0 to MIT+Apache. I am
not doing this in this non-license-related PR to be sneaky; I was just creating
new files so it was an opportunity to use a new license header.
See rust-bitcoin/rust-bitcoin#5849 for motivation.
All the justification from the previous commit message apply here.
The code for value, nonce, asset, have diverged in various ways. By splitting them into multiple files I hope to make diffing them easier. Code move only; the next commits will make chonges to bring the three files closer together. Also I am quietly relicensing this stuff from CC0 to MIT+Apache. I am not doing this to be sneaky; I was just creating new files so it was an opportunity to use a new license header. See rust-bitcoin/rust-bitcoin#5849
This greatly reduces the diff between the different confidential commitments.
More work to make the different confidential types more uniform.
We eventually want to remove Encodable and Decodable. To help with that, start by cleaning up a few things: * don't use the traits on u8 or u64 * especially don't use the trait on u64 then call `swap_bytes` to deal with the fact that we want a BE encoding
apoelstra
force-pushed
the
2026-06/confidential
branch
from
83bce40 to
cf24680
Compare
Member
Author
|
On cf24680 successfully ran local tests |
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
1 participant
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 lot of commits but hopefully should be straightforward to review. Two are formatting, one is a rename-only, at least one is code-move-only, and a few others just enable lints. The big changes are:
RangeProofandSurjectionProofinto dedicated types, rather than usingOption<Box<secp256k1_zkp::RangeProof>>everywhere. This fixes a type confusion in the PSET code and overall greatly simplifies all our interactions with rangeproofs and surjection proofs.Encodable/Decodablefor secp256k1_zkp objects, since these should never appear "bare" in rust-elements code. Instead we implement these only on rust-elements wrapper types. We also simplify these implementations a bit to e.g. directly encode big-endian integers for confidential values rather than callingu64::consensus_encodethenu64::swap_bytes.The next PR will introduce the rust-bitcoin 0.32.100 series, which have new
Encode/Decodetraits with much better error handling, no morebitcoin-io, and fewer allocations. This PR helps set the stage for that by cleaning up the existingEncodable/Decodableimpls and improving the overall type safety of the library.