df1d2f6eb5 Add unchecked variants to Amount and SignedAmount (yancy)
Pull request description:
The checked variants have worse performance than the unchecked variants due to the additional branching operations. To improve performance where overflow is either not possible or not a concern, unchecked variants of `Amount` and `SignedAmount` are introduced for addition, subtraction and multiplication.
Note, it seems the default behavior for the test framework is to panic on overflow, so I haven't figured out a good way to add tests for this. Marking as a draft for now.
closes: https://github.com/rust-bitcoin/rust-bitcoin/issues/2434
ACKs for top commit:
Kixunil:
ACK df1d2f6eb5
apoelstra:
ACK df1d2f6eb5 gonna go ahead and merge this, we can revisit if necessary when we look at `units` overflow behavior in general
Tree-SHA512: 3fbb0ec81a758b350569226c44e25f6ca49e551566bee83c05c1c2b343874ef657d63a36b5f51c41582d8a8e36466275c574ebff6d363ed7c112ac8b4d5376fa
The checked variants have worse performance than the unchecked variants
due to the additional branching operations. To improve performance where
overflow is either not possible or not a concern, unchecked variants
of Amount and SignedAmount are introduced.
1384330029 taproot: add TapNodeHash getter method on TapTree and NodeInfo (conduition)
Pull request description:
Submitting this to fix what I think is an API hole. Please correct me if I'm mistaken here and there is an easier way to do what I'm after.
## Problem
From what I can tell of the existing 0.31.1 API, there doesn't seem to be any way for a consumer to build a taproot tree using `TaprootBuilder` and then simply output the resulting tap tree merkle root `TapNodeHash`.
Instead, the API forces me to do `TaprootBuilder::finalize(secp_ctx, internal_key)` first to get a `TaprootSpendInfo`, and then call `TaprootSpendInfo::merkle_root()` to get the root `TapNodehash`. This requires ECC point addition/multiplication for the tweak operation (inside `TaprootBuilder::finalize`), so it is a lot less performant than the simple hashing operations needed to build a taproot tree.
Obviously if I want to spend the taproot tree, I'll need to tweak an internal key. But if all I want is to examine the merkle root hashes of taproot trees (e.g. for quick validation), there should be a faster and more direct option for me.
## Suggested Solution
My suggestion, demonstrated in this PR, would be to add a couple of simple getter methods:
- `TapTree::node_hash() -> TapNodeHash`
- `NodeInfo::node_hash() -> TapNodeHash`
These rhyme with the existing `LeafNode::node_hash()` method. These provide a roundabout way for downstream consumers to extract a taptree merkle root `TapNodeHash` while still using the safe API provided by `TaprootBuilder`. I would simply use `TaprootBuilder` to build a `TapTree` or `NodeInfo`, and then invoke the `node_hash` method on that object. No point addition required.
## Footguns
This does open up more opportunities for consumers to footgun themselves by, for example, committing a P2TR script pubkey to a leaf node by accident, instead of the root node. I'd argue this possibility already exists in the form of `LeafNode::node_hash()`. We're not making that problem much worse here.
If the caller is using `TaprootBuilder` to construct their tree, the only way they'll be able to get a `NodeInfo` or `TapTree` in the first place would be to finalize the builder into it, which seems like an acceptable and intuitive-enough usage path to me.
ACKs for top commit:
apoelstra:
ACK 1384330029
tcharding:
ACK 1384330029
Tree-SHA512: 9a3c7313fcf281e6439241d0cdc9ea018329ecb5e8b959cba88b0e36d4f1f0df54e09e5318073e1067618e85aea991d5ac7c50fa336a91782896c9cb3c98a973
Fixes a gap in the API of the taproot module. Callers can now use
TapTree::root_hash or NodeInfo::node_hash to extract the taproot
tree merkle root hash for fast validation without any ECC overhead.
251579f4ef feat: implement TryFrom trait to SignedAmount and Amount (Sumit Kumar)
Pull request description:
Closes: #2245
Adds `TryFrom<SignedAmount> for Amount` and `TryFrom<Amount> for Amount` in units module
ACKs for top commit:
tcharding:
ACK 251579f4ef
Kixunil:
ACK 251579f4ef
apoelstra:
ACK 251579f4ef
Tree-SHA512: 3e58d7a891019ccd272417eadc977037167439e3385a7b47c060fe93eba9c47fc37cdc0ca5551174ef2d93b256b2804ad7c01f5f2470ef9e9b7b912877aed11c
db888fa4cc io: Remove blanket trait impls (Tobin C. Harding)
Pull request description:
Remove the blanket impls of `Read`, `BufRead`, and `Write`. Replace them with just the impls required to get `rust-bitcoin` building.
Note, the `TcpStream` stuff is used in `examples/handshake.rs`.
Fix: #2432
ACKs for top commit:
Kixunil:
ACK db888fa4cc
apoelstra:
ACK db888fa4cc
Tree-SHA512: 24a3196e740f7e16a493c2f54fb9bf875fceab66c8973ffe28c7cfc9e1a440e14a36d919cc1d8055ac9da8cd4ffb0fc26fff058b8dbb9da4768d7cf4c07ec48a
7d538c830d units: Implement ops::Neg for SignedAmount (Tobin C. Harding)
Pull request description:
Its useful to be able to do `let x = -btc_amount;`
Implement `core::ops::Neg for SignedAmount`, returning a `SignedAmount`.
Fix: #2470
ACKs for top commit:
Kixunil:
ACK 7d538c830d
apoelstra:
ACK 7d538c830d
Tree-SHA512: 168808c34513ccf7773ba03abe9375f3bed0fa92b320af5538620142552fecda671b75295a8ba6720f1aead3c722869d8dcffeeaab565c370973d2bcb8b59d1b
7e1ba7895f Remove broken kani test (Tobin C. Harding)
Pull request description:
This test is failing. I do not want to dive back into kani right now, just remove it.
This is what I originally did in #2454 but changed directions and tried to fix it. Running kani test takes ages and I'd need to dig back to refresh my memory to work with kani. I don't have the motivation to do that at the moment. Just remove the test.
FTR I added the test recently without fulling thinking it through and it has never passed so we are not loosing any coverage. Doing this was the original mistake I should not have made.
ACKs for top commit:
Kixunil:
ACK 7e1ba7895f
apoelstra:
ACK 7e1ba7895f
Tree-SHA512: cb76807173b637be9d5ce790b015e711ca76add95ce0f0acfdc56947c075f57ea89774c09c4314dbc89086dcf7a8e21053552bfae805fd5dc9c91051cd53c468
1d13020129 test: Add conditional check for debug_assertions (yancy)
Pull request description:
Currently running `cargo test --release` blows up because of the tests that panic. This PR adds a conditional check `debug_assertions` which causes those tests to not be run in release mode. Besides fixing `cargo test --release` this PR sets the stage for a larger PR to test arithmetic tests that are `unchecked` (will overflow in release mode instead of panic) started here https://github.com/rust-bitcoin/rust-bitcoin/pull/2436. Also I think we ought to add `cargo test --release` to CI.
ACKs for top commit:
Kixunil:
ACK 1d13020129
apoelstra:
ACK 1d13020129
Tree-SHA512: 8964af57d20e314f491261b280ade053de03f5cb6a2857208b3cc16a9b39fa37fa4044cec84a46e35eac2a4b2637ebbd1c250817aab397b8a30f620eb61725fc
Remove the blanket impls of `Read`, `BufRead`, and `Write`. Replace them
with a set of sane impls.
Note, we add code to the `impl_write` macro to implement both
`crate::Write` and `std::io::Write` when "std" feature is
enabled.
Fix: #2432
10cf51c4c5 Inline private ScriptBuf::p2wpkh function (Tobin C. Harding)
Pull request description:
This function is a bit unclear and is only called once, just inline it.
Refactor only, no logic changes.
ACKs for top commit:
apoelstra:
ACK 10cf51c4c5
Kixunil:
ACK 10cf51c4c5
Tree-SHA512: 3907923f2258089a5fc1cc1e1d0b34e99457d69a5822cefa7bf90405d7ac05d570fb2855f62e2b5b4b871485e349e8dc09eb8f14c0676a8bdd70593e345b9b41
f317d87ee6 io: Enable "alloc" from "std" (Tobin C. Harding)
1d00d47b32 io: Add Changelog (Tobin C. Harding)
83397c465c io: Add documentation to all public types and functions (Tobin C. Harding)
2810b08b0d io: Add code comment to feature gate (Tobin C. Harding)
4cf2bf4b40 io: Make Take::read_to_end public (Tobin C. Harding)
Pull request description:
Do some cleanups to the new `io` crate.
- Make `Take::read_to_end` public
- Add CI script
- Add documentation
- Add changelog
ACKs for top commit:
apoelstra:
ACK f317d87ee6
Kixunil:
ACK f317d87ee6
Tree-SHA512: 6c7bc0d629a8995d985f8d8a245579ecdac6d0c10fa885c9a3550cc313a933c05ba087340fa3a638a9b631998157f86d439d17e56208f2457ee9ada9741f203d
d3d5ee1047 Improve error handling in errors emmited by `keys` (harshit933)
Pull request description:
For now I have tried to group those functions which can produce more than one error and changed the functions which were generating single error from `Key::Error` to the respective error. Let me know if this needs to be changed.
Also in `psbt/error.rs` I have changed the `InvalidPublicKey(crate::crypto:🔑:Error)` to `InvalidPublicKey(crate::crypto:🔑:FromSliceError)`. What should be done here?
Changes -
- in `from_slice` changed the `error` to `FromSliceError`.
- in `verify` changed to `secp256k1::Error` as it can return only one error.
- in `from_str` changed to `FromSliceError`.
- in `CompressedPublicKey` changed `verify` from `Error` to `secp236k1::Error` as it only returns one error.
- introduces CompressedPublicKeyError
- Removes impl from `bip32.rs`
Potential fix#2291
ACKs for top commit:
Kixunil:
ACK d3d5ee1047
tcharding:
ACK d3d5ee1047
Tree-SHA512: 21681bbf87c37eb0caaefe4b356a8a5e1d9b17de3207a0c9294de66b367ab348a7dda1916eb866fe4382e852af14ccab7b9f25a279291cd5beb56bb60b2523c2
ccbd09d5fb Remove unnecessary m/ prefix requirement (josibake)
Pull request description:
`m` in BIP0032 is a variable, not a constant. Requiring it as a constant here is confusing and can lead to erroneous conclusions if using this library as a means of understanding BIP0032.
Fixes#2449
ACKs for top commit:
Kixunil:
ACK ccbd09d5fb
apoelstra:
ACK ccbd09d5fb
Tree-SHA512: b641679f958f20a51c1890b23bbaa0153716802d6180dfd1f649e104f291c5a99143e02b75d292b22254201b28e5c53a04ecd7b6a88ff6f964073106419c5ec1
47569302fc Fix broken kani test (Tobin C. Harding)
Pull request description:
Recently we added a kani test that doesn't work because of `debug_assert` calls in ops traits.
Instead of opening the can of worms that is correct panic behaviour in ops lets just remove the test.
ACKs for top commit:
Kixunil:
ACK 47569302fc
apoelstra:
ACK 47569302fc
Tree-SHA512: f4a862d99173c1502e70fe4c2b9085a1f23dd4501f2ae25dc8a92e3edda7804b42b0580ef32fef2a3d5ea0d98e16b6f0fdba456cf4f0926c5b051ec8a6e54c78
In BIP0032, m is used as a variable for the root extended key. It is not
meant to be used as a constant prefix when serializing paths.
Update the DerivationPath parser to no longer require the m prefix.
Remove the m prefix from the unit tests and the bip32, ecdsa-psbt,
and taproot-psbt examples.
close#2449
Changes -
- in `from_slice` changed the `error` to `FromSliceError`.
- in `verify` changed to `secp256k1::Error` as it can return only one error.
- in `from_str` changed to `FromSliceError`.
- in `CompressedPublicKey` changed `verify` from `Error` to `secp236k1::Error` as it only returns one error.
- introduces CompressedPublicKeyError
- Removes impl from `bip32.rs`
- introduces `ParsePubKeyError` to return errors while generating publickey from string
3c62f74684 Add public functions p2wpkh_script_code (Tobin C. Harding)
a246dc98a4 Run sighash example in CI (Tobin C. Harding)
Pull request description:
This was done to fix#1920, it may be of questionable value though.
- Patch 1 is definitely useful, its a CI fix.
- Patch 2 adds two new API functions.
Fix: #1920
ACKs for top commit:
Kixunil:
ACK 3c62f74684
apoelstra:
ACK 3c62f74684
Tree-SHA512: 58743612c48e392f9ac0a94477588aee959c5fe9191dd04405bbb71aed7b0730b5927ad98f9da34dc93caaaac939617348c3f71318cc7e65c2c154b0f3897b89
c084afa8b2 Print hex in Debug for Sequence (Tobin C. Harding)
Pull request description:
Printing the `Sequence` as a decimal is not super useful when debugging, print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
ACKs for top commit:
Kixunil:
ACK c084afa8b2
apoelstra:
ACK c084afa8b2
Tree-SHA512: d60cd8896ca56a30fc8bd030cf3dd1bc1fd3a1609e99bfc2f26b9bd665b11c34c9df93b3f3ad731506d916513ca4a192dde476e16d99f2d4c4b2697f70a7bc98
343510d3a0 kani: fix Amount overflow test (Andrew Poelstra)
Pull request description:
Our Kani CI job is currently failing. See https://github.com/rust-bitcoin/rust-bitcoin/actions/runs/7770495422/job/21190756253
This fixes one of the issues; the other is that we're hitting a multiplication assertion in the test we added in https://github.com/rust-bitcoin/rust-bitcoin/pull/2393 which I'm unsure how to deal with.
For reference, testing this was a bit of a PITA. I needed to
```
# Ok, these steps are easy/obvious
cargo install kani-verifier
cargo kani
```
This will give you an error located in core/panic.rs or something with the description `This is a placeholder message; Kani doesn't support message formatted at runtime` which is not super helpful. To get the actual failure, you need to write
```
cargo kani --enable-unstable --concrete-playback=inplace
```
which will add a weird unit test which calls into Kani to exercise the original test with a specific input value. Because it calls into Kani you can't just run it with `cargo test`. You need to run
```
RUST_BACKTRACE=1 CARGO_INCREMENTAL=0 cargo kani playback -Z concrete-playback -- kani_concrete_playback_check_div_rem_8626518785677487871
```
where `CARGO_INCREMENTAL=0` disables incremental compilation (this was causing rustc to flame out with a "filename too long" error because it was trying to create some intermediate file with multiple hashes and crate names in it), and the `kani_concrete_playback_123456789` thing is the name of the test that gets added (which you can easily find by reading `git diff`).
ACKs for top commit:
tcharding:
ACK 343510d3a0
Kixunil:
ACK 343510d3a0
Tree-SHA512: 398ce3c61ffa3246bd27ae5719b4ac4fda587e87b8645ec8418fdfd039e4ed78d58233faab27bc63df7e2a30bb5467660e77a6e3d3a08fe86e7ff3dd31869ec7
Add two public API functions on the two public keys, both called
`p2wpkh_script_code` to do exactly as the name suggests.
Of note, I was not able to find anywhere to use these in example code,
this is because of we always use the new `p2wpkh_signature_hash`
function. The new functions may be useful for a user calling
`segwit_v0_encode_signing_data_to`. The may help document the library as
well.
Printing the `Sequence` as a decimal is not super useful when debugging,
print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
8c17ad7fd7 Remove non_exhaustive from struct errors with pub inner (Tobin C. Harding)
Pull request description:
Using `non_exhaustive` as well as a public inner field is incorrect, it prohibits users from creating or matching on the error and does not achieve forward comparability.
This was never right, we shouldn't have done it.
ACKs for top commit:
Kixunil:
ACK 8c17ad7fd7
apoelstra:
ACK 8c17ad7fd7
Tree-SHA512: 41266aaea25e0e5dba22200725e71f7cc23f386f3990c9d0b831980db2cfb431791ba14d6c6b144bd7db90f2f5dc9df38856f23fade0d7aee68217c4c879d3e0
Using `non_exhaustive` as well as a public inner field is incorrect, it
prohibits users from creating or matching on the error and does not
achieve forward comparability.
This was never right, we shouldn't have done it.
Done in preparation for an initial v0.1.0 release of the new `io` crate.
Add a changelog file with a brief description of whats in the initial release.
Currently `Take::read_to_end` is private forcing users to use our
"custom" `read_to_limit`, for seasoned Rust hackers
`foo.take(16).read_to_end(buf)` make be more unsurprising.
Make `read_to_end` public.
c69caafefc Remove attribute comments (Tobin C. Harding)
3e83ef9276 Remove consensus error wrapper TODO (Tobin C. Harding)
bfabea94e9 Remove unwrap comment (Tobin C. Harding)
8bdaf4a34d Remove carrying_mul TODO (Tobin C. Harding)
Pull request description:
Add issues and remove the TODOs from the code.
Resolves: #2368
ACKs for top commit:
apoelstra:
ACK c69caafefc
Kixunil:
ACK c69caafefc
Tree-SHA512: b10a3de8da7ace890735023f8441605dd11b0227c27a2357556b8aaa8276a7f34ed220e3bcbc93aad4b35357319318ff7de27210e8f60dd90f6c55af23e21470
01a66a7fa7 CI: Check for required commands (Tobin C. Harding)
5c15ed5441 CI: Epic overhaul (Martin Habovstiak)
242aa676b3 Use env bash instead of /bin/bash (Tobin C. Harding)
422d30117c Use bash to run shell scripts (Tobin C. Harding)
Pull request description:
The combination of some work by myself [0] and Kix [1].
Draft so I can use github's infrastructure to test it all out.
Includes some patches at the front to fix real issues that the new test infrastructure found - WIN.
[0] https://github.com/rust-bitcoin/rust-bitcoin/pull/2328
[1] https://github.com/rust-bitcoin/rust-bitcoin/pull/2343
Coincidentally this closes 1124
Resolve: #1124
ACKs for top commit:
apoelstra:
ACK 01a66a7fa7
Tree-SHA512: 026a0948a181102246702eadc3ff245c319c456b03ada9ca269141d006146f30fd8eb50377062735a06c3e369f7edac2e334587120338a3747810d999177d930
Use `bash` instead of `sh` to run shell scripts.
We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
0997382772 io: Enable alloc from std (Tobin C. Harding)
ba1166a63b Make crate level attributes uniform (Tobin C. Harding)
Pull request description:
Make the trait level attributes uniform across all released crates in the repo. Excludes things that are obviously not needed, eg, bench stuff if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an open PR at the moment to add that along with the required docs.
ACKs for top commit:
apoelstra:
ACK 0997382772
Kixunil:
ACK 0997382772
Tree-SHA512: ef1f638aca171536287cce369be98998e871d26468ad2d8c39d9004db610b406471809c283540a4a19bcede78b12b8976a1bb37e5d431fbff8c8a3e53a64d4e3
4383202f23 CI: Add a job to build kani proofs (Tobin C. Harding)
96d3bbd065 Fix kani test (Tobin C. Harding)
Pull request description:
Recently (in #2379) we patched the `ParseAmountError` but we don't check kani code on every pull request so we broke it.
Fix kani test to use the new `OutOfRangeError`.
EDIT: Attempt, as a separate patch, to add a job that runs on each PR to build the kani test code.
Close: #2424
ACKs for top commit:
Kixunil:
ACK 4383202f23
apoelstra:
ACK 4383202f23
Tree-SHA512: dcddcb0d52201efb3246733e9f164f5acde22df256fc4985b23050628ab9ae9c20a80ecd4ab468558b0a8708dacf6f7af099e8303cf4f73e1557e454c351aa34
Currently we do not build the code in the kani tests when PRs are
pushed, instead we run the verifier once a day. We should at least check
the code builds on each PR. One way to do this is to build the proofs
without running them, `kani --only-codegen` does that.
Andrew Poelstra
yancy
conduition
Tobin C. Harding
Sumit Kumar
josibake
harshit933
dependabot[bot]
Martin Habovstiak