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
cb270eae8e Make transaction::Version field private (jrakibi)
6c69b66b0d Use Version constant (jrakibi)
Pull request description:
This commit addresses #4041 by making the `transaction::Version` field private.
This forces people to either use the associated constants (`Version::ONE/TWO/THREE`) or the `non_standard`/`from_consensus` methods for any other transaction version.
This aligns with our approach to `block::Version`
ACKs for top commit:
tcharding:
ACK cb270eae8e
Kixunil:
ACK cb270eae8e
Tree-SHA512: dcf5b50dfeda04e56fec350acd735dcb7099989b552afce4261d559a8a846c0eb3705369ad635ef9bbbfb2373d203a2c3641178925de6685426aa91245db9a8c
This commit addresses #4041 by making the transaction::Version field private
Changes:
- Make the `Version` field private with `pub(crate)`
- Rename `non_standard` to `maybe_non_standard` for clarity since it accepts both standard and non-standard versions
- Add `#[inline]` attributes to small, frequently used methods:
- `as_u32`
- `maybe_non_standard`
Users now must use either:
- Constants (`Version::ONE/TWO/THREE`) for standard versions
- `maybe_non_standard` method for any version (standard or non-standard)
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).
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.
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
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>`.
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
316d8bcb01 Change all occurrences of "IO" to "I/O" (Jamil Lambert, PhD)
Pull request description:
Fixes#3871
ACKs for top commit:
tcharding:
ACK 316d8bcb01
apoelstra:
ACK 316d8bcb01504420a14854d2be122d1c8cffb4a9; successfully ran local tests; lgtm
Tree-SHA512: 437a95a1c36bcd4ae27aaacdfc5e0f3463e522a222c4a6ef2c3e234be4a24be2b600687bd58b300bf2b0a0d6596ab008f60903c91646458228eb34cf510908d6
2f91092d77 Derive Clone on consensus errors (Tobin C. Harding)
Pull request description:
All error types in the repo use either [0]:
- `#[derive(Debug, Clone, PartialEq, Eq)]``
- `#[derive(Debug)]`
However the `consensus` error types do not have `Clone` derived.
Derive `Clone` on `consensus::ParseError` and `consensus::Error`.
[0] Excluding `PushBytesError`, fixed in #3879
ACKs for top commit:
apoelstra:
ACK 2f91092d773e1618314b62c9e3eae17c08e1f5ad; successfully ran local tests; lgtm
Tree-SHA512: 2cae6faae97b63de311538c33eb7d1914330672744ca073bc6ea5c11ebd40a40ff3de9a38f1f5b101ff27798416d685a708599c298a6f82d7f5804575c2cb2c1
0870cd1660 Remove Copy from PushBytesError (Tobin C. Harding)
Pull request description:
The `PushBytesError` is the only error type in the codebase to derive `Copy`. Without thinking too hard this is unusual - remove it.
Thinking a bit harder it makes the code less maintainable because we must commit to implementing `Copy`.
ACKs for top commit:
apoelstra:
ACK 0870cd1660edd21739cc94075e4b3a1c7f1a7d15; successfully ran local tests; lgtm
Tree-SHA512: c71db5de634dfe2bd76336e5c31fab496f2a472a8dd164034233544c15bd89c84ff986e476fa9b7b05d01aa5332dd4bc93f63a93bf7a21e9a0ec67fc145739b2
a7c44cebf9 Use _unchecked to construct amounts (Tobin C. Harding)
09df951760 Use sat variable in tests (Tobin C. Harding)
4a5b2c60c6 Use ssat variable in tests (Tobin C. Harding)
Pull request description:
We have a `_unchecked` constructor now for both `Amount` and `SignedAmount`. Soon we would like to start enforcing the `MAX_MONEY` invariant in both amount types. To make that change easier do a few refactorings:
- Patch 1 and 2 introduce local variables for amount constructors.
- Patch 3 replaces the local variables introduce in (1) and (2) with macros
- Patch 4 uses `_unchecked` constructor for hard coded integers
The strange patch separation is done intentionally so we don't inadvertently reduce test coverage by using the wrong constructor. I made this mistake already in a previous PR, lesson learned.
Note please, the macro introduced in patch 3 is in preparation for enforcing `MAX_MONEY`. The macros allow us to panic (`from_sat().unwrap()`) instead of using the `_unchecked` version.
ACKs for top commit:
apoelstra:
ACK a7c44cebf9975c4eeba56a65c0ea65be90e5c7f3; successfully ran local tests
Tree-SHA512: 55c2428ae231882542a4cfa724675341f7b493d158f4bec26277d3eefb04d9597cc29b05dce859661a96855fa6f4bac250d53c3dfa9f86a9611d43387ee18667
All error types in the repo use either [0]:
- `#[derive(Debug, Clone, PartialEq, Eq)]``
- `#[derive(Debug)]`
However the `consensus` error types do not have `Clone` derived.
Derive `Clone` on `consensus::ParseError` and `consensus::Error`.
[0] Excluding `PushBytesError`, fixed in #3879
The `PushBytesError` is the only error type in the codebase to derive
`Copy`. Without thinking too hard this is unusual - remove it.
Thinking a bit harder it makes the code less maintainable because
we must commit to implementing `Copy`.
BIP324 introduced the second version of p2p network messages which
are designed to be used with the new encrypted transport. This patch
adds a V2NetworkMessage type which wraps a NetworkMessage and handles
the V2 encoding and decoding. It is a parallel of the existing
RawNetworkMessage type (which may be better described as
V1NetworkMessage). A priority of this patch was to not re-invent any
wheels and try to use the existing patterns as much as possible.
f94c7185fd Remove usage of impl_from_infallible in crates (Shing Him Ng)
Pull request description:
Fixes#3843
tcharding Copied your commit message from the other `impl_from_infallible` commit 😄
ACKs for top commit:
apoelstra:
ACK f94c7185fdd62e1ed98ed4016486406146c4d4f3; successfully ran local tests; nice!
tcharding:
ACK f94c7185fd
Tree-SHA512: 8c58c2c87f6892855d74a3306e1027a37394961f0a26b7bd88cc1654a190dda37234e7dde51a419dcd2f1bd1dd1ccceec16bbbc6fbdd5418ad21f10531b402b3
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.
We have a `_unchecked` constructor now for both `Amount` and
`SignedAmount`. In preparation for enforcing the `MAX_MONEY` invariant
use the `_unchecked` constructor throughout the codebase to construct
amounts from hard coded integer values.
2c9fda4135 Add parenthesis to explicitly show precedence (Tobin C. Harding)
Pull request description:
Recent clippy nightly update introduces warnings about precedence. While ours are, IMO, clear the lint docs have some cases that are not so I don't think we should ignore this lint. Specifically I could easily miss this one
1 << 2 + 3 equals 32, while (1 << 2) + 3 equals 7
ref: https://rust-lang.github.io/rust-clippy/master/#/precede
Add parenthesis to explicitly show precedence. Refactor only no logic changes.
ACKs for top commit:
apoelstra:
ACK 2c9fda413572e251fe3fd07f679b6c786da137b3; successfully ran local tests; yeah, these all seem reasonable
Tree-SHA512: 67b8cb56c0b271d41db58f9297e4be43410c04bf7bc2308789624084e5e9fee325a2eb3be7e56bd6b3904cf19c7a8b1b426e7adef7f4af18ea8c9d24b77a9db9
d5de89259b Fix typo in MerkleBlock::from_block_with_predicate comment (Jesus Christ)
Pull request description:
- Corrected a grammatical error by adding "are" to the comment in MerkleBlock::from_block_with_predicate.
ACKs for top commit:
clarkmoody:
ACK d5de89259b
apoelstra:
ACK d5de89259b64ed6d4756973073f06172d1d14090; successfully ran local tests
storopoli:
ACK d5de89259b
Tree-SHA512: 55595e8858e90ee5ab5ef7d592cf2c47dc593f303f3f60040f1612d92abcf4a4d0f9c9ad3bf7eece54f3078a3ab8ff92b36357be58db0919888e239a8a5c1a6b
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in the bitcoin, units, and internals crates
and just write the code.
Recent clippy nightly update introduces warnings about precedence. While
ours are, IMO, clear the lint docs have some cases that are not so I
don't think we should ignore this lint. Specifically I could easily miss
this one
1 << 2 + 3 equals 32, while (1 << 2) + 3 equals 7
ref: https://rust-lang.github.io/rust-clippy/master/#/precede
Add parenthesis to explicitly show precedence. Refactor only no logic
changes.
945fcd0920 fix ParsePublicKeyError using hex::InvalidCharError (Innocent Onyemaenu)
Pull request description:
Replaced the InvalidChar variant u8 with hex::InvalidCharError
Resolves#3835
changed InvalidChar variant of the ParsePublicKeyError from `u8` to `hex::InvalidCharError`
```
pub enum ParsePublicKeyError {
...
/// Hex decoding error.
InvalidChar(hex::InvalidCharError),
...
}
Also,
modified the test cases to accommodate the new variant
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
ACKs for top commit:
apoelstra:
ACK 945fcd09209120ef8869a2e4165e866328cc9bd5; successfully ran local tests; I like it
clarkmoody:
utACK 945fcd0920
Tree-SHA512: c13446c099cb02b4f253f9cc559a860aff3288a2cc5eac96d3cf910bf63e78957741bbdff69b936b16b36e46b366841a5c94876d16cbc0c41aea2a70866a6e45
85e04315d5 Remove test_ prefix from unit tests (Tobin C. Harding)
Pull request description:
There is a loose convention in Rust to not use `test_` prefix. The reason being that `cargo test` outputs 'test <test name>' using the prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some places and not others is confusing to new contributors and is leading me to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
ACKs for top commit:
shinghim:
ACK 85e04315d5
apoelstra:
ACK 85e04315d5eb90075ce55bf18fab8876a4583def; successfully ran local tests
Tree-SHA512: d90ae5ef75cc5e5a8f43f60819544f1a447f13cbe660ba71e84b8f27bfcc04a11d3afde0ed56e4eea5c73ebc3925024b800a1b995f73142cab892f97a414f14a
What:
- Replaced the InvalidChar variant u8 with hex::InvalidCharError
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the `debug_from_display`
macro and just write the code.
This is an API breaking change to `internals` but an internal change
only to any of the _real_ crates.
72e97c637f Add a hash value to Inventory's Error variant (Nick Johnson)
Pull request description:
I am working on adding BIP324 V2 p2p network message support to the p2p module and ran into a little snag. I can post a draft branch of that work if helpful, but the general strategy is to add a `V2NetworkMessage` which operates in parallel with the existing `RawNetworkMessage` type (which is a bit of misnomer and may be better described in the future as `V1NetworkMessage`, see #3157). This allows both p2p message types to hook into the existing `Encodable`/`Decodeable` chain.
But the `Error` variant of the `Inventory` type message does not have symmetrical encoding and decoding paths. Encoding writes out a zero'd 32 bytes while decoding ignores it completely. And while it is not clear to me if there is [requirement written down](https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors) anywhere, [bitcoin core does appear to always expect a hash](c1252b14d7/src/protocol.h (L499)) even with the `Error` variant where it is meaningless.
I believe rust-bitcoin's handling of this was never a problem before because the top level `RawNetworkPackage` pulls all the required bytes off a reader before decoding them. But this is not as easy to do with the v2 p2p network messages since the length is decoded at the transport level, not the message itself. I believe it is preferable for the Decoders to *not* assume that all bytes have been pulled off already given their input stream interface. Maybe somewhat surprisingly, this is the only issue I have run into so far adding the v2 encoding and decoding paths. As it is now, the code panics with an `Unconsumed` because it hasn't pulled the dummy zero bytes off the reader.
This patch adds the 32 byte array to the Error variant in order to make its Encoding and Decoding paths symmetrical. This also allows a reader to discard the 32 bytes when decoding a message while cleanly keeping things hooked up with the `Decodeable` chain. The hash is still not exposed to the caller.
This is *a way* to solve my issue, but not sure if it will cause more confusion than its worth. I tried a few other strategies, but preferred this one due to how well it hooks into `Decodeable`.
ACKs for top commit:
apoelstra:
ACK 72e97c637fa0916be75aef28ea8169ffbbe2c4f5; successfully ran local tests
tcharding:
ACK 72e97c637f
Tree-SHA512: 20cb9fec0768e0fdf7c7f520a00e1a37f5ef0583e2ebc7d47583249c6829c47826d83694a56338efa5844a9fe264a77f6d04c0c46c85f8732c7585515723d7ce
e13355318e Add From impl (yancy)
364e9ff775 Change method return type (yancy)
fdf3336ed5 Add unchecked variant (yancy)
Pull request description:
Any SignedAmount can now be cast to Amount since the range is the same. Specifically, the range for SignedAmount is (- 21 million, 21 million) while the range for Amount is (0, 21 million). Therefore any value from Amount can be cast to a SignedAmount and it will work. Note it's not the same and still requires checking when going from SignedAmount to Amount since Amount can't handle the negative range.
ACKs for top commit:
tcharding:
ACK e13355318e
Tree-SHA512: c016b51bdd87a12eb09d9c1a82699dad1e866bf96bd3235eeb131f216f02422acb992ddb3a8135af00bbc10e240178fde5e37fb7860d9e6eaf433cf917d4d16a
Any SignedAmount can now be cast to Amount since the range is the same.
Specifically, the range for SignedAmount is (- 21 million, 21 million)
while the range for Amount is (0, 21 million). Therefore any value from
Amount can be cast to a SignedAmount and it will work. Note it's not
the same and still requires checking when going from SignedAmount to
Amount since Amount can't handle the negative range.
As a side effect of changing the return type, TryFrom is no longer valid
and does not compile. Therefore in addition to changing the return
type, TryFrom is also removed.
774f066879 refactor: Change from u64 to Amount (yancy)
Pull request description:
Separate out refactor commit from https://github.com/rust-bitcoin/rust-bitcoin/pull/3794. Can be merged independently.
ACKs for top commit:
tcharding:
ACK 774f066879
apoelstra:
ACK 774f066879c8ad1af81c7e46b404fa63682a0b4c; successfully ran local tests
Tree-SHA512: 9ec5121d823ee3ec506eee5b5187bd496221bd3576afcaa6daf647099720d87b58b69521f29ae9537f123e2958771bc867b123da3f2ba941cba403a6c98e46de
The Amount type provides better type safety and is more appropriate in
this context than u64. Currently the checked arithmetic operations for
Amount and u64 are identical in behavior. Therefore, this refactor does
not result in any behavior change and is purely cosmetic.
While the hash value of the Error variant is meaningless, the variant
still conforms to all other Inventory messages and requires a 32
byte hash to be sent over the wire. This is how bitcoin core operates.
This patch adds the 32 byte array to the Error variant in order to make
its Encoding and Decoding paths symmetrical. This also allows a reader
to discard the 32 bytes when decoding a message. The hash is still not
exposed to the caller.
This was never a problem before because the top level RawNetworkPackage
pulls all the required bytes off a reader before decoding. But this is
not as easy to do with the v2 p2p network messages.
e56f461916 Make capitalization of SegWit uniform in strings (Jamil Lambert, PhD)
3520e832ac Make capitalization of SegWit uniform in rustdocs (Jamil Lambert, PhD)
Pull request description:
Fixes#3770
- Searched and replaced all occurrences of `//` * `segwit` (case insensitive) with `//` * `SegWit`
- Searched and replaced all occurrences of `"` * `segwit` (case insensitive) with `"` * `SegWit`
ACKs for top commit:
apoelstra:
ACK e56f461916b85928139950074d0df52caf4f919b; successfully ran local tests
Tree-SHA512: c56704102d8e86f26378bb302d5fdc7ea21d41fd49f55606e589ec32ff896f1adfb1960d8fb29abccfbf298b90fc357ed609d80fd0163dcb4ff01573646b02c3
f85456a726 Improve rustdocs on addresses module (Tobin C. Harding)
Pull request description:
These docs are stale, update them.
- Mention segwit and legacy
- Improve example code to show feature gating
- Fix headings to be as typical
ACKs for top commit:
storopoli:
ACK f85456a726
apoelstra:
ACK f85456a726a201b26eac58d22eb0eb6a58ad411b; successfully ran local tests
Tree-SHA512: c1cdde744fb960c4119805e6b3fb0cc971eaa94173fb85f8f7a34daab7985a0f14c9be477d7dda430c6873fed7265c598b8dcfbffe0ed6a99ca8590a2b2b724e
3ac9b2ded2 Remove unnecessary type (Tobin C. Harding)
Pull request description:
`hex_psbt` returns `Psbt`, no need for the explicit type.
Internal change only.
ACKs for top commit:
shinghim:
ACK 3ac9b2ded2
apoelstra:
ACK 3ac9b2ded24d5428945538c0a674301f6094df96; successfully ran local tests
Tree-SHA512: 2be1bfa56729bf97e60d721a184d3d21bfb27115fab1dd1b9103428658b5faca012d8f01e0458be7f17dadcd8cc495b327f2b472d825d46aa5cb71d0906ecb21
a5993426fd bitcoin: Improve rustdocs on extern crates (Tobin C. Harding)
Pull request description:
Make slight improvement to the extern crates we re-export from `bitcoin`.
ACKs for top commit:
apoelstra:
ACK a5993426fddea85c29ac6cd5a0e3096988d72b03; successfully ran local tests
Tree-SHA512: bb857316fc2606aa553f15e1c9fcf8dd87374e170121fe30cf9e44ae190f873f94c113af49b096cb49a65fd68d1bf24732e3f1aba04861e2ae934414f30473c6
6e01383f16 api: Run just check-api (Tobin C. Harding)
3855d3cc83 Move script hashes to primitives (Tobin C. Harding)
d1dd63d6d4 Remove wildcard in script re-exports (Tobin C. Harding)
Pull request description:
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the errors because they are in the `mod.rs` file so they should appear in both `primitives::script::FooError` and `bitcoin::script::FooError`.
ACKs for top commit:
apoelstra:
ACK 6e01383f162307a573c3454733ccae0596d3eaf6; successfully ran local tests
Tree-SHA512: 4ef838ee9c4cb3eb308ffa855ea5d6f5ad46f81c17b9413faa493a46a262afc18b47f28a0fdd5fc675eea31b895d0eb0e2505a523e820504ec88d9334d6874b4
0135cddc32 Refactor non_input_size (Tobin C. Harding)
f42f13cd8d Simplify closures in call to fold (Tobin C. Harding)
Pull request description:
Two refactorings to make the code easier to read. No logic changes.
ACKs for top commit:
apoelstra:
ACK 0135cddc32462490528dea549fd8db4c0a30646e; successfully ran local tests
Tree-SHA512: 747cfe1212283a62545cb86a5af0be46592ab3b41f94ab0a2dad8f06b094c04eb9265520ece595d2aab303fddf55a652e7ef977911439602550c264bedd4385b
3c8c956511 Remove Weight::from_wu_usize function (Tobin C. Harding)
Pull request description:
This constructor is an anomaly in this repo. Also it is ugly to have the type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like casts that don't obviously loose data. We have a solution for that already in `internals::ToU64`. This trait cannot be used in const contexts though so we do introduce a single cast in this patch.
ACKs for top commit:
apoelstra:
ACK 3c8c95651169fb9329ceb380162721c2d2f9b3aa; successfully ran local tests
Tree-SHA512: 168196edb7c151378d425b96ea3c9cdb0a4f6a879543e89facd02ed1fdf9bc69bde8ef862ffa0959b7c5ca21d6f4fe5ae38a933c379e7e88a946ca7cb68d61ec
99e2a9f530 Remove unnecessary rustdocs (Tobin C. Harding)
Pull request description:
The `deprecated` attribute already fully explains the status of a deprecated function and the replacement - no need to repeat this.
ACKs for top commit:
apoelstra:
ACK 99e2a9f530eced69e9df1441e15d24e626974385; successfully ran local tests
Tree-SHA512: 133d41573a3ba97abf85100649c1e8dfe131521a627e0b7f1ab803845caf1c183b99488cb5d9f7e858cab1b35f8907a79b320ec01901b15fdd63d08f0bc1f87e
4ef06b4f2b Remove ToU64 from rustdocs (Tobin C. Harding)
Pull request description:
The `ToU64` trait is not meant to be used publicly, remove the mention of it from rustdocs.
ACKs for top commit:
shinghim:
ACK 4ef06b4f2b
apoelstra:
ACK 4ef06b4f2b9945b46c190abd0cb3a6dfe234c7b5; successfully ran local tests
Tree-SHA512: e9acef9194cee380aa2aa44af9285d4f6e025bdf09f76e5066916c51215db15f1f920fd8ecf3cf079f0a2dddab0332b3a7df104f5bd61d12fc337b4ed163310c
This constructor is an anomaly in this repo. Also it is ugly to have the
type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like
casts that don't obviously loose data. We have a solution for that
already in `internals::ToU64`. This trait cannot be used in const
contexts though so we do introduce a single cast in this patch.
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the
errors because they are in the `mod.rs` file so they should appear in
both `primitives::script::FooError` and `bitcoin::script::FooError`.
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
428e9787d1 Explicitly define Ord for NodeInfo (Shing Him Ng)
Pull request description:
Fixes#3654 by explicitly defining Ord so that we avoid potentially catastrophically changing addresses out from under users
ACKs for top commit:
sanket1729:
utACK 428e9787d1
apoelstra:
ACK 428e9787d181a462d06a18b7a45701790cbc0929; successfully ran local tests
Tree-SHA512: e900e07b2c53f643e3239bf7d26567b56275899b408a94e45f1cddd81217141c304c87159ce413a7a4660f1c09a0db2bbc1146948a2d9e7600abf14cd73ac691
To prevent rounding errors converting to and from f64 change
`Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has
issues.
Add checks to `from_str_in`, `checked_add` and `checked_mul` that the
result is below MAX, where previously a u64 overflow was relied on.
Change tests to account for new lower MAX that is within the range of
SignedAmount and does not overflow so easily
Remove overflow tests
`Amount::MAX` is now below `u64::MAX` and within the range of values for
`SignedAmount`. These tests therefore do not overflow.
In effective_value there is no error with `Amount::MAX` and the correct
value is returned.
In psbt the removed test is effectively the same as the previous test.
Modify `Amount` tests to work with new `MAX`
Tests need to be changed that checked values above the new `MAX` or
`Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
b5a0731a23 refactor: remove unnecessary if block (Chris Hyunhum Cho)
Pull request description:
This `if...else` block can be replaced with one `if` block, by changing `==` to `!=`.
ACKs for top commit:
jamillambert:
ACK b5a0731a23
apoelstra:
ACK b5a0731a23f945f7e4cc7d8f3c0f1d3f83875272; successfully ran local tests
tcharding:
ACK b5a0731a23
Tree-SHA512: eee0d63532953d27fe081c36180ae9adb63c0ab7f1fb8324d9fc5e4a408f421d8df894efcccfc4bd3142625d6be6ea1258c9478faa025ce147ebe1b4e9377738
22769268f3 units: Close the hex parse errors (Tobin C. Harding)
Pull request description:
As part of the 1.0 effort close the errors in the `units::parse` module.
ACKs for top commit:
apoelstra:
ACK 22769268f34b45c0bd86c548eb13cfe1e290f0d7; successfully ran local tests; thanks!
sanket1729:
ACK 22769268f3
Tree-SHA512: 598ba236f8c08c3f750112e0d8b8e0aa0568be2700a328e4a2d8999ca59eada8a16a6a1d9e9121e7f42ce9bbe3da3f87221ba67c36056996a687e489f4c6007c
77085a1fa1 units: Remove serde from amounts (Tobin C. Harding)
Pull request description:
The `Amount` and `SignedAmount` were not supposed to implement `serde` traits by design because doing so implicitly uses sats. We provide two modules `as_sat` and `as_btc` to allow users to explicitly serialize in their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives were added for `serde` and we did not notice it during review.
ACKs for top commit:
apoelstra:
ACK 77085a1fa1f39e7358202e49189282e48a965747; successfully ran local tests
Tree-SHA512: ff4c1f34191e12d955e66737493af4b2dad761fa6f95eb334b86715906e70d2180e7f15d8b4af302d53fb0b1c8b5fb50b4924e9fcdee8824840ba555e73c1c17
60f43a893d Remove duplicate test case (Tobin C. Harding)
Pull request description:
In commit:
`a10d5e15b3 Extract the Script assembly creator from fmt::Debug`
A test case was refactored and where there used to be two test case, one for `Debug` and one for `Display`, two identical test cases were left.
Remove duplicate test case.
ACKs for top commit:
apoelstra:
ACK 60f43a893d67c221f61e289cab6394418411cf55; successfully ran local tests
Tree-SHA512: 8d21f07b33c9f88ac820422b9f5471bf53e36050a31854a7152ab14c6e25654455f4eb366ea2b497a80f741af36bf068b1df8a69321a0299c9bae60f001b354e
f961f3c0ec Add test (yancy)
3352032892 Fix internal weight calculation (yancy)
Pull request description:
A fix for 261c8d8ae6. Maybe it's better to roll back that commit instead. Clearly there needs to be a test or two added here as well since this change causes not test failures.
ACKs for top commit:
apoelstra:
ACK f961f3c0eca4e9b175761756f48699012b63d70a; successfully ran local tests
sanket1729:
utACK f961f3c0ec
tcharding:
ACK f961f3c0ec
Tree-SHA512: e6bb6d492b6a85f328bdfae6d0e5df9ea48c9c06bd0c310816826c893163bd0a6071af893fa7012fbd4eea0394bc8997277151486807fb8bd06ac78ce8cfc5fd
4d0d78a3af Fix typo in from_next_work_required documentation (Shing Him Ng)
5e47c4123d Update CompactTarget::from_next_work_required to take timespan as i64 (Shing Him Ng)
Pull request description:
[In Core](f7144b24be/src/pow.cpp (L56)), the timespan is defined as a signed int64. This PR will update `from_next_work_required` to take in an `i64` instead of a `u64`
Fixes#3652
ACKs for top commit:
tcharding:
ACK 4d0d78a3af
apoelstra:
ACK 4d0d78a3af2ca300c6b40348724e806bd51ef27a; successfully ran local tests
Tree-SHA512: eea33ec18b081a97ae2a22c681982ad8693200ff8de3a3fc8f46b84b5e72f41589b79140ddd79ba3fce628f1539baf8387422bcc2c6b409fcadc4c2043aa2901
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
e0442782c8 fix(script): account for data pushing opcodes in `is_standard_op_return` (Rob N)
Pull request description:
While I believe the original commit used 80 bytes for the entire script as the limit, Bitcoin Core as of [this commit](1101837461/src/policy/policy.h (L68-L72)) will relay OP_RETURN outputs as long as the data itself is not above 80 bytes, meaning a script of maximum size 83 bytes should be standard.
ACKs for top commit:
tcharding:
ACK e0442782c8
apoelstra:
ACK e0442782c8c04bdc48c2743e70f46e41c507e5f4; successfully ran local tests
Tree-SHA512: edd627806c9b5c85050594d45fde49ce3945b9358ae67b4bd0e22f7feb855fe5f4d90f1770870716decbc70aa1a3e0758902c74e3c010977731b823fd72ec23b
In commit:
`a10d5e15b3 Extract the Script assembly creator from fmt::Debug`
A test case was refactored and where there used to be two test case, one
for `Debug` and one for `Display`, two identical test cases were left.
Remove duplicate test case.
9aebb96fb9 Fix psbt fuzz crash (Sanket Kanjalkar)
Pull request description:
Fixes: https://github.com/rust-bitcoin/rust-bitcoin/issues/3628
This occurs when combining two PSBTs with different xpub key sources. Added a length check before indexing into slices to prevent out-of-bounds access.
For some reason, the precommit hook complained about non-ascii files. I don't think any of the names here are non-ascii
ACKs for top commit:
apoelstra:
ACK 9aebb96fb99e8e9e019663659c6eff851a62f2ce; successfully ran local tests; thanks!
tcharding:
ACK 9aebb96fb9
Tree-SHA512: b61274c594bc1f2ea4d04c8a7ace673a7632bb9ea31f59b56779a008c35e61281ea4f6b869990d886779e3e556932a3b2e8b015733ef18e236f12ca77e211c26
While I believe the original commit used 80 bytes for the entire script as the limit,
Bitcoin Core as of [this commit](7a172c76d2/src/policy/policy.h)
will relay OP_RETURN outputs as long as the data itself is not above 80 bytes, meaning a script of maximum size 83 bytes should be standard.
Bitcoin Core's consensus rules allow this timespan interval to be
negative. This commit also updates Params::pow_target_timespan to be a
u32. This field is almost compared to u64 and i64s, so changing this to
a u32 will allow users to use `.into()` for converstions
This function calls helper function predict_weight_internal and that
function was refactored. The refactor changed the interface to accept
the input_weight instead of partial input_weight, however this function
was not updated with the refactor and was still passing a partial input
weight. This commit is a followup that fixes the internal calculation
error by pass input_weight instead of partial_weight to the helper
function predict_weight_internal.
261c8d8ae6 Add total_weight to Input Weight Prediction (yancy)
cc4c36e8ac Deprecate weight (yancy)
Pull request description:
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3645
I'm not sure if `total_weight` is the most accurate. Please advise if there's a different name that is better.
ACKs for top commit:
apoelstra:
ACK 261c8d8ae659d98fd3f63228f16d8f2eb4010b7e; successfully ran local tests
Tree-SHA512: f7fbcc587d3b72edf4add8a7bfecf99ee8133f679bd905c2ab6eeb959db455905c7e32ae91c95571599f09df605454f1e9953f1a47d1f1ee6d66270784c77793
d9ccb60a2f Fix typo in hash tag type (Tobin C. Harding)
Pull request description:
In commit: `39f7dcb Reduce API surface of tagged wrapped hash types` I introduced a typo in the tagged hash tag type. Fortunately it has no effect because the tag type controls how an engine is created (when the tag is input into the engine) and has no effect on the call to `from_engine`. However the typo makes the code confusing.
This bug is currently unreleased.
Fix: #3649
ACKs for top commit:
apoelstra:
ACK d9ccb60a2f9e89c8bb1979f032e8a222ab9b025a; successfully ran local tests
Tree-SHA512: 54cc78fb74ea119beaf4500d49a4d3954ffc3c52494b3ebc1539a3eec35078cb9388e72b42c59f371b53cced89b7b6303a0e3d3b02e59d72da182dd7bbfe2a55
`rust-bitcoin` is a lot further from 1.0 than `bech32` is. We previously
removed the re-export of `bech32` because we thought it might hold us up
releasing `rust-bitcoin 1.0` but in hindsite this was incorrect.
Having a public re-export of dependencies helps users keep their deps in
sync with those in `rust-bitcoin` and is thus useful.
Close: #3650
In commit: `39f7dcb Reduce API surface of tagged wrapped hash types` I
introduced a typo in the tagged hash tag type. Fortunately it has no
effect because the tag type controls how an engine is created (when the
tag is input into the engine) and has no effect on the call to
`from_engine`. However the typo makes the code confusing.
This bug is currently unreleased.
Fix: #3649
073ff81536 address: Simplify `Address::assume_checked` impl (Max Fang)
e4cf8ebc20 address: Add `Address::into_unchecked` (Max Fang)
Pull request description:
## Commits
### address: Add `Address::into_unchecked`
Adds an ergonomic way to convert any `Address` (network can be checked
or unchecked) into an `Address<NetworkUnchecked>` without cloning, which
I've found useful in several contexts.
### address: Simplify `Address::assume_checked` impl
Removes an unnecessary `match` which I noticed while implementing `Address::into_unchecked`.
## Small note on use of `Self`
The style guide in `CONTRIBUTING.md` notes to return `Self` when possible, but that doesn't apply here as the `Address` being returned is different from the `Address` type referred to by `Self`, due to the changed `NetworkValidation` type.
ACKs for top commit:
tcharding:
ACK 073ff81536
apoelstra:
ACK 073ff81536e7a24883d6470ecf3054f4b7263186; successfully ran local tests; nice!
Tree-SHA512: dd6749cffad75d88568106169032d8af023ed56df4b1b38fa613ae7b140d1eb22933e7b0ffc1b17681a5db4b11b28d9db4efb62868755ca3c0f7cac20de0be4b
a8379bf005 Mark `checked_` functions const in bitcoin. (Jamil Lambert, PhD)
fe10ff2eb7 Mark functions `const` in `units` (Jamil Lambert, PhD)
Pull request description:
Following on #3608, #3627 and #1174 the rest of the `checked_` functions in all `rust-bitcoin` have been marked as const. Except for tests and traits.
ACKs for top commit:
apoelstra:
ACK a8379bf0053e66cf5984ce449d19e54e529b6b70; successfully ran local tests; thanks! I find this much easier to read
tcharding:
ACK a8379bf005
Tree-SHA512: b8d97b7a3d9fc33b57349f418ccc5aac2f3e8c4145a73bf0e24e85547217d41214696e9a80b1fb5a1bae7e766aad1c2b5df6044564772fe76cec9b3628bcd8e5
Adds an ergonomic way to convert any `Address` (network can be checked
or unchecked) into an `Address<NetworkUnchecked>` without cloning, which
I've found useful in several contexts.
ad82ed7179 Fix spelling typo (yancy)
8fe5ffde4c Rename tests that have _test suffix (yancy)
Pull request description:
Convention is to not include test as a suffix
Used `git grep -n _test` and renamed all the tests that made sense to rename. There are a number of fn `do_test(data: &[u8]) {` that I felt it would be better not to touch. Everything else that was a testname I renamed if it had a _test suffix.
ACKs for top commit:
apoelstra:
ACK ad82ed71796c79399cd8a81814b1af61c4ca3582; successfully ran local tests
tcharding:
ACK ad82ed7179
Tree-SHA512: 9c4ee71974e39814a8a63d269e3dcf6e761312dd0903ac1e6268dc421b9ef89a63f27cade3d5a51436660bb01457ac1a2f23a628a4d11622cd4a33fa6c483934
ec06028f63 hashes: Make hex dependency optional (Tobin C. Harding)
9dce0b4b8c Remove hex string trait bounds from GeneralHash (Tobin C. Harding)
766f498b33 Pull serde stuff out of impl_bytelike_traits macro (Tobin C. Harding)
Pull request description:
This is done in 3 parts:
1. Pull the `serde` stuff out of `impl_bytelike_traits` to fix the bug described here: https://github.com/rust-bitcoin/rust-bitcoin/issues/2654#issuecomment-2470716693
2. Prepare the `hashes` trait by removing string/hex trait bounds from `GeneralHash` and also pull the hex/string stuff out of `impl_bytelike_traits`
3. Make hex optional, including adding custom debug logic when `hex` feature is not enabled
Patch 3 is tested in `hashes/embedded`, by the new `debug` unit test, and there is a `Midstate` unit test as well that covers the `Debug` impl.
Close: #2654 - BOOM!
ACKs for top commit:
apoelstra:
ACK ec06028f63ba591a14c3a15cdfd410bb5ff1c09b; successfully ran local tests; nice!
Tree-SHA512: 85eb10d36a4581af6cd700f7ff876585bcc114c60e9864906e65659f3b3ee550fe6d9f40ca4230d870a9e23f0720723e11443ec329f16e40259a259b9be57466
On the way re-design the API by doing:
- Introduce `Checked` and `Unchecked` tags
- Rename the `txdata` field to `transactions`
- Make the `Block` fields private
- Add getters for `header` and `transactions` fields
- Move the various `compute_` methods to be free standing functions
- Make the `check_` functions private
- Introduce extension traits
In preparation for adding a block extension trait move the only private
method off the `Block` type and make it free standing.
While we are at it just take a slice of transactions since we don't need
the block itself - rename appropriately.
Internal change only.
For the `hashes` crate we would like to make `hex` an optional
dependency. In preparation for doing so do the following:
- Remove the trait bounds from `GeneralHash`
- Split the hex/string stuff out of `impl_bytelike_traits` into a
separate macro.
The `impl_bytelike_traits` macro is public and it is used in the
`hash_newtype` macro, also public.
Currently if a user calls the `hash_newtype` macro in a crate that
depends on `hashes` without the `serde` feature enabled and with no
`serde` dependency everything works. However if the user then adds a
dependency that happens to enable the `serde` feature in `hashes` their
build will blow up because `serde` code will start getting called from
the original crate's call to `hash_newtype`.
Pull the serde stuff out of `hash_newtype` and provide a macro to
implement it `impl_serde_for_newtype`.
Add a unit test that verifies we correctly remove the annex when getting
the tapscript from the witness stack.
Unit test written by Casey, pulled out of #3599.
Co-developed-by: Casey Rodarmor <casey@rodarmor.com>
In #2646 we introduced a bug in the taproot witness stack getter
functions, of which we have three:
- `tapscript`
- `taproot_control_block`
- `taproot_annex`
Each returns `Some` if a possible bytes slice is found (with no other
guarantees).
Use `taproot_annex` combined with getters from `primitives` to implement
the other two getters. This simplifies the code and fixes the bug.
Add an additional getter to `primitives` `Witness::third_from_last`.
Fix: #3598
These re-exports are hard to get right. We (I) recently made an attempt
to make our stack of crates easier to navigate using the idea that
exporting a type from the crate it is defined in adds some additional
information without any loss of clarity. Note however that the module
must be re-exported from the "highest" place possible because we at
times add additional functionality as we move up the stack e.g.,
`bitcoin::merkle_tree` has logic in it that `primitives::merkle_tree`
does not. In order to future proof the codebase default to always using
the highest module up the stack even when that module adds no additional
code e.g., re-export `blockdata::fee_rate` as opposed to
`units::fee_rate` in the event that we later add logic to
`bitcoin::blockdata::fee_rate`.
This patch adds an additional re-export: `sequence` (previously missing).
8098d5ee05 Reword `Address` constructor docs (Jamil Lambert, PhD)
Pull request description:
Change the wording of the Address constructor function docs to be a standard format. Following up on [#3584 comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/3584#issuecomment-2457197066)
ACKs for top commit:
apoelstra:
ACK 8098d5ee055d672fc8f6e4eef3971b66478a61b0; successfully ran local tests
tcharding:
ACK 8098d5ee05
Tree-SHA512: 1027f334d69182465b2ecafd6fcf9f5deb2b299a64d12dea7f09f25379bd98b1e11be52a04fea83c2194164fd428931f6416eee52603f4bc043717b4e4d1b193
3523b8e117 Rename generic parameter and variable (yancy)
Pull request description:
Use W for Write
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3590
ACKs for top commit:
tcharding:
ACK 3523b8e117
apoelstra:
ACK 3523b8e1171663fc93ab25765271a1070a2d649f; successfully ran local tests
Tree-SHA512: 8dd9f38ee07e7652ebd291eb121a2630ebb09ea9ea70873f87e57241f51e5114106517511d1fe07774ec4f42920e9d0619031a1545787631a30422de12421a67
c61df05916 Fix bip34 number parsing for Block (junderw)
Pull request description:
Fixes#3583
ACKs for top commit:
apoelstra:
ACK c61df059160d3aeef73046de28cb5d5afcb962d2; successfully ran local tests
tcharding:
ACK c61df05916
Tree-SHA512: 3fade122f98c7c9e3b2888c33479d3a9426fb35f212ccee6b89b7cd5c92d2b58c93e165d5c59eda183c0943809d7e633d929000be92b197988a3ceb0ea37514f
1649b68589 Standardize wording to `constructs a new` (Jamil Lambert, PhD)
27f94d5540 Replace `creates` with `constructs` (Jamil Lambert, PhD)
Pull request description:
As discussed in issue #3575 there are various ways of saying a new object is created.
These have all be standardized to the agreed version.
Close#3575
ACKs for top commit:
apoelstra:
ACK 1649b68589834dfe9d5b63812da3e9f0e5930107; successfully ran local tests
tcharding:
ACK 1649b68589
Tree-SHA512: 0ed9b56819c95f1fc14da1e0fdbbe03c4af2d97a95ea6b56125f72913e8d832db5d2882d713ae139d00614e651f3834a4d72528bdf776231cceb6772bf2f9963
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
Is there any advantage trying to lay out the re-exports to give users an
idea of the crate structure?
We have the explicit aim that users who depend on `bitcoin` do not ever
need to reach directly into `primitives` (or `units`) however it is kind
of nice to know where things come from, saves jumping to multiple files
looking for them (for those of us that jump to files manually).
I do not know how all the re-exports interact with other folks IDEs, I
personally open files manually and just remember where stuff is.
85942c355d Re-export block::Header as BlockHeader (Tobin C. Harding)
Pull request description:
For users who want to just grab stuff from the crate root it makes total sense for there to be a `BlockHeader`.
Another nice thing, in the HMTL docs it makes BlockHeader be in the struct list right along with `Block`, `BlockHash`, and `BlockHeight`.
Close: #3548
ACKs for top commit:
apoelstra:
ACK 85942c355dfdcf9ddf9a46202f4db56794dcc85d; successfully ran local tests
jamillambert:
ACK 85942c355d
Tree-SHA512: 6fbaf7936062323d31940179ffcbc62951b21f53955a42dca2e28476bda0cebeb041db0e1b4f1ceeac32e43d94fe5476ef055595ae88dc6cfc266d32082bdf4d
4431df18fe script: remove `unsafe` marker from slice-to-script conversions (Andrew Poelstra)
Pull request description:
The length of the slices is not a safety invariant.
Fixes#3531
ACKs for top commit:
tcharding:
ACK 4431df18fe
Tree-SHA512: 85f745058bc5bf68d312f75c53cef570ead986247ca3cb3fc54b5381dd0214e7df55c1fcb99f06e693b0b9478ec6a9785186cac56886f8007ab7b827d903bd01
The length of the slices is not a safety invariant.
Fairly large diff because I had to remove a bunch of `unsafe` blocks
around calls to this function.
Fixes#3531
We have a couple of problems:
1. There are two macros currently for fmt stuff that do similar things,
`arr_newtype_fmt_impl` and `hex_fmt_impl` - the difference is not
immediately obvious, its the way that the byte array is iterated.
2. Our hash types are missing `AsRef<[u8; len]>` and `Borrow<[u8; len]>`.
Introduce a new macro and remove a bunch of other macros. Include
extensive docs but hide the macro from public docs because its not
really for consumers of the library.
The macro requires `$crate::hex` to point to `hex-conservative`.
Note the macro is pretty generic (as in general purpose), `hashes` might
not be the right home for it. Potentially a better place would be in
`hex` itself?
For users who want to just grab stuff from the crate root it makes total
sense for there to be a `BlockHeader`.
Another nice thing, in the HMTL docs it makes BlockHeader be in the
struct list right along with `Block`, `BlockHash`, and `BlockHeight`.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
a51768af3f key: Deprecate to_bytes (Tobin C. Harding)
3af3239ad0 script: Re-order functions (Tobin C. Harding)
db40297f87 script: deprecate to_bytes (Tobin C. Harding)
c5cd0db493 Revert the change to to_bytes (Tobin C. Harding)
dc2ca785d2 Add to_vec and deprecate to_bytes for array types (Tobin C. Harding)
a6b7ab32a8 Move impl_array_newtype to internal_macros (Tobin C. Harding)
Pull request description:
Use `to_vec` and deprecate `to_bytes`, the opposite of what we did in #2585.
For functions that return a `Vec` by first allocating use function name `to_vec`. This explicitly excludes:
- Functions that return an array (`CompressedPublicKey::to_bytes`)
- Functions that consume self and return a `Vec` without allocating (`ScriptBuf::into_bytes`)
See #3025 for discussion and consensus.
Close: #3025
ACKs for top commit:
apoelstra:
ACK a51768af3f3d4c8e138e1ded250800810bedc903; successfully ran local tests
Tree-SHA512: ee932c13ad2e09c2b76a7833b23c859df175aa307f56e673921f3ae8b5d865518c6f999749e3b627594457b3ca33301b777177ada3520cf006acc0f14e5dacf8
0bf1910980 Remove wildcard from script self re-exports (Tobin C. Harding)
397a4b9382 Remove wildcard in push_bytes module (Tobin C. Harding)
Pull request description:
We thought #3436 was done (well I thought it was), turns out I was wrong.
ACKs for top commit:
jamillambert:
ACK 0bf1910980
apoelstra:
ACK 0bf1910980a13005496244ec4d4adf0553afbc73; successfully ran local tests
Tree-SHA512: 131a4aa4a907099790d14cfc2d19305943a2270cf6852c7dd92e35ea69188c9a40797fb22fd2ed8b2fefc2d6380b884401b5e32f521711f0f7b2da610d0e332f
The private_key_debug_is_obfuscated test is removed because it belongs
in the secp256k1 library, not in the bitcoin library. Keeping it here
is redundant and creates unnecessary maintenance.
As we have been doing in other places deprecate `to_bytes` in favour of
`to_vec`. Note this is only for functions that return a `Vec`, the `key`
module has `CompressedKey::to_bytes` still as it returns an array.
Deprecate the `Script::to_bytes` function in favour of `to_vec` as we
are doing elsewhere.
Note that `ScriptBuf` has `into_bytes` because it does not copy.
Potentially this should be deprecated in favour of `into_vec`?
Note that in regards to the `to_` prefix this naming as valid according
to convention because the `Script` type is borrowed and `to_vec` copies
the underlying bytes.
During this release cycle we deprecated `to_vec` in favour of
`to_bytes`, we have since reversed our position on the name.
Remove the deprecation of `to_bytes` from the three types that had it
and use `to_vec`.
For the array wrapper types (eg `ShortId`) created with the
`impl_array_newtype` macro deprecated the `to_bytes` function and add a
`to_vec` function.
Discussed and decided upon in issue: #3025
dfb76c1e15 Use doc(notable_trait) (Tobin C. Harding)
Pull request description:
There is an unstable feature that puts up a little 'i' in a circle next to any function that returns a type that implements an notable trait. For us this means we can make the extension traits more discoverable.
ref: https://doc.rust-lang.org/unstable-book/language-features/doc-notable-trait.htmlClose: #3232
ACKs for top commit:
apoelstra:
ACK dfb76c1e15933fe0058c1bb69c4b1b9acddceee8; successfully ran local tests
Tree-SHA512: 0fbc7a2a3c8c499a9276d1e86b9966a7ae6bd8a354aff5fd40aa11d07945db589b2a9c2cdfa43ddadfafcee706ae7f68cedc269f74622643307cc43cd07d554f
In the `script` module remove the wildcards and re-export stuff from
`self` explicitly in both `primitives` and `bitcoin`.
Internal change only, everything is re-exported.
The current feature gating is wrong, this bug is unreleased because it
was introduced #2585.
The `impl_array_newtype` macro is only used in the `bitcoin` crate, it
does not need to be in `internals`. Also, other crates have an `alloc`
feature which `bitcoin` does not have so if we ever need it in other
places we'll need a duplicate with the correct feature gating anyways.
Move the macro to `bitcoin::internal_macros` and remove the incorrect
`alloc` feature gating.
WARNING: This is not like all the other extension traits.
Because of the use of generics on various `Transaction` methods it is
not easily possible to use the `define_extension_trait` macro.
Manually create the extension traits (public and private) for the
`Transaction` type. This is quite ugly but c'est la vie
(Includes two in the `transaction` module and one in the
`consensus_validation` module.)
Currently `script_pubkey_lens` returns a generic `Iterator` using `impl`
syntax. This syntax is not supported in traits and we want to move the
function to the soon-to-be-added `TransactionExt` trait.
Add a struct to hold the iterator returned by `Map`, this is ugly but
its the least ugly thing I could come up with.
Split the `Transaction` impl block into three parts:
- The bits going to `primitives`
- The bits staying in a public extension trait
- The bits staying in a private extension trait
Internal change only.
It is acceptable to use a wildcard import in bench code for the same
reasons it is acceptable in the `tests` module.
In preparation for introducing extension traits in the `transaction`
module use wildcard import in the module's bench code.
c89b816437 psbt: Fix bug in Subtype consensus_encode (Tobin C. Harding)
Pull request description:
In #2906 we switched from using a `u8` for type keys to using a `u64` and encoding as a compact int (inline with the spec). Note that a `u8` encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by `PoprietaryKey::consensus_encode` uses a hard code 1 for the length of the encoding (because of single byte) instead of the variable length for the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
Fix: #3501
ACKs for top commit:
jamillambert:
ACK c89b816437
apoelstra:
ACK c89b8164377123eb20476636f2f5271c6a687406; successfully ran local tests
Tree-SHA512: 1b61b6a9ece197d74038ceedb447fd3ca21db8e2a6a96c9281a99ac232c18c3ca55da8e3f46930401714d3575e9a406a36e4f44929ca963208a5df4be6b46cfb
025a8773bf Use fully qualified path in macro (Tobin C. Harding)
Pull request description:
Using fully qualified paths in macros reduces maintenance burden. We have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
ACKs for top commit:
apoelstra:
ACK 025a8773bf63aacdaca011ef000f41a85a961567; successfully ran local tests; will one-ACK merge
Tree-SHA512: eb5923a48ae4d82499679a58375ef7d2e8ba85c91671e350f7be19f0372750a269f44dd2f05f4a70ed0c7f277b160400eb41ff1d42b90e6057f1344be7e11a89
66da2266e2 Explicitly re-export stuff from crates down the stack (Tobin C. Harding)
Pull request description:
Up until recently we were using wildcard re-exports for types moved to `units` and `primitives`. We have decided against doing so in favour of explicit re-exports.
Audit `units` and `primitives` using `git grep 'pub enum'` (and `struct`) and explicitly re-export all types.
Remove all wildcards except for the re-exports from `opcodes`, there are too many opcodes, explicitly re-exporting them does not aid clarity.
ACKs for top commit:
apoelstra:
ACK 66da2266e26dfe53947c4606e9d18620931e93cf; successfully ran local tests
Tree-SHA512: 74717f8b127e975e3d131aab884bdfe78e699d88b7ee1db7731ad117437d37684285264001cf6b2182eb1e565171167695e00c4b6aef28a3e26b69d9cebfbb74
9a7b1c232b Wrap the bech32 decoding error (Tobin C. Harding)
Pull request description:
In #2381 we attempted to fully encapsulate the `bech32` crate to help with stabalizing `rust-bitcoin` however we failed to notice the `address:ParseError` has a variant that includes `bech32`. Public enums have public variant internals in Rust. Also the `From<bech32::segtiw::DecodeError` makes `bech32` public.
Closes: #3043
ACKs for top commit:
apoelstra:
ACK 9a7b1c232b494dccdce091a46d916cc411a612a1; successfully ran local tests; will one-ACK merge since this is a gazillion years old and obviously right
Tree-SHA512: b5053aa43107aa47da1fe7e7db0f882cfb231b9769a7b67d8c930532c471df191f588bf98f2b00cc76d5a2e9c74e035ee96128da115363ac3952f96a766494ea
Using fully qualified paths in macros reduces maintenance burden. We
have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
In #2906 we switched from using a `u8` for type keys to using a `u64`
and encoding as a compact int (inline with the spec). Note that a `u8`
encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by
`PoprietaryKey::consensus_encode` uses a hard code 1 for the length of
the encoding (because of single byte) instead of the variable length for
the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
We do not want `bech32` to appear in the public API of the `address`
module in case `bech32` does not stabalize before the soon-to-be-created
`address` crates does.
We already had a go at removing it but forgot one error variant - wrap
the variant in a new type with a private inner bech32 error field.
Up until recently we were using wildcard re-exports for types moved to
`units` and `primitives`. We have decided against doing so in favour of
explicit re-exports.
Audit `units` and `primitives` using `git grep 'pub enum'` (and
`struct`) and explicitly re-export all types.
Remove all wildcards except for the re-exports from `opcodes`, there are
too many opcodes, explicitly re-exporting them does not aid clarity.
5633b10f5c Manually implement compute_txid and compute_wtxid (Tobin C. Harding)
Pull request description:
We would like to move the `Transaction` type over to `primitives` including the `compute_txid` and `compute_wtxid` functions however currently the implementations, as expected, use `Encodable`.
Manually implement `Encodable` by hashing all the fields in the correct order.
Note we have unit tests already that check the output string of the txid returned so these act as regression tests for this patch.
ACKs for top commit:
apoelstra:
ACK 5633b10f5c826e0b2ac47dd85f697f12710898d7; successfully ran local tests; nice
Tree-SHA512: 66a955d3d896801cfefe0388aade3a31f22fac5b6da7b996be61f374b93772487c0c203320aaf5165fcef26874564bce375ecb364175b0a01c3008b7ea8db981
We would like to move the `Transaction` type over to `primitives`
including the `compute_txid` and `compute_wtxid` functions however
currently the implementations, as expected, use `Encodable`.
Manually implement `Encodable` by hashing all the fields in the correct
order.
Note we have unit tests already that check the output string of the txid
returned so these act as regression tests for this patch.
In preparation for moving the `TxIn` over to `primitives` make the
private `TxIn::BASE_WEIGHT` associated const into a file-scoped constant
because the other alternative is to make it public.
9c2ac46902 Split up ParseError (Jamil Lambert, PhD)
3d994f7bdb Decode an address string based on prefix (Jamil Lambert, PhD)
Pull request description:
When a decoding error occurs for a bech32 address string the error is discarded and the same address string is attempted to be decoded as base58. This then incorrectly returns a base58 error.
Check the string prefix and decode as bech32 or base58 and return the relevant error. If the prefix is unknown return an `UnknownHrpError`.
Close#3044
ACKs for top commit:
tcharding:
ACK 9c2ac46902
apoelstra:
ACK 9c2ac46902ae2e6f2513ee125ea5c89953ac89a2; successfully ran local tests
Tree-SHA512: 40c94328828af86723e84d4196e8949430fb9a15efd8865c18cb5048fe59b8a2514d97f4809d828353b78c010544a8a6d4589a8c9c7fbd75d9d0ecceb3151e8f
bbffa3db43 Remove the IO error from DecodeError (Tobin C. Harding)
713196be0d Return DeserError from encode::deserialize (Tobin C. Harding)
33566ac58c Split encode::Error into two parts (Tobin C. Harding)
b04142c745 Add encode::Error::MissingData variant (Tobin C. Harding)
5a42ef2850 Do not manually map IO error (Tobin C. Harding)
efd7f9f06c Add error constructor parse_failed_error (Tobin C. Harding)
ebfef3f114 Return generic error as Some (Tobin C. Harding)
a6254212dc Move consensus error code to submodule (Tobin C. Harding)
Pull request description:
The `consensus::deserialize` and `consensus::deserilaize_partial` functions should not return an I/O error. Doing so causes various other error types to include an `io::Error` and the `io::Error` is an annoying type to work with.
This PR is a bunch of steps, and it took me a good while with quite a bit of backtracking to get here. As such you may want to review the final state before looking at each patch.
The `consensus` errors can be further cleaned up but I'd prefer not to spend more time on this unless it has some chance of merging.
ACKs for top commit:
apoelstra:
ACK bbffa3db43802b30d23259c0372f16a877a0ef8b; successfully ran local tests
Tree-SHA512: 522fdd29638a214cb7fcee29dd3b9f5c846f041fba087a56a91b83e6d85f033cbed95f659dc4321cd4596943ff233bdd184cdfbfcc787fe89172bb93aa4ab186
Currently `combine_node_hashes` is an associated function, it is also
private. It is called from within other methods of the `TapNodeHash`.
In preparation for moving the `TapNodeHash` to `primitives` while
leaving all the methods in `bitcoin` in an extension trait; move the
associated function out of `TapNodeHash` and make it a stand alone
private function.
e7d326f071 Seal extension traits (Tobin C. Harding)
Pull request description:
The extension traits are temporary just while we try to stabalize `primitives`, they are not intended to be implemented by downstream.
Seal the extension traits so that downstream crates cannot implement them.
Fix: #3231
ACKs for top commit:
apoelstra:
ACK e7d326f071a368389f087ddb10ee9bbf3552c33a; successfully ran local tests; thanks! I know this is tedious and annoying
Tree-SHA512: 365979aeabb7941b9c8fa526f71aaadae3ab1cdd6a39e992c5eea2c1057b4b7c2b3a846ffd96a7eab47b9ad4e3e4de4fb141c24c62747e5cee45c74f52f9a172
The `DecodeError` (badly named) consensus decodes an object from an
iterator that implements `Read`. The `Read` impl never returns a real IO
error, we use the `io::Error` to temporarily wrap the error returned by
the inner iterator and unwrap it in `IterReader::decode`. As such there
is no reason for the `DecodeError` to hold an `encode::Error`, it can
hold an `encode::ParseError`.
The value of this change is easily seen in the removal of calls to
`unreachable`.
The `consensus::encode::Error` contains an IO error but reading from a
buffer only ever errors for EOF. We converted all instances of EOF to
`MissingData` already so now we can split the IO error apart from the
actual encoding errors variants.
The `io::Error` is troublesome because it contains a bunch of stuff that
never happens when reading from a buffer. However the EOF variant can
occur if the buffer is too short. As an initial step towards reducing
usage of the `io::Error` add a `MissingData` variant to the
`encode::Error` and when converting from an IO error map to
`MissingData` if EOF is encountered.
The `encode::Error::ParseFailed` variant holds an inner string, this is
suboptimal.
In an effort to patch the `encode::Error` while mimizing the diffs
required add a helper function that creates the variant. The benefit is
that later patches that effect this variant will only need to update the
constructor function and not every call site.
Internal change only.
The `consensus` module has a bunch of error types, move them all to a
separate module. Add re-exports so the types are still available at the
same place they were. Make the `error` module private and re-export all
errors from the `consensus` module root.
ParseError is too general and the functions returning it do not have an
error path for all variants.
Split out the Bech32 and Base58 related errors into their own enums.
The extension traits are temporary just while we try to stabalize
`primitives`, they are not intended to be implemented by downstream.
Seal the extension traits so that downstream crates cannot implement
them.
Fix: #3231
c1eccfde25 Move Witness to primitives (Tobin C. Harding)
6ce76cd7c8 Add unstable constructor to Witness (Tobin C. Harding)
Pull request description:
Patch 1 introduces a new policy to the codebase, we use `foo__unstable` for public unstable functions and there are zero semver guarantees if you call these functions.
Patch 2 does the move.
Close#3406
ACKs for top commit:
apoelstra:
ACK c1eccfde25fd4c2b19e7ec6759352b46ac246113; successfully ran local tests
Tree-SHA512: 2388066be2b6bb2cf3d6757c8f6334beeda6115ef1ce7c537955d32aa5e466add5162d0d2adee27f416fe622fc93c4e94bd848326463ee55e08d1c0f4e03719c
When a decoding error occurs for a bech32 address string the error is
discarded and the same address string is attempted to be decoded as
base58. This then incorrectly returns a base58 error.
Check the string prefix and decode as bech32 or base58 and return the
relevant error. If the prefix is unknown return an `UnknownHrpError`.
Move the `Witness` over to `primitives` leaving behind any method that
takes or returns a `Script` or a signature.
Includes addition of a feature gate to unit test.
In preparation for moving the `Witness` to `primitives` we need a way to
construct the `Witness` when decoding. In order to maintain the current
performance and not introduce additional allocations we need to be able
to construct a `Witness` from the content buffer, this leaks the
implementation details of `Witness`.
Add a clearly marked unstable constructor to create a `Witness` from
parts. This introduces the concept of `foo__unstable` function names;
add a section to the README describing unstable functions and semver
guarantees.
ff64fa3c46 Move block::Header to primitives (Tobin C. Harding)
65925117a0 Manually implement block::Header::block_hash (Tobin C. Harding)
b02ab25342 Add regression test for block_hash function (Tobin C. Harding)
5614694b69 block: Remove wildcard re-export (Tobin C. Harding)
Pull request description:
- Patch 1: Trivial preparation.
- Patch 2 and 3: Reduce the API surface in a disappointing way due to the orphan rule.
- Patch 4: Do the move.
ACKs for top commit:
apoelstra:
ACK ff64fa3c46 successfully ran local tests; nice!
Tree-SHA512: 8d93de4a12c9f71f9dfbdc023fd8defb78f7d3da995490af8f83a927c2ca6338dd236bc08691311dc7345183fb6d0be61eedce3e4424d4060be2de934facd60a
88a35c8918 test: add test for new sign fn (Chris Hyunhum Cho)
Pull request description:
Follow up https://github.com/rust-bitcoin/rust-bitcoin/pull/3456.
add test to verify the signature generated from newly added `sign` fn.
ACKs for top commit:
apoelstra:
ACK 88a35c8918f48e72f032b73a233a016a1a0056b6; successfully ran local tests; sorry, testing local CI changes..
tcharding:
ACK 88a35c8918
Tree-SHA512: 8c457fbabf1ff86a9369934564874e78660f706714465731c185da16eff5c88a5e680c4373effbcc273394a5653edde9f559558917fee6e438b79bfee6f63b22
49914c50f4 Remove `allow_unused` flag in serde (Jamil Lambert, PhD)
Pull request description:
In serde `#[allow(unused_variables)]` has been dropped and the unused variables have been "used" when `debug_assertions` is off.
Close#3332
ACKs for top commit:
tcharding:
ACK 49914c50f4
apoelstra:
ACK 49914c50f4 successfully ran local tests
Tree-SHA512: c83703f91abb5ce513f6403375ce3b2cb24d5200272c98f0a5e532fe41dda1aae8a5334ee8868751e156457f1cfe6b4fe08849fb5329bfbb1c12ed18364e0b1c
88b53a471e Unify deprecated note field format (Jamil Lambert, PhD)
Pull request description:
Following the suggestion in Issue #3306 all the deprecated note fields have been changed to be lower case and in the format "use `abc` instead".
ACKs for top commit:
tcharding:
ACK 88b53a471e
apoelstra:
ACK 88b53a471e successfully ran local tests
Tree-SHA512: 5c20eda7140f37ce78eb58dfdf03ecc11a67fcb10f294d860e81eaaee696e3a4209516017a9885cef9bfff1aa69b845534d139578b674933770fa24d59e4275f
8696cb4b07 Fix unused import warning (Jamil Lambert, PhD)
fbc7aa7fd5 Remove unnecessary lifetimes (Jamil Lambert, PhD)
Pull request description:
New lint warnings from a recent nightly toolchain show some explicit lifetimes that can be omitted.
The unnecessary lifetimes have been removed.
An unused import warning has also been fixed by placing the use statement behind the relevant feature gate.
ACKs for top commit:
tcharding:
ACK 8696cb4b07
apoelstra:
ACK 8696cb4b07 successfully ran local tests
Tree-SHA512: 37e14ca9be2e8cfa64498084a7363783c2cca92dfce40a0a3ed9bda98f081c1fa9119e5d49d81684b6968329945c7004b86534412fd2fcd4898b862c859d42c6
2f656f77ba psbt: Use u64 for key type (Tobin C. Harding)
Pull request description:
Currently we use `u8` for key type but it was pointed out that we should be using a `u64` and encoding it as a compact type. The reason our code works now is because the compact type encoding for a `u8` (less than
This breaks the `serde` impl, as shown by changes to the regression tests,
Fix: #2891
ACKs for top commit:
apoelstra:
ACK 2f656f77ba successfully ran local tests
Tree-SHA512: ce5fe46b54cb724a0b0f9f874c037e5b5d344e5d3c380f9cdb3fdb5cbc5e31e4e32c229f5f2f72547823238934f6b0c3640c2c2ce79d98aa57bac509d130cb82
We want to move the `block_hash` function to `primitives` but it uses
`Encodable` which currently lives in `bitcoin`. Just implement it
manually.
We added a regression test already in a previous commit to check that
this is correct.
We use `TBD` in our `deprecated` string and it was discovered that there
is an exception on this string so as not to warn because it is used
internally by the Rust language. However there is a special lint to
enable warnings, lets use it.
Add `#![warn(deprecated_in_future)]` to the coding conventions section
of all crates except `fuzz`.
We had an initial go at this but we didn't do the `Hash` trait method.
In order to do so we need to hack the serde code a fair bit, note the
public visitor types.
Recently we deprecated `to_vec` in favour of `to_bytes` however we
continued to use `to_vec` in a few places. This wasn't noticed because
of our usage of `TBD` in the `deprecated` attribute.
Use `to_bytes` instead of `to_vec`.
d649c06238 Move script types to primitives (Tobin C. Harding)
ec4635904b Inline bytes_to_asm_fmt into Script::Display impl (Tobin C. Harding)
Pull request description:
First patch removes `bytes_to_asm_fmt` as requested by Kix here: https://github.com/rust-bitcoin/rust-bitcoin/pull/3194#discussion_r1756557768
Second patch does the move. The move is minimal but there is quite a bit of code moved in `script/mod.rs` - I believe it is as minimal as required as well.
ACKs for top commit:
apoelstra:
ACK d649c06238 successfully ran local tests
Tree-SHA512: 329a23948ac5617402a724b734d81cde8ab1f57ddd4860f858880618e260ea8b5cc89315de1fd93ae32787d5e8508fd604a41f003b1f5772a773b5b1648d382c
c41a6e9b1b feat: add sign fn for sign_message (ChrisCho-H)
Pull request description:
While it's not hard to create the signature using `secp256k1` modules with `signed_msg_hash`, it's much more convenient and safe to provide one-way function to generate signed signature(even without the understanding about the semantics of bitcoin message signing).
ACKs for top commit:
apoelstra:
ACK c41a6e9b1b successfully ran local tests
tcharding:
ACK c41a6e9b1b
Tree-SHA512: 84caea275059381040c71100badb54556dd105722f79ac43d2df7eb0e5428cf8e7acc2d7f262625dd008837099a928c3c8be858f9ab11838c2eec1786e9f1844
9e6b8faf84 feat: add version three variant to transaction version (Rob N)
Pull request description:
Topologically restricted transactions are now considered standard as of Bitcoin 28.0.
ACKs for top commit:
apoelstra:
ACK 9e6b8faf84 successfully ran local tests
tcharding:
ACK 9e6b8faf84
Tree-SHA512: 289b986304e206802f04cee6607087167f6d91d8a81d4fc49ed01c430f4f6ad00b44646fbefdd000148fc5bfe2d257f92b386bfaf4405c482e4e438d830ab586
a250c8eee4 Fix unused imports (Shing Him Ng)
Pull request description:
Found some unused imports while working on something unrelated
ACKs for top commit:
apoelstra:
ACK a250c8eee4 successfully ran local tests
tcharding:
ACK a250c8eee4
Tree-SHA512: ffa4fcddb91c849df7885a6d734405ab704e0353293fa3462a981e179ea49091990df4482eee2324e7b5ef250a0890b2e8983d53ec288ca6e8ada77e30c102dc
0c824c9c68 Implement Arbitrary for Block (Shing Him Ng)
Pull request description:
Implementing `Arbitrary` for `Block` and its child types
ACKs for top commit:
apoelstra:
ACK 0c824c9c68 successfully ran local tests
tcharding:
ACK 0c824c9c68
Tree-SHA512: 407acd4155ca7496bf7c59af19f103b22b4c74dd013c2e2c42aae690ba7264ecd42ed034355cf8895f2532f8cef77dfdbeac250301a6973afd5f3920be7e4310
323e706113 Add rustfmt config option style_edition (Tobin C. Harding)
2e4179ed0f Run the formatter (Tobin C. Harding)
2c40b4f4ec Configure formmater to skip read_compact_size (Tobin C. Harding)
Pull request description:
`rustfmt` is emitting:
Warning: the `version` option is deprecated. Use `style_edition` instead.
As suggested add a config option and set it to 2021.
- Patch 1: Manually configure rustfmt to skip some code
- Patch 2: Run the formmater with current configuration
- Patch 3: Add the new config option (remove old one), introduces no new formatting requirements
ACKs for top commit:
apoelstra:
ACK 323e706113 successfully ran local tests
Tree-SHA512: 7f80cc89f86d2d50936e51704344955fa00532424c29c0ee3fae1a6836e24030f909b770d28da13e1c5efde3d49ad7d52c6d909d120fb09c33abf1755f62cd38
3b7ba4f977 Remove the SliceIndex implementation from hash types (Tobin C. Harding)
Pull request description:
If folk really want to index into a hash they can us `as_byte_array` then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this is because otherwise `secp` won't build since we are breaking an API that is used in the current release of secp.
Fix: #3115
ACKs for top commit:
apoelstra:
ACK 3b7ba4f977 successfully ran local tests
Tree-SHA512: 0ba93268cd8133fe683183c5e39ab8b3bf25c15bfa5767d2934d67a5f6a0d2f65f6c9304952315fe8a33abfce488d810a8d28400a28facfb658879ed06acca63
70264bfcec Move block::Version to primitives (Tobin C. Harding)
819d8d72e8 Stop using private Version constructor (Tobin C. Harding)
Pull request description:
This is a straight up move of the whole type because there are only three methods, a getter, a setter, and `is_signalling_soft_fork`.
If we use an extension trait for `is_signalling_soft_fork` then we have to make the two private associated consts public which is in my opinion worse.
Patch 1 is preparation, use getter/setter, patch 2 does the move.
ACKs for top commit:
apoelstra:
ACK 70264bfcec successfully ran local tests
Tree-SHA512: b61e853b4b96cb1cc56c7bfb67cc6c3ba7c631cb9e540393eb780dcf63bd2d934058794f2ac0145b4b5404a6a68887c3a1d225b2d87b57415474c05d3ef2811f
Currently we use `u8` for key type but it was pointed out that we should
be using a `u64` and encoding it as a compact type. The reason our code
works now is because the compact type encoding for a `u8` (less than
253) is the same as for a `u8`.
This breaks the `serde` impl, as shown by changes to the regression tests.
aa1fd5f44c Update the doc for InputWeightPrediction::weight() (spacebear)
8fd53c8ecf Add nested P2WPKH helpers to InputWeightPrediction (spacebear)
Pull request description:
Following up on https://github.com/rust-bitcoin/rust-bitcoin/issues/630#issuecomment-2392050517.
I amended the docstring on `InputWeightPrediction::weight()` to clarify that it only returns the signature weight. I'm not sure what the rationale was for only returning partial input weight, and if there are any arguments against something like `InputWeightPrediction::total_weight()` which returns `self.weight() + Weight::from_non_witness_data_size(32 + 4 + 4)` (and maybe deprecate `weight()` eventually to prevent misuse).
ACKs for top commit:
apoelstra:
ACK aa1fd5f44c successfully ran local tests
tcharding:
ACK aa1fd5f44c
Tree-SHA512: 9bdd882164a80a1a1a44c593750aa59e470f2928c0380d4d53a0409add597844cf11945785264ebd6b281029b3f2ef1be6c77e1e9cd148192dbd601df30a60ae
If folk really want to index into a hash they can us `as_byte_array`
then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this
is because otherwise `secp` won't build since we are breaking an API
that is used in the current release of secp.
Fix: #3115
This is a straight up move of the whole type because there are only
three methods, a getter, a setter, and `is_signalling_soft_fork`.
If we use an extension trait for `is_signalling_soft_fork` then we
have to make the two private associated consts public which is in my
opinion worse.
In preparation for moving the `block::Version` type over to `primitives`
stop using the private constructor and inner field. Use the public
getter/setter instead (`to_consensus`and `from_consensus` respectively).
3f3f30d6c7 Use iter instead of accessing content field (Tobin C. Harding)
6389d1cbb3 Stop using push_slice (Tobin C. Harding)
be163eec99 Use Witness::len instead of accessing field (Tobin C. Harding)
Pull request description:
Prepare `Witness` to be moved to `primitives`.
This is the first three patches out of #3406. Patch 1 and 2 are internal changes, path 3 is also internal but introduces a slight perf hit by doing multiple writes.
ACKs for top commit:
apoelstra:
ACK 3f3f30d6c7 successfully ran local tests
Tree-SHA512: 25e2570a22797dbfa15d6e5af9b72938243192ca264bc5fe82c2f327486e52a73993b3b61ee1161e5d17b01a7f9843960cb4031e6550561de4ecafb9f935e375
We would like to move the `Witness` to `primitives` however in the
`Encodable` implementation we are currently accessing the private
`content` field.
Instead of accessing `content` we can iterate over the witness elements
and write each individually, this has the same result but does a bunch
of additional calls to `Write::write_all` (via `emit_slice`).
This patch effects performance negatively but makes no changes to the
encoding.
The `Witness::push_slice` function is called by `Witness::push` after
calling `as_ref`, hence is equivalent for all types that implement
`AsRef<[u8]>`. Also, `push_slice` is a private method on `Witness`.
In preparation for moving `Witness` over to `primitives` stop using
`push_slice` in favour of `push`.
Internal change only.
In preparation for moving the `Witness` oven to `primitives` use the
`len` function instead of accessing the `witness_elements` field.
No logic change, `Witness::len()` returns `witness_elements`.
da0795e590 primitives: Use doc links for OutPoint (Tobin C. Harding)
b079cbafee Move OutPoint to primitives (Tobin C. Harding)
f5c46cd411 Introduce OutPoint extension traits (Tobin C. Harding)
7e5bd5048d Remove docs on deprecated is_null function (Tobin C. Harding)
97b20a2316 Add additional impl block to OutPoint (Tobin C. Harding)
Pull request description:
Just the minimal move of `OutPoint` to `primitives`.
The last patch closes#3347
ACKs for top commit:
apoelstra:
ACK da0795e590 successfully ran local tests
Tree-SHA512: dced5a6d6bc6af0ce8b4ae4e52c04b45c85eb77a24bb25762ba8ab7deeab1e6b392cc4b258bb14218e8a2af999715dfed45ba94599cb16963a843995a7475917
5fab6b178f Rename iter len unit test (Tobin C. Harding)
f6a74ef4af Refactor the serde Witness unit tests (Tobin C. Harding)
9860453b5b Improve Witness consensus encode unit test (Tobin C. Harding)
7e2899d310 Improve Witness::push unit test (Tobin C. Harding)
fe967279e5 Improve witness unit tests for single empty element (Tobin C. Harding)
Pull request description:
In preparation for moving the `Witness` type over to `primitives` refactor and improve all the unit tests that will be moved, do not touch the ones that will stay behind.
The first five patches are from #3406, the last is just a re-name of the test function I tried to refactor in ac6fe3a881
ACKs for top commit:
apoelstra:
ACK 5fab6b178f successfully ran local tests
Tree-SHA512: bc00f81e3c5cc92ae58dd2fc876d368a487ae6c08cc0735d7227c3a89287e321dbfb5b571b951d0616af0ec7cf9a0ea2d0e724645b1c419933a212ece80a0fbf
2d8c613340 Move the block hash types to primitives (Tobin C. Harding)
6b9429ac7b Remove BlockHash::all_zeros (Tobin C. Harding)
20d8dbd586 Add missing line of whitespace (Tobin C. Harding)
Pull request description:
As an initial step in moving the `block` module, just move over the hash types `BlockHash` and `WitnessCommitment`.
Patch 2 introduces an associated const `BlockHash::GENESIS_PREV_BLOCKHASH` and removes `all_zeros`.
ACKs for top commit:
apoelstra:
ACK 2d8c613340 successfully ran local tests
Tree-SHA512: 64aa0ae81e1c8ab1b5d4cd8cd28e6ef04ed01bf79175dc5b1fd607a6f0967e06b0aaf4c10ad368e2b327edcad3705187b6643d5ca8647716319424f19a838ba1
9ded58fc99 Move merkle_tree hash types to primitives (Tobin C. Harding)
Pull request description:
In preparation for moving the `block::Header` struct over to `primitives` move the `merkle_tree` hash types.
ACKs for top commit:
apoelstra:
ACK 9ded58fc99 successfully ran local tests
Tree-SHA512: 98017cf0403871f01a6efeda22e8f319cc8104b9bc2f3a9bae2d6a31f6df8172307466c6486a9259204166933137fa03e565e08a0c156c278cfeb72cdae09b89
18d8b0e469 Replace VarInt type with ReadExt and WriteExt functions (Steven Roose)
003db025c1 Return encoded length from WriteExt::emit_slice (Steven Roose)
Pull request description:
This the meat and potatoes out of Steven's work in #2133 and also closes#1016
ACKs for top commit:
apoelstra:
ACK 18d8b0e469 successfully ran local tests
Tree-SHA512: 2df96c91e0fbfdc87158bde9bbdd9565f67e3f66601697d0e22341416c0cd45dd69d09637393993f350354a44031bead99fd0d2f006b4fc6e7613aedc4b0a832
e215a39dba Improve documentation of Xpub::from_xpriv (Jiri Jakes)
0dcba98382 Add Xpriv::to_xpub (Jiri Jakes)
5a9341bfc5 Improve naming of methods on Xpub and Xpriv (Jiri Jakes)
Pull request description:
Adds `Xpriv::to_xpub` and makes naming of methods related to extended and non-extended keys on `Xpub` and `Xpriv` consistent and easier to discover:
- if method takes or returns extended key, its name uses `xpriv` or `xpub`
- if method takes or returns non-extended key, its name uses `public_key` or `private_key`
Previous naming of the methods was confusing and unclear and this PR deprecates them.
Closes#3327.
### Notes for reviewers
- `xpriv` and `xpub` could be without `x`, I opted for this version to remove any ambiguity
- also considered `raw` or similar name (as suggested) for methods with non-extended keys, however `raw` is usually not used in this context and `to_public_key` vs. `to_x_only_public_key` did not look alright with any other name
- if splitting this PR into two would be desirable, please let me know
- is there a reason why `ckd_priv` is private and `ckd_pub` is public?
ACKs for top commit:
apoelstra:
ACK e215a39dba successfully ran local tests
tcharding:
ACK e215a39dba
Tree-SHA512: f76b22740b06df80dd9d01fbb991149243e47619e0bd5f1699b446456259cc6e97ecb9fca7b4881e921a9d7341ca92c6ee2dae90a417f702aff5eb760439ca42
This change makes method names on Xpub and Xpriv more consistent and
easier to discover by following two patterns:
- if the method deals with extended key, it contains 'xpub' or
'xpriv' in its name
- if the method deals with non-extended key, it contains
'public_key' or 'private_key'
One exception is 'ckd_*' methods, which are lower-level and their names
come from BIP32; these keep using 'priv' and 'pub'.
Move the `OutPoint` type and associated code over to `primitives`. Take
the opportunity to re-order the code so the file reads like a story,
things are implemented below where they are called.
Our `define_extension_trait` macro cannot handle examples in rustdocs,
since these docs are on the deprecated `OutPoint::is_null` function just
remove them.
In preparation for moving the `OutPoint` to `primitives` add an
additional impl block that holds everthing that will stay here in
`bitcoin`.
Internal change only.
Rust convention is to not use `test_` prefix on unit tests. Also this
unit test is testing that the `ExactSizedIterator` trait is implemented
and working.
Re-name unit test to `exact_sized_iterator`.
In preparation for moving unit tests to `primitives` give the serde
tests some love by doing:
- Split them up to do one thing only
- Round trip arbitrary witness
- Use better names
Make an effort to clean up the encoding unit test, by doing:
- Remove element accessor assertions (tested already above)
- Add roundtrip encoding assertion
33edaf935d fix: check overflow for push_int with push_int_unchecked (Chris Hyunhum Cho)
876146154b refactor: use push_lock_time instead of push_int (Chris Hyunhum Cho)
Pull request description:
Fix the issue https://github.com/rust-bitcoin/rust-bitcoin/issues/1530. In the discussion of https://github.com/rust-bitcoin/rust-bitcoin/issues/1530, the suggested solution is to implement `ScriptInt` type to embrace the various type of integer(`i32, u32, i64, u64, i128, u128, isize, usize...`) to support both script number and locktime number.
However, as `push_locktime` and `push_sequence` implemented, there’s no need to support `u32` of lock time for `push_int` anymore. Therefore, I’ve just changed the type of parameter to `i32`, and only check if it’s `i32::MIN`(which overflows 4 bytes sign-magnitude integer).
~I also added push_uint method to use internally for `push_locktime` and `push_sequence`, which have a dependency on `push_int` method.~
UPDATE: also add `push_int_unchecked` for those who want to push the integer out of range(and helper for `push_locktime` and `push_sequence`, which has the same functionality of former `push_int`.
ACKs for top commit:
tcharding:
ACK 33edaf935d
Tree-SHA512: 89b02bd3faf1e0a1ed530b7210250f0db33886d2acd553d07761f4aef0bb6388b22ddc06a88de05acfe465305db4cf34822fb6547576aae2aa224b4d0045fa07
The current unit test is incorrect, the indices field of a witness with
a single element starts at 1 because 0 is encode as a single
byte (compact encoded integer).
Fix the debug test and add a test that pushes an empty slice.
Recently we removed the `all_zeros` function from `OutPoint` in favour
of a more meaningfully named associated const. We can do the same for
`BlockHash`, the all zeros has is used for the previous blockhash of the
genesis block, add a const named as such.
In test code where we use the `all_zeros` function, just use the more
explicit form `from_byte_array([0; 32])`.
Currently we provide `Default` implementations for a couple of types in
the `transaction` module, the values returned are meaningless and it
seems these impls were added to make writing test code easier. In
hindsight this was the wrong thing to do.
Break the API and remove the `Default` implementations for `OutPoint`
and `TxIn`.
Add an associated const `TxIn::EMPTY_COINBASE` that is, as the name
suggests, an empty transaction input with the prevout set to all
zeros as for the coinbase transaction.
At some stage we named the compact encoding `VarInt` (which makes sense
because the compact size encoding is a variable length integer encoding).
However it turns out the term "varint" is used in Core for a different
encoding so this may lead to confusion.
While we fix this naming thing observe also that the `VarInt` type is
unnecessarily complicated, all we need to be able to do is encode and
decode integers in compact form as specified by Core. We can do this
simply by extending our `WriteExt` and `ReadExt` traits.
Add `emit_compact_size` and `read_compact_size` to emit and read compact
endcodings respectively.
Includes addition of `internals::compact_size::encoded_size_const`.
Patch originally written by Steven, Tobin cherry-picked and did a bunch
of impovements after the varint vs compact_size thing (#1016).
ref: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
Co-developed-by: Tobin C. Harding <me@tobin.cc>
We would like to add a `emit_varint` function, however doing so requires
that we can get access to the length of a slice when we are encoding it
so we can use `emit_slice` to implement `emit_varint`. It would be
easier to do so if `emit_slice` returned the length of the slice.
In preparation for adding `emit_varint` (and removing the `VarInt` type)
return the encoded length of a slice from `WriteExt::emit_slice`.
(Patch originally written by Steven, cherry-pick and patch description
written by Tobin.)
Co-developed-by: Tobin C. Harding <me@tobin.cc>
46386337b0 Ignore doc compile error (Jamil Lambert, PhD)
Pull request description:
With the stricter doc tests required to pick up unused imports etc. the code under the `compile_fail` tag also creates an Error.
Changed `compile_fail` to `ignore` to remove the Error.
ACKs for top commit:
tcharding:
ACK 46386337b0
apoelstra:
ACK 46386337b0 successfully ran local tests
Tree-SHA512: 67513464dede8d4bd9f9fa4363ccb58e774b180f331edc4486290784f61e931195225765768030d2709f618ba4afe17d034d4bbc4d649a02e7a34b47b48bd297
f6abdcc001 Allow unused in `macros.rs` docs (Jamil Lambert, PhD)
fd89ddf401 Remove or fix unused variables and methods in docs (Jamil Lambert, PhD)
ff6b1d4f19 Remove unused variables and methods from docs (Jamil Lambert, PhD)
e58cda6f92 Remove `unused_imports` in docs (Jamil Lambert, PhD)
Pull request description:
As mentioned in #3362 examples in documentation are not linted in the same way as other code, but should still contain correctly written code.
#![doc(test(attr(warn(unused))))] has been added to all lib.rs files
In the docs throughout all crates:
- Unused imports have been removed.
- Unused variables, structs and enums have been used e.g. with an `assert_eq!` or prefixed with `_`
- Unused methods have been called in the example code.
ACKs for top commit:
tcharding:
ACK f6abdcc001
apoelstra:
ACK f6abdcc001 successfully ran local tests
Tree-SHA512: c3de1775ecde6971056e9fed2c9fa1621785787a6a6ccbf3a6dbd11e18d42d4956949f3f8adfc75d94fd25db998b04adb1c346cc2c2ba47f4dc37402e1388277
With the stricter doc tests required to pick up unused imports etc. the
`compile_fail` tag also creates an Error.
Changed `compile_fail` to `ignore` to remove the Error.
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout the bitcoin crate unused variables have either been prefixed
with _ or an assert used. And unused methods have been used in the
example code.
5b4e81b379 Implement Arbitrary for Transaction (Shing Him Ng)
Pull request description:
Implement `Arbitrary` for Transaction and its child types
ACKs for top commit:
apoelstra:
ACK 5b4e81b379 successfully ran local tests
tcharding:
ACK 5b4e81b379
Tree-SHA512: 411b97344e01e2bf38fff994902acda51f9a4fe748573e600fbdd643951a0a56b37395594349706b13c744304bd5396fd8ee31c27e5b1ff97e1a3cebd479e7c8
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
unused_imports in docs have been removed in bitcoin, and a warn
attribute added to lib.rs.
25d906d936 Use UFCS in macros (Liam Aharon)
Pull request description:
Closes#3304
ACKs for top commit:
apoelstra:
ACK 25d906d936 successfully ran local tests
tcharding:
ACK 25d906d936
Tree-SHA512: a02a8507bcec73dab1ec8d49e45eab327d989c276d931520e0fff312faf7d3165292c300fb9414314f9b94b1255ee49a0fb64303df6d971c9089226b6873c36a
8ec3571d80 Implement GetKey for Vec<Xpriv> (Nadav Ivgi)
Pull request description:
It appears that the `BTreeSet<Xpriv>`/`HashSet<Xpriv>` sets currently implementing `GetKey` cannot actually be constructed, because `Xpriv` does not implement `Ord` nor `Hash`. (And that the rust-bitcoin code referencing these sets should not even compile? yet evidently it does 👀 )
This PR adds support for `Vec<Xpriv>` to enable signing with multiple `Xpriv`s, but does not address the issue with the existing sets.
The added test case demonstrates the issue:
```rust
error[E0277]: the trait bound `bip32::Xpriv: std:#️⃣:Hash` is not satisfied
--> bitcoin/src/psbt/mod.rs:2301:24
|
2301 | HashSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `std:#️⃣:Hash` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::HashSet::<T, S>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/collections/hash/set.rs:888:5
error[E0277]: the trait bound `bip32::Xpriv: Ord` is not satisfied
--> bitcoin/src/psbt/mod.rs:2302:25
|
2302 | BTreeSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `Ord` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::BTreeSet::<T, A>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/collections/btree/set.rs:899:5
```
ACKs for top commit:
apoelstra:
ACK 8ec3571d80 successfully ran local tests
tcharding:
ACK 8ec3571d80
Tree-SHA512: aceb95f8eaf11f91c6829e0b5e1c0264ebffbf587fd420145a22e924cb45678b2f4334f0b7de6ed99b57f0ce24c3d61f9e5c1e348e1b40975bc515e8fd16b75d
d942882b36 Document the magic bytes for witness commitment (Peter Ryszkiewicz)
Pull request description:
ACKs for top commit:
apoelstra:
ACK d942882b36 successfully ran local tests; neat! I don't think I was aware of this
tcharding:
ACK d942882b36
Tree-SHA512: 89766e986574e49b6c5fd00844db679eb8894a591463029f7f88079fa9ca65bd56265a42dd8fa5aad20ef4744bca3259c06f8a461abd1c695fb89433ed5cc145
b593c886e3 Support GetKey where the Xpriv is a direct child of the looked up KeySource (Nadav Ivgi)
055aa9d4dc Refactor GetKey to take the KeyRequest by reference (Nadav Ivgi)
d15c57bd1f Refactor GetKey for sets to internally use Xpriv::get_key() (Nadav Ivgi)
d25c62bf45 Fix GetKey for sets to properly compare the fingerprint (Nadav Ivgi)
Pull request description:
- The first commit is the simplest fix for a bug where the fingerprint wasn't compared correctly.
- The second & third commits are optional refactoring to reuse `Xpriv::get_key` for `$set<Xpriv>::get_key`, so the Xpriv matching logic only has to be maintained in one place.
- The forth commit adds support for signing with `Xpriv`s that are direct children of the `KeySource` -- possibly what the original (buggy) code author had in mind?
Of course, feel free to take just the first commit if the others seem unnecessary. The last one is kind of meh, not sure if really useful.
Note that multi-`Xpriv` signing does not actually work until #2850 is addressed too.
ACKs for top commit:
apoelstra:
ACK b593c886e3 successfully ran local tests
tcharding:
ACK b593c886e3
Tree-SHA512: a6f84fe12b68ddb71fc722649b1596d1561ab08a07d47f2345bd8932e155571f2b1e23c32a9c03d588fb697540340cb8ca38275dfe11f4910fd48265a993762a
70ccd6b629 signed_msg_hash takes impl AsRef<[u8]> (Liam Aharon)
Pull request description:
Closes#3249
Seems not required for Kixunil's immediate use case anymore, but figured I'd open a PR anyways.
ACKs for top commit:
apoelstra:
ACK 70ccd6b629 successfully ran local tests
Kixunil:
ACK 70ccd6b629
Tree-SHA512: b15bafd1af0749cbac36652a4e4c9ab3f91b3980bb6c3cbc1c977d339810fc15075af5d4bc6b210de364bedc496ce3759eaf4539e40d215ab66a122ba69dd56a
8f2f4cbb3c Re-order optional dependencies (Tobin C. Harding)
95f2a8dab6 Do not access ScriptBuf inner from builder (Tobin C. Harding)
900af453ff Stop accessing inner ScriptBuf field when encoding (Tobin C. Harding)
8b82363d97 Use Script::as_bytes instead of inner when indexing (Tobin C. Harding)
b0675a4a4f Use Script::len instead of inner field (Tobin C. Harding)
374c6118dc Deprecate Script::fmt_asm and to_asm_str (Tobin C. Harding)
Pull request description:
Move the `Script` and `ScriptBuf` types to `primitives`. There were still a few preparations required, things we had missed while creating the extension traits.
Note also please, in the last patch, we enable `hex` from the `serder` feature. This is not the final state we want but like we did for `alloc` it is a step to reduce the size of the diff.
ACKs for top commit:
Kixunil:
ACK 8f2f4cbb3c
apoelstra:
ACK 8f2f4cbb3c successfully ran local tests
Tree-SHA512: 62a5f3c253ecb54d95c37fdc7eb955f3952909dc3bca20444b85c44665f54d5a0c48daf729bed0dd60ff3e9571b41deed039984c8b757b075ac6e136cacd17d7
0403e52ce3 Move the transaction hash types over to primitives (Tobin C. Harding)
7e454d756d Define extension traits for txid types (Tobin C. Harding)
832b726d03 Stop using all_zeros (Tobin C. Harding)
d69c241b5c Improve docs on associated consts (Tobin C. Harding)
68c9e28165 Do not use private constructor for txid hash types (Tobin C. Harding)
98328b5a7b Use as_byte_array to encode hash type (Tobin C. Harding)
Pull request description:
Move the `Txid` and `Wtxid` hash wrapper types over to `primitives`. This introduces to `primitves` an unconditional dependency on `hashes`.
ACKs for top commit:
Kixunil:
ACK 0403e52ce3
apoelstra:
ACK 0403e52ce3 successfully ran local tests
Tree-SHA512: d14fa95bc12c2399d30d4d640b5a3fce625d51adf587a8037158f7d7e7b6288170b2d4418ca2cb68f612086ea0bdd0fae3b577f84f0d60627072fdb2217a6531
e064686397 Deprecate OutPoint::new constructor (Tobin C. Harding)
Pull request description:
The `OutPoint` type has two public fields, providing a `new` constructor that just sets these two fields adds no value.
Done after discussion in #3340 as part of `primitives` work.
ACKs for top commit:
Kixunil:
ACK e064686397
apoelstra:
ACK e064686397 successfully ran local tests
Tree-SHA512: d80cac85093946b3678883f4e3ad7fd2052d858dbd3fab4127916b0eb6153999d30827c84ccf8c6e4412fbc3842bd127a2e4d3ca0248d2307d0bca467a2555ce
f811e0adb6 Stop using deprecated OutPoint functions (Tobin C. Harding)
Pull request description:
Either our CI is failing us or `deprecated` does not work as expected, either way we should not be using the `OutPoint::null()` or `is_null` functions any more because we deprecated them already.
ACKs for top commit:
Kixunil:
ACK f811e0adb6
apoelstra:
ACK f811e0adb6 successfully ran local tests
Tree-SHA512: 64e75601ce8062da78bbd9aea97792e35c728aa31255348413fe9b57ee585974370d3f59b5467d7e4a65dc05d2718aea18bb381e128c8410fbfde4a851006416
The `Builder` is staying in `bitcoin` while the `ScriptBuf` is moving to
`primitives`, so we cannot access the inner field of `ScriptBuf`.
Use the new `as_byte_vec` hack to mutate the inner `ScriptBuf` field.
Martin Habovstiak
merge-script
Jose Storopoli
jrakibi
Tobin C. Harding
Fmt Bot
ndungudedan
Andrew Toth
yancy
yancy
Peter Todd
Jamil Lambert, PhD
Innocent Onyemaenu
Nick Johnson
Shing Him Ng
Jesus Christ
calciumbe
Chris Hyunhum Cho
Rob N
Sanket Kanjalkar
Max Fang
LLFourn
junderw
Diogo Canut
Leo Nash
Casey Rodarmor
BinChengZhao
spacebear
Jiri Jakes
Steven Roose
Peter Ryszkiewicz
Nadav Ivgi
Liam Aharon