The `taproot_control_block` did not properly detect whether it deals
with script spend or key spend. As a result, if key spend with annex was
used it'd return the first element (the signature) as if it was a
control block.
Further, the conditions identifying which kind of spend it was were
repeated multiple times but behaved subtly differently making only
`taproot_control_block` buggy but the other places confusing.
To resolve these issues this change adds a `P2TrSpend` enum that
represents a parsed witness and has a single method doing all the
parsing. The other methods can then be trivially implemented by matching
on that type. This way only one place needs to be verified and the
parsing code is more readable since it uses one big `match` to handle
all possibilities.
The downside of this is a potential perf impact if the parsing code
doesn't get inlined since the common parsing code has to shuffle around
data that the caller is not intersted in. I don't think this will be a
problem but if it will I suppose it will be solvable (e.g. by using
`#[inline(always)]`).
The enum also looks somewhat nice and perhaps downstream consumers could
make use of it. This change does not expose it yet but is written such
that after exposing it the API would be (mostly) idiomatic.
Closes#4097
f61e93ccf1 Properly deprecate Hash::from_slice (Tobin C. Harding)
50c0af7138 Stop using Hash::from_slice (Tobin C. Harding)
Pull request description:
The `hashes::error::FromSliceError` error is only returned from `from_slice`. We attempted to deprecate this function but it seems we only did half a job at it.
- deprecate _all_ instances of the method/function
- deprecate the error type
- stop using the deprecated functions in `bitcoin`
Close: #4053
ACKs for top commit:
apoelstra:
ACK f61e93ccf1db7e7e3c9604fdb09b4e25195d88b2; successfully ran local tests
Tree-SHA512: 61a0e5127019859776ffac66bd4d320c86b8462bb1e908127d0bf42896aaa8df85fd2b06850342b694ca1cd68ed50355c81cad6ae3e9a5fd6e3933efe85498ad
It has turned out that the `rust-ordered` crate and it's
`ArbitraryOrd` trait are only useful for locktimes and only marginally
useful for them at best.
Remove the `ArbitraryOrd` impls and the `rust-ordered` dependency.
This topic was discussed in various places including:
- #2500
- #4002
- #3881Close: #4029
This function is deprecated, stop using it in favour of
`Hash::from_byte_array`.
Patch only touches test code, I'm guessing that is why lint warnings
were no showing up.
6244cb75fa Introduce monadic AmountOpResult (Tobin C. Harding)
Pull request description:
We would like to return an error when doing math ops on amount types. We cannot however use the stdlib `Result` or `Option` because we want to implement ops on the result type.
Add an `NumOpResult` type. Return this type from all math operations on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators of amounts.
Note please this removes `AddAssign` impls for amount types.
ACKs for top commit:
apoelstra:
ACK 6244cb75faf62aed4b47d63a59d14cb766e4e7a8; successfully ran local tests; let's do it -- but definitely want the followup issues addressed
Kixunil:
ACK 6244cb75fa
Tree-SHA512: 7a105acb1aa492ab3e97d94ae182ac4c30a364edd183f71cc320cf80d85060049e8caf1e5736ef6d1af32f39c3376f21382afe35ac65ea1b8c15130c622d9d64
For private WIF keys corresponding to a compressed address,
the last byte of the key needs to be 0x01, but the API
doesn't enforce this when using PrivateKey::from_wif(). So,
invalid keys can be accepted.
Thus we check if the last byte is equivalent to 0x01
if the key's length is 34 (which indicates it's
compressed).
Currently in order to release `hashes v1.0` we need to 1.0 `io` as well.
For multiple reasons, many out of our control, the `io` crate may not
stabalise any time soon.
Instead we can invert the dependency between the two crates.
This is an ingenious idea, props to Kixunil for coming up with it.
Notes
- `io` does not currently re-export the `hashes` crate.
- This work highlights that we cannot call `hash_reader` on a siphash.
- The `Hmac::hash_reader` uses the default key which may not be obvious.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
We would like to return an error when doing math ops on amount types.
We cannot however use the stdlib `Result` or `Option` because we want to
implement ops on the result type.
Add an `AmountOpResult` type. Return this type from all math operations
on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators
of amounts - somewhat ugly to use, see tests for example usage.
6002ccdc56 Add a tagged sha256t hash engine (Tobin C. Harding)
3e8e2e46bf Use Self::Engine in GeneralHash impl (Tobin C. Harding)
a0211906fe sha256t: Remove standalone from_engine function (Tobin C. Harding)
5ce8781162 Remove the Tag::engine method (Tobin C. Harding)
ba6425947f hashes: Use associated cost for pre-tagging (Tobin C. Harding)
613fddc82b Delete deprecated sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
Add a new hash engine to the `sha256t` module and put the tag on it.
Note the issue suggests adding the tag to `sha256::HashEngine` but this PR adds a new type to `sha256t` and adds the tag on it.
Resolve: #1552
ACKs for top commit:
apoelstra:
ACK 6002ccdc567b26c69f6b55173f5d11b95b4d6966; successfully ran local tests
Kixunil:
ACK 6002ccdc56
Tree-SHA512: 8a8945bdb89841b87af2eb94073b4d01993338f34a7a3e919bccf4e8edea0fe9d5d2818b6484700b2e72143f315633338692a436149935c5156b77757b025f5e
We would like it if two different pre-tagged engines were considered
different types so it is not possible to mix them up.
Add a new `sha256t::HashEngine<T>` where `T` is a tag the same as on
`sha256t::Hash<T>`.
Now we have an associated const we can do away with the `engine` trait
method all together. Users can call `Hash<FooTag>::engine` instead. This
is better because its an API more similar to the other hash types and
therefor easier to discover and remember.
85e0330d7f Run the formatter (Tobin C. Harding)
7be0db730a hashes: Move bench and test code into files (Tobin C. Harding)
665fa9de99 hashes: Pull crypto out into submodule (Tobin C. Harding)
1bfd1e071a hashes: Make module subdirectories (Tobin C. Harding)
Pull request description:
This is an attempt at point 3 in https://github.com/rust-bitcoin/rust-bitcoin/pull/3961#issuecomment-2619946074
However instead of using `api.rs` and `implementation.rs` it uses `crypto.rs` for the cryptography stuff and leaves the rest in `mod.rs`.
This whole PR is internal changes only. Almost entirely code moves, review is easy if you have your `diff` configured nicely.
ACKs for top commit:
Kixunil:
ACK 85e0330d7f
apoelstra:
ACK 85e0330d7f057c9fe447bff5fdb6023150ead319; successfully ran local tests; look great! thanks!
Tree-SHA512: e52e6410e86fc93ec34a72be8c64f02148f46958f8f5c1850075b1a7f41886c220f09144ccd05a98a551c356a5d25524c8884fc8578a205b27f385ec0725f13b
a7526b6a70 Remove `fee_vb` (yancy)
73b14d03b9 Add `to_fee` in place of `fee_wu` (yancy)
Pull request description:
Rename fee_wu and remove fee_vb
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3908
ACKs for top commit:
apoelstra:
ACK a7526b6a70642e04f0a8ad8a89aa368031fdb6cd; successfully ran local tests
Kixunil:
ACK a7526b6a70
Tree-SHA512: fd865525e56caddc49158fd94fb125d92fd09654d5c72609a6f8e34370e79a9be4213dbd7e69b0d8498d92fca8a970142262f56b63cebd76c200aca75b6e0ca6
Use the more idiomatic to_fee instead of `fee_wu`. Since the method
takes a strongly typed argument, remove `wu` from the method name
to improve clarity.
We want to get rid of this constant, so we replace it in tests with 0
amount, empty script. Notably, the tests were already using it as a
dummy value - the exact amount was irrelevant, so this change doesn't
break anything.
The `encode_signing_data_to_inner` function previously constructed a
transaction internally, requiring a bunch of allocations, which it'd
then consensus-serialize into a writer (hasher). It also used a dummy
`TxOut::NULL` value which we want to get rid of.
To get rid of both allocations and the NULL value we serialize the
transaction on-the-fly. Because the encoding doesn't involve witnesses
it's not too complicated and the consensus encoding will never change so
there are no desync bugs possible. We may later change this to an
abstract transaction though.
4dad4730a8 Add unreleased changelog entry (Tobin C. Harding)
08bb57e499 Use MAX_MONEY in serde regression test (Tobin C. Harding)
Pull request description:
We plan on enforcing MAX_MONEY in the amount types. In preparation use MAX_MONEY in the serde regression test instead of the arbitrary, and too large DEADBEEFCAFEBABE value.
ACKs for top commit:
apoelstra:
ACK 4dad4730a81f9f08a565f196e5699f054e28f977; successfully ran local tests
Tree-SHA512: 10fefdb75289f4c93537e031410eb729420729563b262d6104948a0176a78c4f6130956b550cc08ffc924b1bce1c42f0544542879a4d44165022196ef695718a
We plan on enforcing MAX_MONEY in the amount types. In preparation use
MAX_MONEY in the serde regression test instead of the arbitrary, and too
large DEADBEEFCAFEBABE value.
We have an `_unchecked` amount constructor that makes no assumptions
about the argument. We would like to start enforcing MAX_MONEY but the
diff to introduce this is massive. In an effort to make it smaller we
can do all the hardcoded ints first. We did this already but a bunch
more snuck in or were missed.
In any amount constructor that passes in a hardcoded const as a decimal
integer (i.e., not hex) use the `_unchecked` version.
Done in preparation for enforcing MAX_MONEY.
New nightly lint warning "called `Iterator::last` on a
`DoubleEndedIterator`; this will needlessly iterate the entire iterator"
Code that gives the warning is correct, allow the lint to remove the
warning.
Update rustc nightly to 2025-01-16
Back in 2022 we elected to use `mutagen` for mutation testing. Since
then `cargo mutants` has progressed to a point where we would now like
to use it instead.
Remove all the `mutagen` stuff and update the lock files.
Close: #2829
e0028239cf api: Run just check-api (Tobin C. Harding)
5eb5941215 Add FIFTY_BTC const to the amount types (Tobin C. Harding)
Pull request description:
The mining reward for the first epoc is 50 bitcoin. For mainnet this is old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the amount types uniform also add it to the `SignedAmount`.
ACKs for top commit:
storopoli:
ACK e0028239cf
jamillambert:
ACK e0028239cf
apoelstra:
ACK e0028239cf207660deb2873a92bacfa1315af634; successfully ran local tests
Tree-SHA512: 623ed8b1f5fe8dd95309179308fea83d68be4349becf6305769b0378cc9032961df2c062dc2bf702fec5e2394e8abb7360d2be6f19b6cf505db8769a5ae39e16
The mining reward for the first epoc is 50 bitcoin. For mainnet this is
old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the
amount types uniform also add it to the `SignedAmount`.
b11ace359a Fix up ParsePublickeyError (Innocent Onyemaenu)
Pull request description:
Resolves#3835
In #945fcd09 we forgot to Update impl for ParsePublicKeyError Display and Error traits
ACKs for top commit:
tcharding:
ACK b11ace359a
apoelstra:
ACK b11ace359a52d9137b8fe5919d9825d61e3e9fad; successfully ran local tests; thanks!!
Tree-SHA512: 5c4e5e113605bc5b9c3c0d2ca65a0fdae80726a0f2f791255cc1d6567812a9132ff4f5bd8f30276fef916469d5fd29f6d72ddf4adb6340e52f4e0198ac4fb371
In #3847 we added an `InvalidCharError` into one of the variants of
`ParsePublicKeyError` but we forgot to update the trait
implementations.
Fix the `error::Error` and `Display` implementations for
`ParsePublicKeyError`. While we are at it match on `*self` as is
typical in this codebase.
With this applied #3835 is fully resolved.
Close: #3835
Currently `InputString` is in the public API of `units` because of the
trait bound on `parse::int()`. We can just do the monomorphisisation
manually to remove it.
This patch renames `int` to have three different names, one for `&str`
one for `String`, and one for `Box<str>`.
1e0c065740 typo: Address::is_valid_for_network (Jose Storopoli)
Pull request description:
I believe this is a typo.
ACKs for top commit:
apoelstra:
ACK 1e0c065740266a8ad207a95f4d964394ba3f07d7; successfully ran local tests
tcharding:
ACK 1e0c065740
Tree-SHA512: aff71b91dc707c0c6c723e35510d284fd9a60895b7f12828819eb0d4520216491a2cf054b59872ac4be175d566674a32bde8c170f43beb6fc15dd19e6be12e45
706a135de6 bitcoin: Add an example of doing I/O to encode/decode (Tobin C. Harding)
Pull request description:
In an effort to improve the documentation on `bitcoin_io` add an example in `bitcoin` crate that demonstrates a few things:
- Encode/Decode a `rust-bitcoin` type to/from a stdlib type.
- Encode to a custom type by implementing `bitcoin_io` traits.
- Encode to a foreign custom type by using the `bitcoin_io::bridge::FromStd` wrapper.
Later we can link to this example online in the `bitcoin_io` docs.
ACKs for top commit:
apoelstra:
ACK 706a135de6ca2be0231c2f62f4f2b156e08c9a49; successfully ran local tests
Tree-SHA512: cd3ff4067d5b86031255cb31fe17dea3fd22479699d32efad93c359de465ef6250f29aa4c43ce4218ae623fa6c355661960c11908a729a895655319141b82852
e09bdb5f98 Add BIP324 V2 p2p network message support (Nick Johnson)
Pull request description:
Migrating over the BIP324's library's V2 network message encoding and decoding. As discussed over in https://github.com/rust-bitcoin/rust-bitcoin/discussions/2959, this is probably the natural home for it and also cleans up some gross copy/pasta of some of the encoding/decoding chain logic.
This patch adds `V2NetworkMessage` which wraps a `NetworkMessage`, but handles the V2 encoding and decoding. It is a parallel of the existing `RawNetworkMessage` (which mentioned before, may be better described as `V1NetworkMessage` https://github.com/rust-bitcoin/rust-bitcoin/issues/3157). A priority of this patch was to not re-invent any wheels and try to use the existing patterns as much as possible.
ACKs for top commit:
tcharding:
ACK e09bdb5f98
apoelstra:
ACK e09bdb5f98ea516382a04283373ad97a41d57c2b; successfully ran local tests; nice!
Tree-SHA512: a5078d4d3deb04c2e06ea513bbc8c97d0e6d5da5b029847a97b3f90bf55a263858dd16d88299f853aa3c468f7b9bceb3973c5652a49d3e96df3e91181b455f29
29a71de928 Bound Address parsing on NetworkValidationUnchecked (Tobin C. Harding)
cf455d3a06 Fix typo in prifixes (Tobin C. Harding)
Pull request description:
Currently it is not possible for downstream to use a generic on the `Address` type in structs in conjuncture with
derives (`serde::Deserialize` and `Display`) because our impls are only done for `NetworkUnchecked` (as they should be).
However, as observed by dpc, if we add a secondary marker trait and use it to bound the impls, implementing the new marker for `NetworkUnchecked` then downstream can use derives by way of
```
#[derive(Serialize, Deserialize)]
struct Foo<V>
where V: NetworkValidation,
{
#[serde(bound(deserialize = "V: NetworkValidationUnchecked"))]
address: Address<V>,
}
```
This is cool as hell because the `Address` type is currently a royal PITA.
Patch 1 is trivial cleanup.
To get past a build error in `FromStr` I used this little trick
```rust
// We know that `U` is only ever `NetworkUnchecked` but the compiler does not.
Ok(Address(address.0, PhantomData::<U>))
```
Resolve: #3760
and
Close: #3856
ACKs for top commit:
apoelstra:
ACK 29a71de92817bccd49b42b1055cc570832e6b959; successfully ran local tests
Tree-SHA512: 7c158dddb9fdbaaa1e48204bbf915b18ced56f5d82ce82630db6c0b52161bcf43b3ac413fa990a23975743c56917985b2666a74f9067221f003f2dcf080f827e
Martin Habovstiak
merge-script
Tobin C. Harding
Fmt Bot
ndungudedan
Andrew Toth
yancy
yancy
Peter Todd
Jamil Lambert, PhD
Innocent Onyemaenu
Jose Storopoli