We just re-named `Timestamp` to `BlockTime`. We have a `units::block`
module but it currently holds abstractions (`BlockHeight` and
`BlockInterval`) that are not onchain abstractions and therefore
somewhat different from the `BlockTime`. Instead of making `block` a
block 'utils' module instead re-name the `timestamp` module to `time`.
We just added a `Timestamp` type without knowing that there was a push
by OpenTimestamps to also create a timestamp and that our new type may
lead to confusion. Our timestamp is explicitly for the `time` field in a
block so we can call it `BlockTime`. This name change makes the module
name stale but we will change that in a following patch to ease review.
95ad91cdb6 hashes: Remove the GeneralHash trait (Tobin C. Harding)
6426e59c63 Remove unused trait import (Tobin C. Harding)
791501eabc io: Use function in place of GeneralHashExt (Tobin C. Harding)
2b6ef31469 hashes: Add hash_byte_chunks function to modules (Tobin C. Harding)
d3846895d7 hashes: Add hash function to modules (Tobin C. Harding)
e1bac7da55 Bound HmacEngine on HashEngine (Tobin C. Harding)
ab63b7a0ff Add Hash type and finalize method to HashEngine (Tobin C. Harding)
84623ffaf9 Add hash_again regression test (Tobin C. Harding)
Pull request description:
This is the done as part of #4051.
Requires some surgery on the `Hmac` and `Hkdf` types as well as a few other patches to maintain the logic that is currently provided by the trait. Final patch is a pure red diff - enjoy.
ACKs for top commit:
Kixunil:
ACK 95ad91cdb6
apoelstra:
ACK 95ad91cdb64c8870d3eb992090bab7a70d1369b9; successfully ran local tests
Tree-SHA512: bfd215de51c115144c6f5b3430b17dad5d770a8c876fe3775af2828ec620a1f4e4155e63bb18dac244a82c3840413a615d55a0fef91b6949d3de319aa5bb8c2f
When we added the `fee_rate::serde` module we forgot to re-export it.
This is needed so downstream can do specify serde attributes on struct
fields.
```rust
#[serde(with = "bitcoin::fee_rate::serde::as_sat_per_kwu")]
rate: FeeRate,
```
5da506b506 Add additional re-exports (Tobin C. Harding)
Pull request description:
As we do for other types add two new alias' at the crate root of `primitives` and mirror it in `bitcoin`:
- `BlockVersion`
- `TransactionVersion`
ACKs for top commit:
apoelstra:
ACK 5da506b506aa41b88aa7e8ecdffdcc0478ec72b6; successfully ran local tests
Kixunil:
ACK 5da506b506
Tree-SHA512: 5f91e3aae87b1128256b528d20d9aab562bf054131ed8028e0db789e2b487863ad4c89aaca080d0fbcf760dd160815a1843046c5fab0ed1483230fdedc66402e
b3f122b399 Add Timestamp newtype (Tobin C. Harding)
Pull request description:
Bitcoin block headers have a timestamp. Currently we are using a `u32`. While this functions correctly it gives the compiler no chance to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`. Document it and test the API surface in `api.rs`.
ACKs for top commit:
apoelstra:
ACK b3f122b3996c1a73479be2f95b7f2ae642c9c56f; successfully ran local tests
Kixunil:
ACK b3f122b399
Tree-SHA512: 6f4a4a588bc836243ae28f3d36be6c0ae264cb2b7a0061277910b107d05e5ca0e679497d2890208f5d8ec148f37bf263bcd0b0410f9e5e6370d8e763ff30b78a
Pull request description:
Enhance Witness struct element access methods:
- Rename `nth()` to `get()` for clearer slice-like element retrieval
- Introduce `get_back()` method for flexible reverse indexing
- Remove redundant `second_to_last()` and `third_to_last()` methods
- Add `#[track_caller]` to index implementation for better error tracking
- Update all references to use new method names
- Improve documentation with usage examples
The changes provide a more intuitive and consistent approach to accessing witness elements.
Close#4098
ACKs for top commit:
Kixunil:
ACK 3ca3218c23
tcharding:
ACK 3ca3218c23
apoelstra:
ACK 3ca3218c236c63a9b006047524e2b47e310f07d9; successfully ran local tests
Tree-SHA512: 163e7457f3fe5141373e27a6df5fe1da6f2f05f02e877ef96243510d030d832c0fa86ade781e015a3c392f004651170b60438a83d330f1059457e5ade6478af7
In the past we've been using `chunks_exact` because const generics were
unstable but then, when they were stabilized we didn't use `as_chunks`
(or `array_chunks`) since they were unstable. But the instability was
only because Rust devs don't know how to handle `0` being passed in. The
function is perfectly implementable on stable. (With a tiny,
easy-to-understand `unsafe` block.) `core` doesn't want to make a
decision for all other crates yet but we can make it for our own crates
because we know that we simply never pass zero. (And even if we did, we
could just change the decision.)
It also turns out there's a hack to simulate `const {}` block in our
MSRV, so we can make compilation fail early.
This commit adds an extension trait to internals to provide the methods,
so we no longer have to use `chunks_exact`. It also cleans up the code
quite nicely.
d1d483491f Make `hex` in `internals` optional (Martin Habovstiak)
Pull request description:
The `hex` crate is not always desirable - e.g. when the consumer wants to work with raw data only. We already had this optional in `hashes` but if `hashes` is going to depend on `internals` it would break this property.
This change makes `hash` optional, since it's easy: there's just one struct that depends on it.
ACKs for top commit:
tcharding:
ACK d1d483491f
apoelstra:
ACK d1d483491f5f0181e60778e557fadd99b44b5d30; successfully ran local tests; nice
Tree-SHA512: c20091e6febb49b9114273c280580ddcdafc91893f3c73288b374f51990f09f035a044806dd26777148f2e4341ad082ab05f1b49f8ceb3bcd24eb210ffa5fd5f
This fixes issue #4141
Change count_witness_sigops and count_p2sh_sigops to take the spent
closure by value instead of &mut
- Changed both functions to accept S as a value (FnMut) instead of &mut S
- Removes need to annotate &mut when calling the function
Add a standalone `hash` function that is a drop in replacement for
`GeneralHash::hash`. Do not add it to `hmac` - this is in parity with
the current code because `Hmac` does not implement `GeneralHash::hash`.
Use the new function in `bitcoin` removing all occurrences of
`GeneralHash` from `bitcoin`.
In `hashes` replace usage of `GeneralHash::hash` with the new `hash`
function.
We would like to do away with the `GeneralHash` trait. Currently we
bound `Hmac` and `HmacEngine` on it but this is unnecessary now that we
have added `HashEngine::finalize` and `HashEngine::Hash`.
Bound the `HmacEngine` on `HashEngine` (which has an associated `Hash`
type returned by `finilalize`).
Bound `Hmac` type on `T::Hash` where `T` is `HashEngine`.
Includes some minor shortening of local variable names around hmac
engine usage.
Note this means that `Hmac` no longer implements `GeneralHash`.
ecc5791930 Add test checking `XOnlyPublicKey::deserialize` (Martin Habovstiak)
bbe87eccf2 Fix bug in PSBT `Deserialize` for `XOnlyPublicKey` (Martin Habovstiak)
8efacd4dda Deprecate `PrivateKey::from_slice` method (Martin Habovstiak)
0d5cd7af43 Add `from_byte_array` to `PrivateKey`. (Martin Habovstiak)
1778fea66e Add a test checking `PrivateKey::from_slice` (Martin Habovstiak)
b87ddc0043 Don't panic in `PrivateKey::from_slice` (Martin Habovstiak)
Pull request description:
This fixes the bug introduced in #4154 and deprecates `from_slice` method in favor of `from_byte_array` (see commits).
Closes#4191
ACKs for top commit:
jamillambert:
ACK ecc5791930
apoelstra:
ACK ecc5791930b88581fcbc8f2417b0221486bd1031; successfully ran local tests
Tree-SHA512: 05117c68db7b46605dba6104ee7696220416f4efaef1fff01843a910037e4c96bfebc45fcdd16f875e5e800bb33af17193c4aa9b0b1593807df5153e7e935c22
During upgrade of `secp256k1` a number of function calls needed to be
rewritten to not use `from_slice` but `from_byte_array`. Unfortunately,
the conversion wasn't correct and caused panics on invalid inputs
rather than errors.
This fixes it simply by calling `try_into` on the entire slice and
converting the error.
852bcf6017 bitcoin: Remove hash type re-exports (Tobin C. Harding)
Pull request description:
The `{W}PubkeyHash` and `{W}ScriptHash` types are not likely to be used directly by consumers of the library because we have other function that return them and are more ergonomic to use. There is therefor no good reason to re-export them from the crate root.
ACKs for top commit:
apoelstra:
ACK 852bcf60178c9491ebcbf13243cf79857592ccec; successfully ran local tests; will one-ACK merge, still does not affect any public API
Kixunil:
ACK 852bcf6017
Tree-SHA512: fbc4a7e3e116e962dc4b65b8681343e6ecd485a2289c5798979ce8e6ead4c0b6bda04ae33af1bdd711e852f95896107f9fa3c63d52fd36a6f802de4a7b677073
a74393324b Move opcodes back to bitcoin (Tobin C. Harding)
Pull request description:
Duplicate `opcodes` in `bitcoin` and hide it in `primitives` so we do not have to commit to the API.
We use opcodes in `impl fmt::Display for Script`.
Close: #4144
ACKs for top commit:
apoelstra:
ACK a74393324bd47f89fd47281d567ab15ab6bcb2ba; successfully ran local tests; sure
Kixunil:
ACK a74393324b
Tree-SHA512: 738685b9cd2288a581daa6219e3b21bd48bb4845ea627bf6b8085e0e48f5649ac5ec616a3421d10cd37543f76b66d31f94fd55bf94effc2fb8f91d1ecf5c8611
a4b9c196b1 Manually update nightly version (Tobin C. Harding)
Pull request description:
While trying to use the `macro_use_imports` lint I found that there is a bug in last weeks nightly. It has been fixed already so lets update.
Update to todays nightly compiler. Doing so causes some new linter warnings, for now we just allow them.
ACKs for top commit:
apoelstra:
ACK a4b9c196b13a2029ef0de198114a29c71a192d03; successfully ran local tests
Tree-SHA512: a982ba05713c214af3b9375ecf37b66aea07ec0330392c30d2dbe3c9b40e1219b587ee43222f2355292b9b374a6a1d8a19d18b559bb47d33e544ed78509a51b8
Since arrays better convey the intention than slices when parsing
fixed-sized bytes we're migrating to them. This deprecates the
`from_slice` method similarly to how we do it elsewhere.
Private keys have statically-known length of 32 bytes and we are
migrating types with known lenths to use `from_byte_array` methods. This
adds the method to `PrivateKey` as well and uses it to implement
`from_slice`.
During upgrade of `secp256k1` a number of function calls needed to be
rewritten to not use `from_slice` but `from_byte_array`. Unfortunately,
the conversions wasn't correct and caused panics on invalid inputs
rather than errors.
This fixes it simply by calling `try_into` on the entire slice and
converting the error.
Enhance Witness struct element access methods:
- Rename `nth()` to `get()` for clearer slice-like element retrieval
- Introduce `get_back()` method for flexible reverse indexing
- Remove redundant `second_to_last()` and `third_to_last()` methods
- Add `#[track_caller]` to index implementation for better error tracking
- Update all references to use new method names
- Improve documentation with usage examples
The changes provide a more intuitive and consistent approach to
accessing witness elements.
The `hex` crate is not always desirable - e.g. when the consumer wants
to work with raw data only. We already had this optional in `hashes` but
if `hashes` is going to depend on `internals` it would break this
property.
This change makes `hash` optional, since it's easy: there's just one
struct that depends on it.
The `{W}PubkeyHash` and `{W}ScriptHash` types are not likely to be used
directly by consumers of the library because we have other function that
return them and are more ergonomic to use. There is therefor no good
reason to re-export them from the crate root.
f80cf2cb2a update secp256k1 to 0.30.0 (19年梦醒)
Pull request description:
ACKs for top commit:
apoelstra:
ACK f80cf2cb2aa318978da3a6c5df49d82c49344763; successfully ran local tests
tcharding:
ACK f80cf2cb2a
Tree-SHA512: 83b8bb72372025c4a4b81c2b7973a7808a4a1d9d6450adef8b60a890e128b2559b55832159c25fa91daac1856049b070cd910d87313fed2851ced9e72ae5ddf5
While trying to use the `macro_use_imports` lint I found that there is a
bug in last weeks nightly. It has been fixed already so lets update.
Update to todays nightly compiler. Doing so causes some new linter
warnings, for now we just allow them.
Duplicate `opcodes` in `bitcoin` and hide it in `primitives` so we do
not have to commit to the API.
We use opcodes in `impl fmt::Display for Script`.
Close: #4144
bb8f833ca0 Update instruction.rs (kilavvy)
0ce622e668 Update message.rs (kilavvy)
f61941bbe6 Update serialized_signature.rs (kilavvy)
1d2de62e01 Update mod.rs (kilavvy)
Pull request description:
This PR fixes several typos in comments across multiple files:
- Fixed typo `interpretted` -> `interpreted` in `blockdata/script/instruction.rs`
- Fixed typo `neccessity` -> `necessity` in `p2p/message.rs`
- Fixed typo `underlflow` -> `underflow` in `taproot/serialized_signature.rs`
- Fixed typo `ambigous` -> `ambiguous"` in `units/src/amount/mod.rs`
These changes only affect comments and documentation, no functional code changes.
ACKs for top commit:
apoelstra:
ACK bb8f833ca01688eaae75e0fa322f698d34243185; successfully ran local tests; though all these commits could be squashed IMO
Tree-SHA512: d73dc2a86b20de87c0c5efb3e5042e3901c846236670e3a6501f4c93fd54328fef08bfeca276b93642e7b51d04cb8b9c8e1af558f3aabc3c924d60a61e58b031
43ae9d7516 primitives: Hide script error internals (Tobin C. Harding)
2d8227f091 Hide relative locktime error internals (Tobin C. Harding)
Pull request description:
Make the struct fields private and add getters.
ACKs for top commit:
apoelstra:
ACK 43ae9d751622c7bef548a469466d74cf01284129; successfully ran local tests; nice! Way easier to understand these types with the new incompatible / expected names
Tree-SHA512: cfe67d60ea61a2a4c27b09071a6b11739ca281bf0b4a655121f90215ce38c3a637acf53a6e01aa2ef26fa80004cd919bf3b3334dbd9566ee2f594cab7750b563
f7ea6e50b5 Add support for pay to anchor outputs (Erik De Smedt)
Pull request description:
Add support for the newly created Pay2Anchor output-type which was introduced in bitcoin 28.0
See https://github.com/bitcoin/bitcoin/pull/30352
ACKs for top commit:
Kixunil:
ACK f7ea6e50b5
apoelstra:
ACK f7ea6e50b578238b0a7ff421d18d7c7f71d43278; successfully ran local tests
Tree-SHA512: cd3da860e81bd25e6fef72a9118b43d647af2339e9d226c124fa221f63d9c3149189480d40368d38900a999bf59a23fd5302025751ea1bebfea059b4fab21c0b
cf12ba262a Move taproot back to bitcoin crate (Tobin C. Harding)
Pull request description:
I don't know what I was thinking when I move the taproot hash types to `primitives`. As correctly pointed out by Kix we agreed to only have blockdata in `primitives`.
Move the taproot hash types back to `bitcoin::taproot` and remove the extension traits.
ACKs for top commit:
Kixunil:
ACK cf12ba262a
apoelstra:
ACK cf12ba262a646a6341098ee3f4c178a52fc90211; successfully ran local tests
Tree-SHA512: 0c5eabf395c05a93603a46b277c6ea2cc547f3894eef182fceb80f309123d67fe457936a388bac0249ec24cae7521eaef3bf8bd8facca5282e4ce2ea6fafd5f7
83bd83385e Update sighash.rs (leopardracer)
fc4ea87429 Update transaction.rs (leopardracer)
0e70e85a1a Update key.rs (leopardracer)
f7c28ab44d Update input_string.rs (leopardracer)
Pull request description:
This pull request contains changes to improve clarity, correctness and structure.
- Corrected typos in multiple files (e.g., `transacton` → `transaction`, `function panics` formatting).
- Improved readability of comments in `transaction.rs`, `key.rs`, and `input_string.rs.`
- No functional code changes, only documentation updates.
This message provides a clear summary of what was done and why the changes were made. Let me know if you need any modifications!
ACKs for top commit:
Kixunil:
ACK 83bd83385e
tcharding:
ACK 83bd83385e
apoelstra:
ACK 83bd83385e0bbad993f96f5abbe51f71d199aad1; successfully ran local tests
Tree-SHA512: ec0cae0357dd8c60e9286db71c3d18dfa6ba7acbaa27cce78c0586ab73c9631ea58ab453acd85b18d4edd72dcc578a4e08d69b9529e856837d5651efa288a4db
a8168c3f81 Add `taproot_leaf_script` methood to `Witness` (Martin Habovstiak)
59f21a291f Add a test case checking `taproot_control_block` (Martin Habovstiak)
e810ecff7c Fix key/script spend detection in `Witness` (Martin Habovstiak)
Pull request description:
Fixes#4097
High priority because it blocks a bunch of work and should be probably swiftly backported.
My plan is to backport this entire PR and then in the breaking version remove the broken `tapscript` method entirely. Keeping it around would be way too confusing if we're going to have tagged script.
ACKs for top commit:
tcharding:
ACK a8168c3f81
apoelstra:
ACK a8168c3f81a76165022af3f3aeec82317d8a6bf1; successfully ran local tests
Tree-SHA512: 9e3c065f045664c7e4fdf8ba4d9e7dc9281a59eda1187f39b297861714007d58e9e5071c37e1f3b16d171372313ae06aca2d98425a06a6be557718d9f73c0e14
2d6ee57d9c Grab missing changelog (Tobin C. Harding)
Pull request description:
We release `v0.32.4` and `v0.32.5` already but forgot to merge the changelog entries back into master.
Grab the missing changelog entries from the `0.32.x` release branch.
ACKs for top commit:
Kixunil:
ACK 2d6ee57d9c
apoelstra:
ACK 2d6ee57d9c67a49a39eaf89781129a63c7d05e1a; successfully ran local tests
Tree-SHA512: bfa4d8ef2c58f1cc74c6a027ed33bab70a660c3bfcf2d2a72839741a3beb5ef6a355e1ca99e3aaa24a77a48f8c678a84d1c0bf7e5e30c1b33496b866e8667351
Align with Bitcoin Core's policy by reducing the minimum non-witness
transaction size from 82 to 65 bytes. This change allows for more
minimal transaction cases (e.g., 1 input with 1 OP_RETURN output),
while still maintaining protection against CVE-2017-12842.
Matches bitcoin/bitcoin#26398
I don't know what I was thinking when I move the taproot hash types to
`primitives`. As correctly pointed out by Kix we agreed to only have
blockdata in `primitives`.
Move the taproot hash types back to `bitcoin::taproot` and remove the
extension traits.
As part of the 1.0 effort and forward maintainability hide the internals
of the two error types in the `script` module. Add getters to get at the
invalid size.
We release `v0.32.4` and `v0.32.5` already but forgot to merge the
changelog entries back into master.
Grab the missing changelog entries from the `0.32.x` release branch.
Bitcoin block headers have a timestamp. Currently we are using a
`u32`. while this functions correctly it gives the compiler no chance
to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`.
Document it and test the API surface in `api.rs`.
We already have `tapscript` method on `Witness` which is broken because
it doesn't check that the leaf script is a tapscript, however that
behavior might have been intended by some consumers who want to inspect
the script independent of the version. To resolve the confusion, we're
going to add a new method that returns both the leaf script and, to
avoid forgetting version check, also the leaf version.
This doesn't touch the `tapscript` method yet to make backporting of
this commit easier. It's also worth noting that leaf script is often
used together with version. To make passing them around easier it'd be
helpful to use a separate type. Thus this also adds a public POD type
containing the script and the version. In anticipation of if being
usable in different APIs it's also generic over the script type.
Similarly to the `tapscript` method, this also only adds the type and
doesn't change other functions to use it yet. Only the newly added
`taproot_leaf_script` method uses it now.
This is a part of #4073
The previous commit fixed a bug when `taproot_control_block` returned
`Some` on key-spends. This adds a test case for it which succeeds when
applied after the previous commit and fails if applied before it.
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).
Currently in order to release `hashes v1.0` we need to 1.0 `io` as well.
For multiple reasons, many out of our control, the `io` crate may not
stabalise any time soon.
Instead we can invert the dependency between the two crates.
This is an ingenious idea, props to Kixunil for coming up with it.
Notes
- `io` does not currently re-export the `hashes` crate.
- This work highlights that we cannot call `hash_reader` on a siphash.
- The `Hmac::hash_reader` uses the default key which may not be obvious.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
We would like to return an error when doing math ops on amount types.
We cannot however use the stdlib `Result` or `Option` because we want to
implement ops on the result type.
Add an `AmountOpResult` type. Return this type from all math operations
on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators
of amounts - somewhat ugly to use, see tests for example usage.
6002ccdc56 Add a tagged sha256t hash engine (Tobin C. Harding)
3e8e2e46bf Use Self::Engine in GeneralHash impl (Tobin C. Harding)
a0211906fe sha256t: Remove standalone from_engine function (Tobin C. Harding)
5ce8781162 Remove the Tag::engine method (Tobin C. Harding)
ba6425947f hashes: Use associated cost for pre-tagging (Tobin C. Harding)
613fddc82b Delete deprecated sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
Add a new hash engine to the `sha256t` module and put the tag on it.
Note the issue suggests adding the tag to `sha256::HashEngine` but this PR adds a new type to `sha256t` and adds the tag on it.
Resolve: #1552
ACKs for top commit:
apoelstra:
ACK 6002ccdc567b26c69f6b55173f5d11b95b4d6966; successfully ran local tests
Kixunil:
ACK 6002ccdc56
Tree-SHA512: 8a8945bdb89841b87af2eb94073b4d01993338f34a7a3e919bccf4e8edea0fe9d5d2818b6484700b2e72143f315633338692a436149935c5156b77757b025f5e
We would like it if two different pre-tagged engines were considered
different types so it is not possible to mix them up.
Add a new `sha256t::HashEngine<T>` where `T` is a tag the same as on
`sha256t::Hash<T>`.
Now we have an associated const we can do away with the `engine` trait
method all together. Users can call `Hash<FooTag>::engine` instead. This
is better because its an API more similar to the other hash types and
therefor easier to discover and remember.
85e0330d7f Run the formatter (Tobin C. Harding)
7be0db730a hashes: Move bench and test code into files (Tobin C. Harding)
665fa9de99 hashes: Pull crypto out into submodule (Tobin C. Harding)
1bfd1e071a hashes: Make module subdirectories (Tobin C. Harding)
Pull request description:
This is an attempt at point 3 in https://github.com/rust-bitcoin/rust-bitcoin/pull/3961#issuecomment-2619946074
However instead of using `api.rs` and `implementation.rs` it uses `crypto.rs` for the cryptography stuff and leaves the rest in `mod.rs`.
This whole PR is internal changes only. Almost entirely code moves, review is easy if you have your `diff` configured nicely.
ACKs for top commit:
Kixunil:
ACK 85e0330d7f
apoelstra:
ACK 85e0330d7f057c9fe447bff5fdb6023150ead319; successfully ran local tests; look great! thanks!
Tree-SHA512: e52e6410e86fc93ec34a72be8c64f02148f46958f8f5c1850075b1a7f41886c220f09144ccd05a98a551c356a5d25524c8884fc8578a205b27f385ec0725f13b
a7526b6a70 Remove `fee_vb` (yancy)
73b14d03b9 Add `to_fee` in place of `fee_wu` (yancy)
Pull request description:
Rename fee_wu and remove fee_vb
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3908
ACKs for top commit:
apoelstra:
ACK a7526b6a70642e04f0a8ad8a89aa368031fdb6cd; successfully ran local tests
Kixunil:
ACK a7526b6a70
Tree-SHA512: fd865525e56caddc49158fd94fb125d92fd09654d5c72609a6f8e34370e79a9be4213dbd7e69b0d8498d92fca8a970142262f56b63cebd76c200aca75b6e0ca6
Use the more idiomatic to_fee instead of `fee_wu`. Since the method
takes a strongly typed argument, remove `wu` from the method name
to improve clarity.
We want to get rid of this constant, so we replace it in tests with 0
amount, empty script. Notably, the tests were already using it as a
dummy value - the exact amount was irrelevant, so this change doesn't
break anything.
The `encode_signing_data_to_inner` function previously constructed a
transaction internally, requiring a bunch of allocations, which it'd
then consensus-serialize into a writer (hasher). It also used a dummy
`TxOut::NULL` value which we want to get rid of.
To get rid of both allocations and the NULL value we serialize the
transaction on-the-fly. Because the encoding doesn't involve witnesses
it's not too complicated and the consensus encoding will never change so
there are no desync bugs possible. We may later change this to an
abstract transaction though.
4dad4730a8 Add unreleased changelog entry (Tobin C. Harding)
08bb57e499 Use MAX_MONEY in serde regression test (Tobin C. Harding)
Pull request description:
We plan on enforcing MAX_MONEY in the amount types. In preparation use MAX_MONEY in the serde regression test instead of the arbitrary, and too large DEADBEEFCAFEBABE value.
ACKs for top commit:
apoelstra:
ACK 4dad4730a81f9f08a565f196e5699f054e28f977; successfully ran local tests
Tree-SHA512: 10fefdb75289f4c93537e031410eb729420729563b262d6104948a0176a78c4f6130956b550cc08ffc924b1bce1c42f0544542879a4d44165022196ef695718a
We plan on enforcing MAX_MONEY in the amount types. In preparation use
MAX_MONEY in the serde regression test instead of the arbitrary, and too
large DEADBEEFCAFEBABE value.
We have an `_unchecked` amount constructor that makes no assumptions
about the argument. We would like to start enforcing MAX_MONEY but the
diff to introduce this is massive. In an effort to make it smaller we
can do all the hardcoded ints first. We did this already but a bunch
more snuck in or were missed.
In any amount constructor that passes in a hardcoded const as a decimal
integer (i.e., not hex) use the `_unchecked` version.
Done in preparation for enforcing MAX_MONEY.
New nightly lint warning "called `Iterator::last` on a
`DoubleEndedIterator`; this will needlessly iterate the entire iterator"
Code that gives the warning is correct, allow the lint to remove the
warning.
Update rustc nightly to 2025-01-16
Back in 2022 we elected to use `mutagen` for mutation testing. Since
then `cargo mutants` has progressed to a point where we would now like
to use it instead.
Remove all the `mutagen` stuff and update the lock files.
Close: #2829
e0028239cf api: Run just check-api (Tobin C. Harding)
5eb5941215 Add FIFTY_BTC const to the amount types (Tobin C. Harding)
Pull request description:
The mining reward for the first epoc is 50 bitcoin. For mainnet this is old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the amount types uniform also add it to the `SignedAmount`.
ACKs for top commit:
storopoli:
ACK e0028239cf
jamillambert:
ACK e0028239cf
apoelstra:
ACK e0028239cf207660deb2873a92bacfa1315af634; successfully ran local tests
Tree-SHA512: 623ed8b1f5fe8dd95309179308fea83d68be4349becf6305769b0378cc9032961df2c062dc2bf702fec5e2394e8abb7360d2be6f19b6cf505db8769a5ae39e16
The mining reward for the first epoc is 50 bitcoin. For mainnet this is
old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the
amount types uniform also add it to the `SignedAmount`.
b11ace359a Fix up ParsePublickeyError (Innocent Onyemaenu)
Pull request description:
Resolves#3835
In #945fcd09 we forgot to Update impl for ParsePublicKeyError Display and Error traits
ACKs for top commit:
tcharding:
ACK b11ace359a
apoelstra:
ACK b11ace359a52d9137b8fe5919d9825d61e3e9fad; successfully ran local tests; thanks!!
Tree-SHA512: 5c4e5e113605bc5b9c3c0d2ca65a0fdae80726a0f2f791255cc1d6567812a9132ff4f5bd8f30276fef916469d5fd29f6d72ddf4adb6340e52f4e0198ac4fb371
In #3847 we added an `InvalidCharError` into one of the variants of
`ParsePublicKeyError` but we forgot to update the trait
implementations.
Fix the `error::Error` and `Display` implementations for
`ParsePublicKeyError`. While we are at it match on `*self` as is
typical in this codebase.
With this applied #3835 is fully resolved.
Close: #3835
Currently `InputString` is in the public API of `units` because of the
trait bound on `parse::int()`. We can just do the monomorphisisation
manually to remove it.
This patch renames `int` to have three different names, one for `&str`
one for `String`, and one for `Box<str>`.
1e0c065740 typo: Address::is_valid_for_network (Jose Storopoli)
Pull request description:
I believe this is a typo.
ACKs for top commit:
apoelstra:
ACK 1e0c065740266a8ad207a95f4d964394ba3f07d7; successfully ran local tests
tcharding:
ACK 1e0c065740
Tree-SHA512: aff71b91dc707c0c6c723e35510d284fd9a60895b7f12828819eb0d4520216491a2cf054b59872ac4be175d566674a32bde8c170f43beb6fc15dd19e6be12e45
706a135de6 bitcoin: Add an example of doing I/O to encode/decode (Tobin C. Harding)
Pull request description:
In an effort to improve the documentation on `bitcoin_io` add an example in `bitcoin` crate that demonstrates a few things:
- Encode/Decode a `rust-bitcoin` type to/from a stdlib type.
- Encode to a custom type by implementing `bitcoin_io` traits.
- Encode to a foreign custom type by using the `bitcoin_io::bridge::FromStd` wrapper.
Later we can link to this example online in the `bitcoin_io` docs.
ACKs for top commit:
apoelstra:
ACK 706a135de6ca2be0231c2f62f4f2b156e08c9a49; successfully ran local tests
Tree-SHA512: cd3ff4067d5b86031255cb31fe17dea3fd22479699d32efad93c359de465ef6250f29aa4c43ce4218ae623fa6c355661960c11908a729a895655319141b82852
e09bdb5f98 Add BIP324 V2 p2p network message support (Nick Johnson)
Pull request description:
Migrating over the BIP324's library's V2 network message encoding and decoding. As discussed over in https://github.com/rust-bitcoin/rust-bitcoin/discussions/2959, this is probably the natural home for it and also cleans up some gross copy/pasta of some of the encoding/decoding chain logic.
This patch adds `V2NetworkMessage` which wraps a `NetworkMessage`, but handles the V2 encoding and decoding. It is a parallel of the existing `RawNetworkMessage` (which mentioned before, may be better described as `V1NetworkMessage` https://github.com/rust-bitcoin/rust-bitcoin/issues/3157). A priority of this patch was to not re-invent any wheels and try to use the existing patterns as much as possible.
ACKs for top commit:
tcharding:
ACK e09bdb5f98
apoelstra:
ACK e09bdb5f98ea516382a04283373ad97a41d57c2b; successfully ran local tests; nice!
Tree-SHA512: a5078d4d3deb04c2e06ea513bbc8c97d0e6d5da5b029847a97b3f90bf55a263858dd16d88299f853aa3c468f7b9bceb3973c5652a49d3e96df3e91181b455f29
29a71de928 Bound Address parsing on NetworkValidationUnchecked (Tobin C. Harding)
cf455d3a06 Fix typo in prifixes (Tobin C. Harding)
Pull request description:
Currently it is not possible for downstream to use a generic on the `Address` type in structs in conjuncture with
derives (`serde::Deserialize` and `Display`) because our impls are only done for `NetworkUnchecked` (as they should be).
However, as observed by dpc, if we add a secondary marker trait and use it to bound the impls, implementing the new marker for `NetworkUnchecked` then downstream can use derives by way of
```
#[derive(Serialize, Deserialize)]
struct Foo<V>
where V: NetworkValidation,
{
#[serde(bound(deserialize = "V: NetworkValidationUnchecked"))]
address: Address<V>,
}
```
This is cool as hell because the `Address` type is currently a royal PITA.
Patch 1 is trivial cleanup.
To get past a build error in `FromStr` I used this little trick
```rust
// We know that `U` is only ever `NetworkUnchecked` but the compiler does not.
Ok(Address(address.0, PhantomData::<U>))
```
Resolve: #3760
and
Close: #3856
ACKs for top commit:
apoelstra:
ACK 29a71de92817bccd49b42b1055cc570832e6b959; successfully ran local tests
Tree-SHA512: 7c158dddb9fdbaaa1e48204bbf915b18ced56f5d82ce82630db6c0b52161bcf43b3ac413fa990a23975743c56917985b2666a74f9067221f003f2dcf080f827e
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`.
In an effort to improve the documentation on `bitcoin_io` add an example
in `bitcoin` crate that demonstrates a few things:
- Encode/Decode a `rust-bitcoin` type to/from a stdlib type.
- Encode to a custom type by implementing `bitcoin_io` traits.
- Encode to a foreign custom type by using the `bitcoin_io::bridge::FromStd` wrapper.
Later we can link to this example online in the `bitcoin_io` docs.
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.
1a8f5b19fb Update to rust-ordered 0.4.0 (Tobin C. Harding)
Pull request description:
We just released a version of `ordered` that makes `ArbitraryOrd` object safe - use it.
Upgrade to the latest version of `rust-ordered` - `v0.4.0`.
ACKs for top commit:
apoelstra:
ACK 1a8f5b19fbc37c74bbdd7dcee55b3294046ef88e; successfully ran local tests
Tree-SHA512: 823962a9e6b956126c7ff7a63b780431e358a9f0713f30e3801de3febea1e1e569b8ba3f6e3c0dc3c38759ac233c3d0ddea2223588cd796db6849fae8b599e04
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
I just went to town on the `rust-ordered` crate to get it ready for
releasing a `1.0` version. None of the changes effect our usage here in
`rust-bitcoin`.
Upgrade to the latest version of `rust-ordered` - `v0.3.0`.
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
We need to do a quick release because it turns out I was wrong in
thinking that making `hex` an optional dependency is not a breaking
change.
```
the package `bitcoin` depends on `bitcoin_hashes`, with features: `hex`
but `bitcoin_hashes` does not have these features. It has a required
dependency with that name, but only optional dependencies can be used as
features.
```
Add a changelog, bump the version, depend on the new version repo wide,
and update the lock files - like its our job.
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
463fbd16f1 Update to arbitrary v1.4 (Tobin C. Harding)
Pull request description:
Out with the old in with the new, update to the latest minor version of arbitrary.
No real reason to do this except that I wanted to add the minor version instead of using just `1` so that we don't accidentally pull a new minor version in during a patch release (after we 1.0).
(I'm unsure if this is necessary and did not test it against MSRV.)
ACKs for top commit:
apoelstra:
ACK 463fbd16f1431febe7e4fc50abd7415886542109; successfully ran local tests
Tree-SHA512: 88913f993b97abba224ba94ee9d5057894bb7142fef07e557de03a423da314a9f21632932fb64c4c27c676051bbb6e90d9b90bfa944af810e834bf42707d3cff
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
Out with the old in with the new, update to the latest minor version of
arbitrary.
No real reason to do this except that I wanted to add the minor version
instead of using just `1` so that we don't accidentally pull a new minor
version in during a patch release (after we 1.0).
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`.
5b3e4ab95b base58ck: Bump version to 0.2.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changelog entry, bump the version, and update the lock files.
ACKs for top commit:
storopoli:
ACK 5b3e4ab95b
apoelstra:
ACK 5b3e4ab95b015b5bba5b73db2052cbb63a901466; successfully ran local tests
Tree-SHA512: 8347b0540ccdfc29f4a03f0a3554142b4fbd39b519512dcc886c28cae75666201aa8a7e4a07999e70c0348e59f02c4c0d67541a20102153b36150f5e4748a89e
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
6950c0a7b5 Change `Amount::MAX` to equal `MAX_MONEY` (Jamil Lambert, PhD)
Pull request description:
As discussed in #3688 and #3691 using `u64::MAX` causes errors when converting to `f64` so `MAX_MONEY` is should be used as the maximum `Amount`.
- `Amount::MAX` changed to equal `MAX_MONEY`
- Add a check to add, multiply and from_str functions
- Change tests to account for new lower maximum
Different approach to the existing PR #3692. I have only done Amount and not SignedAmount until there is feedback on which approach to use.
ACKs for top commit:
tcharding:
ACK 6950c0a7b5
apoelstra:
ACK 6950c0a7b507f9d70c1ebdab540634482f73b247; successfully ran local tests
Tree-SHA512: 03ebf39c47b19ba88d95235538039f28bfa29f4499618fab25c9b627684c348ed41caef682e8f0e01ca62cf9ced8a1183fe3ed861bffeb9609b09440ddfb1c92
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
Needs release notes but there are 20 pages of them, that feels like too
many to go through manually.
Includes a missing changelog entry in `units` for an already released
change that is included in this `bitcoin` release.
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
667b7f1010 Remove unused extern statement (yancy)
Pull request description:
`extern crate` keywords are no longer needed to import a crate as of Rust 2018
ACKs for top commit:
tcharding:
ACK 667b7f1010
apoelstra:
ACK 667b7f1010bc0745d3e39181e15ec795aeca68dc; successfully ran local tests
sanket1729:
ACK 667b7f1010
Tree-SHA512: d157aa881dfce12b65bb676425cb5f17fa5a193d0001f37c26f74cab1254ab08fdcc9b7462b9a74210f85028299af9e14c0071ba390642b59bb7e986ba85933d
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
d9b48cc2ce Add p2wpkh address creation example (yancy)
Pull request description:
Add example for creating p2wpkh address. It would be helpful to add examples the other address types (p2tr etc) either in separate files or in one. Note this should be created in the Address crate however that crate is currently empty.
ACKs for top commit:
tcharding:
ACK d9b48cc2ce
apoelstra:
ACK d9b48cc2ce68b065640caa22da6fbcb1f3ad85f8; successfully ran local tests
Tree-SHA512: b383c1aca2d3669f66f0dcaf858526793ffc9e62671cf4985c44a11f7717c8e02d359b4d7d8b5822ebdde27595612e10bc831c84a2c4b654f69d4137ad16ba2f
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
In preparation for releasing `primitives v0.101.0` bump the version
number, add a changelog entry, update the lock files, and depend on the
new version in all crates that depend on `primitives`.
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.
The only reason we need `hex-conservative` is to parse strings and
format them as hex. For users that do not require this functionality we
can make the `hex-conservative` crate an optional dependency.
The `serde` feature requires `Display` so we enable `hex` from the
`serde` feature.
If `hex` feature is not enabled we still need to be able to debug so
provide `fmt::Debug` functionality by way of macros.
Close: #2654
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.
81d8699b55 units: Put no_std up top (Tobin C. Harding)
3e332c3839 amount: Fix docs on FromStr (Tobin C. Harding)
5bec76aa51 amount: Fix rustdocs (Tobin C. Harding)
41c80cc476 amount: Improve docs on div by weight (Tobin C. Harding)
2b4a61739a Add type to debug output for Amount (Tobin C. Harding)
42e5043b33 Add from_int_btc group of functions (Tobin C. Harding)
Pull request description:
Improve the `amount` module by doing:
- Patch 1: Add/update `from_int_btc` and `from_int_btc_const` functions
- Patch 2: Add type to debug output for `Amount`
- Patch 3: Fix incorrect docs
- Patch 4: Make docs use correct style and be uniform across the two amount types
- Patch 5: Fix docs on `FromStr`
ACKs for top commit:
apoelstra:
ACK 81d8699b55dad570247cb14d2b3eea64270a83cf; successfully ran local tests
jamillambert:
ACK 81d8699b55
Tree-SHA512: ddd6e02b4cd9a9aa8cbdd2d2f7ae289a6bcca9eb002ef0c6d83a6eca950b2cd08f5918286bb33e814f321e3bfe83fdf75ae051cf8f91ee45d3e4abe76c1c2a4c
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
Add/update the from_int group of functions to provide one that errors
and one that is const and panics (errors in const context are not useful
because one cannot call `unwrap` in const context).
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.
f2e1a4b16c bitcoin: Add some upgrade notes (Tobin C. Harding)
Pull request description:
While upgrading `miniscript` to use tip of master I made some notes to help users upgrade. Just throw them in the changelog file or now.
ACKs for top commit:
storopoli:
ACK f2e1a4b16c
apoelstra:
ACK f2e1a4b16cb8b71b50da723a20761fbe3822d6fc; successfully ran local tests
Tree-SHA512: 3e7a64445476dce8c716edf127b2594c434948bfff7c301ac74a723fa49b2784cca90cf74d231fe18a9fa8e80ffd5cb43232fb90aeae5943568623d4252fc4ef
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.
f37b573290 bitcoin: Set version to 0.33.0-alpha (Tobin C. Harding)
Pull request description:
We would like to create branches in other repos/crates that track master in this repo for testing purposes. In order to do so we need a non-0.32 version otherwise `cargo` pulls from crates.io.
Set the version to `0.33.0-alpha` - try not to get too excited, this release is a looong way away.
ACKs for top commit:
apoelstra:
ACK f37b5732901133eff0b8b11744f743c71533c240; successfully ran local tests; though I think if you try you can make cargo behave better
Tree-SHA512: 08d9f6e658ac575e4d42681ffe3fff2116ee35e31c1150b0f21228cb5ef9fc326ea604a9cfc56378a7da60815c2e6e9add4dd4d96969fea2dda2005d1c9af34f
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
We would like to create branches in other repos/crates that track master
in this repo for testing purposes. In order to do so we need a non-0.32
version otherwise `cargo` pulls from crates.io.
Set the version to `0.33.0-alpha` - try not to get too excited, this
release is a looong way away.
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
adf2fa3113 Update lock files and downgrade minimum arbitrary (Tobin C. Harding)
Pull request description:
Recently we added a use of `Arbitrary` for `[u8; 64]` (schnorr sig). I hit a build failure locally and discovered that
- `arbitrary 1.0.0` does not implement `Arbitrary` for 64 byte array
- We use `arbitrary = 1` in the `bitcoin` manifest
- We have `arbitrary 1.3.2` in the `Cargo-minimal.lock` file
Fix this all up by doing:
- Use `1.0.1` in the `bitcoin` manifest because that is a hard minimum version required to build
- Downgrade the `Cargo-minimal.lock` file to use `arbitrary v1.0.1`
- Upgrade the `Cargo-recent.lock` file while we are at it for good measure.
ACKs for top commit:
shinghim:
ACK adf2fa3113
apoelstra:
ACK adf2fa3113 successfully ran local tests
Tree-SHA512: 7be48d2086523322ca4b5f83a344d5096c46cfaff6bd2deb829d30b439feb045ba86b51e9ab2eac4ad5693b0f54a56ca4b86aadafc17a7e4a15cda4a349d0b18
4d18624435 Update CHANGELOG files (Tobin C. Harding)
Pull request description:
When we create point releases we merge the release tracking PR directly into the version branch, including the changelog entries. This means the changes never end up on `master`. We need to make a mental note to patch `master` anytime we merge a point release PR.
Audit all crates with a non-zero current point release and check we have the changelog entries on `master`.
ACKs for top commit:
apoelstra:
ACK 4d18624435 successfully ran local tests
Tree-SHA512: 826a5e5a565237d4f499dd56471b01ee7970b04125931f5a49355099507e24ae4452108004276939884e338201dd3ae7b66fd68439523b6bb1b07b83d3759ae4
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`.
Tobin C. Harding
merge-script
NinaLua
Martin Habovstiak
jrakibi
Erick Cestari
19年梦醒
Fmt Bot
kilavvy
Erik De Smedt
leopardracer
Liu-Cheng Xu
Jose Storopoli
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