Currently we have two macros used when creating array wrapper types,
one is in `internals` and the other in `bitcoin::internal_macros`. It
is not immediately obvious what is what and why there are two.
Improve the macros by:
- Move the inherent functions to `impl_array_newtype`
- Use `*_byte_array` for the names instead of `*_bytes` for functions
that return arrays
- Add `as_bytes` to return a slice
- Add `to_bytes` to return a vector
- Re-name the other macro to match what it now does
d353be4546 bip32: derive_xpriv should not return a Result (Jose Storopoli)
Pull request description:
We discussed in #2752 that `derive_priv` never fails.
This PR addresses that issue.
ACKs for top commit:
apoelstra:
ACK d353be4546
tcharding:
ACK d353be4546
sanket1729:
ACK d353be4546
Tree-SHA512: 3a3d09027c6079581636b51a506fac2b325e592d182167c0a3d8676b41fb2bef59a85a404ad12b2e14d73e58fd4b6d8f5923a3dc76a4b9724033097fc08b36ac
7685461e62 Document the sha256t_hash_newtype direction (Tobin C. Harding)
30e91cc766 Default to forward for tagged hashes (Tobin C. Harding)
5ecc69cd28 Add forward/backward unit test (Tobin C. Harding)
9aee65d1ba Refactor tagged hash tests (Tobin C. Harding)
216422dffc Remove schemars impl for test type (Tobin C. Harding)
Pull request description:
First three patches are preparation, improvements to the units tests in `sha256t`.
From the final patch:
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
This is an API break and may quietly break some users downstream - eventually we should stop doing that sort of thing.
ACKs for top commit:
apoelstra:
ACK 7685461e62
Tree-SHA512: cb8a41b207aa68ecf63cb7af7f39f7d7c8a3a27f38595867949b288a81a20bff0c17aa4c17bb099e2ecf85194d83bad23c9c9792f511b6c4cd625ff27c1affaa
Currently we require indexing trait bounds as well as `Borrow` on the
`Hash` trait. We also already implement `AsRef`.
It was observed that `Borrow<[u8]>` does not best describe what we want
from the `Hash` trait implementor but rather `AsRef<[u8]>` does.
Remove all the inexing trait bounds. Remove the `borrow::Borrow<[u8]>`
trait bound. Add a `convert::AsRef<[u8]>` trait bound.
This leaves the `Borrow<[u8]>` implementation for hashes created with
`hash_newtype`, I'm not sure if this should be removed or not.
During the recent release cycle we left `bitcoin` on the last rc
version.
Set the version number to `v0.33.0-unreleased` to make it obvious what
it is.
Close: #2724
12411fc917 Fix typo in deprecated BIP-32 type (matthiasdebernardini)
Pull request description:
In #2258 we attempted to add back in deprecated BIP-32 types - but we spelled the identifier incorrectly. The patch was then backported to the `0.31.x` branch in December but was only just noticed now.
Fix typo in deprecated type from `Extendend` -> `Extended`.
ACKs for top commit:
tcharding:
ACK 12411fc917
storopoli:
ACK 12411fc917
apoelstra:
ACK 12411fc917
Tree-SHA512: f70e8fe741740f62b29932d8ee84cbe7803cb71dfb0491d251c3a982ede07ea7a32b5ecdf569d6012ee05509e8182a439b022c606a2f01742f4908089edc85a9
In PR #2258, deprecated BIP-32 types were re-added but contained a typo in the identifier: "Extendend" instead of "Extended". This commit fixes that typo.
The incorrect patch was backported to the 0.31.x branch in December but only noticed recently.
26b9782d8b CI: Re-write run_task.sh (Tobin C. Harding)
Pull request description:
Recently we re-wrote CI to increase VM level parallelism, in hindsite this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling to the workflow. Also don't bother testing with beta toolchain.
### Note on review
The diff is hard to read for `rust.yml`, I tried splitting out a bunch of separate patches but it resulted in the same thing (because there are so many identical lines in the yaml file). I suggest just looking at the yaml file and not the diff.
ACKs for top commit:
apoelstra:
ACK 26b9782d8b
sanket1729:
ACK 26b9782d8b.
Tree-SHA512: 1b0a0bab5cf729c5890f7150953499b42aebd3b1c31a1b0d3dfa5b5e78fda11e17a62a2df6b610ab4a950d5709f3af6fff1ae64d9e67379338903497ab77ae0e
b355740da4 chore: format and standardize all markdowns files (Jose Storopoli)
Pull request description:
according to the github flavor
(https://github.github.com/gfm/)
ACKs for top commit:
apoelstra:
ACK b355740da4 thanks for going through all this!
tcharding:
ACK b355740da4
Tree-SHA512: a9d5671ddc6f922b42189cae11b2a2a877663c909f73f1e8c407a4de7ac93b291e435373b79be2c708f1ecb717b2ede147c0f7730d582a1cb5bee937603005f0
1c836acf30 bitcoin: Stop slicing hashes (Tobin C. Harding)
Pull request description:
As part of the ongoing effort to improve `hashes`; stop using slicing of hash types and use `as_byte_array()` to get an array reference instead. This gives us more flexability to modify the `hashes` module.
ACKs for top commit:
apoelstra:
ACK 1c836acf30
storopoli:
ACK 1c836acf30
clarkmoody:
ACK 1c836acf30
Tree-SHA512: ca4becfc9bae13127aae09bd1eb5e03efc670fb4e56c2aba1cf0e3cf5f5a762dd8c5cff8ff7c15167902b7440f70202e0884f1e4d3bb651476019c78d2be3408
Recently we re-wrote CI to increase VM level parallelism, in hindsite
this has proved to be not that great because:
- It resulted in approx 180 jobs
- We are on free tier so only get 20 jobs (VMs) at a time so its slow to run
- The UI is annoying to dig through the long job list to find failures
Have another go at organising the jobs with the main aim of shortening
total run time and making it easier to quickly see fails.
Re-write the `run_task.sh` script, notable moving manifest handling
to the workflow. Also don't bother testing with beta toolchain.
WASM Note
Removes the `cdylib` and `rlib` from the manifest patching during wasm
build - I do not know the following:
- Why this breaks on this PR but not on other PRs
- Why I can't get wasm test to run locally on master but PRs are passing
- What the `cdylib` and `rlib` were meant to be doing
This is the docs from: https://doc.rust-lang.org/reference/linkage.html
* --crate-type=cdylib, #![crate_type = "cdylib"] - A dynamic system
library will be produced. This is used when compiling a dynamic library
to be loaded from another language. This output type will create *.so
files on Linux, *.dylib files on macOS, and *.dll files on Windows.
* --crate-type=rlib, #![crate_type = "rlib"] - A "Rust library" file
will be produced. This is used as an intermediate artifact and can be
thought of as a "static Rust library". These rlib files, unlike
staticlib files, are interpreted by the compiler in future linkage. This
essentially means that rustc will look for metadata in rlib files like
it looks for metadata in dynamic libraries. This form of output is used
to produce statically linked executables as well as staticlib outputs.
As part of the ongoing effort to improve `hashes`; stop using slicing of
hash types and use `as_byte_array()` to get an array reference instead.
This gives us more flexability to modify the `hashes` module.
If one writes signing data using one of the two
`*_encode_signing_data_to` functions then creating the message to sign
is slightly nuanced and different for each of the functions. For Taproot
one must use a specific tagged hash and for ECDSA one must use a sha256d
hash.
Add documentation that explains the hashing requirements for each
function.
Displaying backward is an anomaly of Bitcoin Core's early days and the
double SHA256 hash type. We should not let this unfortunate beast leak
out into other places.
Default to displaying forward when creating a new tagged hash and remove
all the explicit attributes from `bitcoin` that just clutter the code.
f96bbebdcc Set release version in deprecated attribute (Tobin C. Harding)
Pull request description:
In preparation for release replace "TBD" with the next release version - `v0.32.0`.
ACKs for top commit:
apoelstra:
ACK f96bbebdcc
storopoli:
ACK f96bbebdcc
Tree-SHA512: 7478808322357d853fab2bf25a7d42a972d5ee05ed6f206bfb73748efe1154fb392dc76c3d0e1a50314bcfdac3a55a415f3c6d40dfaaab802ae1c69dd1ad9e76
In the segwit signing example we are using the incorrect value when
creating the signature - we should be using the utxo amount (input
amount) not the spend amount (output spend amount).
Close: #2680
30a09670e8 Add docs for custom signets (Tobin C. Harding)
Pull request description:
We have started using `AsRef<Params>` in a few places as a function parameter. If a user of the library wishes to use these functions they need to create a type that can implement this trait. Because we use `non_exhaustive` on the `Params` struct it is not possible to just construct a `Params` type. This may be surprising for some folk.
Add module level docs to the `consensus::params` module with an example of how to create a type that can be used to describe a custom signet network. Use fields inspired by Mutiny Wallet's described usage.
Close: #2690
ACKs for top commit:
sanket1729:
ACK 30a09670e8.
apoelstra:
ACK 30a09670e8 this is great; would like to see more `const` but for example code no big deal
Tree-SHA512: 50881763aea99641e24871b0eae60650174c48f620742944e7d5617fcf1edff73a20b2a8f043433f6f114ff5f3f4691703fc37b28880c305bb052c2d75d1eeeb
We have started using `AsRef<Params>` in a few places as a function
parameter. If a user of the library wishes to use these functions they
need to create a type that can implement this trait. Because we use
`non_exhaustive` on the `Params` struct it is not possible to just
construct a `Params` type. This may be surprising for some folk.
Add module level docs to the `consensus::params` module with an example
of how to create a type that can be used to describe a custom signet
network. Use fields inspired by Mutiny Wallet's described usage.
Close: #2690
I'm not sure why I haven't see this before during the whole test cycle
but while running `cargo kani --only-codegen` we get a bunch of warnings
of form:
warning: use of deprecated field `consensus::params::Params::pow_limit`
We deprecated the `pow_limit` field but still set it (obviously) in
const structs - just shoosh the warning.
830c1e9cfe Allow m prefix in derivation paths (Tobin C. Harding)
Pull request description:
Recently in #2451 we disallowed bip32 derivation paths with the leading 'm' variable.
There is some confusion as to what exactly the bip specifies however Bitcoin Core RPC call `getaddressinfo` returns a derivation path with a leading "m/". This means we need to be able to parse it irrespective of what the bip says.
Be more liberal in what we accept as a derivation path, including both with and without the leading 'm/'.
Leave the full investigation of the bip to a later date.
Change back some of the test strings as makes sense and include test strings to showcase the full current behaviour.
This PR replaces #2674.
ACKs for top commit:
apoelstra:
ACK 830c1e9cfe
sanket1729:
ACK 830c1e9cfe
junderw:
ACK 830c1e9cfe
Tree-SHA512: 7a4fccd49cb8cd91a6c8db51d758ae116d9d2e98fead7b87520ca302022b37ddbcf3f85453941c5f336f8e934ad224beba99527dc29ce8368fbb1f25508c1615
Recently in #2451 we disallowed bip32 derivation paths with the leading
'm' variable.
There is some confusion as to what exactly the bip specifies however
Bitcoin Core RPC call `getaddressinfo` returns a derivation path with a
leading "m/". This means we need to be able to parse it irrespective of
what the bip says.
Be more liberal in what we accept as a derivation path, including both
with and without the leading 'm/'.
Leave the full investigation of the bip to a later date.
Change back some of the test strings as makes sense and include test
strings to showcase the full current behaviour.
The deprecation notice for `is_provably_unspendable` contains "is not
very useful" which is a bit presumptuous to tell to users, it may very
well be useful to them. Use the more helpful text that already exists in
rustdoc on the function.
We only check clippy in CI with --all-features, which usually is the
best way to get maximum coverage. But if you try a couple other feature
combos, especially those related to nostd, you can hit more code.
051c358bcb Remove deprecated legacy numeric methods (Divyansh Gupta)
Pull request description:
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replaced `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
ACKs for top commit:
tcharding:
ACK 051c358bcb
apoelstra:
ACK 051c358bcb thanks! I will remove an equivalent commit from my #2669
Tree-SHA512: c08c856f7f3b281417c29283351eac5e0f75cc1c8d23d9aae58d969219a327b2337fe57932053e53773ebb9dbec04254f90149266b6639a66c5c09f2ad1675ef
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replace `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
In preparation for dropping the first release candidate bump the version
and add a changelog.
Please not I went to much more effort that usual with the changelog,
open to review on the overall form - not promising I'll change it but
definitely would like to keep iterating and improving.
If this changelog is appreciated then FWIW I don't think we should
bother automating it, a machine does not have all the context required
to create it.
e06ebd69e7 units: Bump version number to 0.1.1 (Tobin C. Harding)
a2b019f823 Enable internals "alloc" feature (Tobin C. Harding)
Pull request description:
Fix a minor internal bug in error code in `units` and bump the version number so we can do a point release.
This can go in after the RC drops as part of the release candidate cycle if its easier - as long as its in and released before the finale `v0.32.0` release.
ACKs for top commit:
apoelstra:
ACK e06ebd69e7
sanket1729:
ACK e06ebd69e7
Tree-SHA512: b6523fae57ba3becf27c0a11fe0b1d75db9226d01bde527390e2cf1697520d5daabc5ef3909b2c464b14f38ba4f8ab87aa49d17a1d054767920963ef1c3ef3b9
We have 2 crates that require an allocator, `bitcoin` and `base58ck` -
these crates should enable the "alloc" feature when depending on
`internals`.
For `units` we use the `internals::error::InputString` but do not enable
the "alloc" feature - this is a bug, it means that the parsed string is
being lost from the error types that use `InputString`.
Enable "alloc" for `bitcoin`, `base58ck`, and `units`.
- `bitcoin` and `base56ck` is just for good measure so we don't get
bitten later on.
- `units` is a bug fix and requires a point release.
When signing a Taproot input (in a PSBT) using a key path spend we
currently return the pubkey associated with key that signs. However it
is common to think of the internal key as being the one that signs even
though this is not technically true. We also have the internal key in
the PSBT so matching against it is less surprising.
When using the `Psbt` type to sign a Taproot input using a key path
spend return the internal key.
There is no need to panic if input index is out of bounds because we
have a function to check the validity of the `input_index` argument and
use it in other places already.
f6467ac98d Minimize usage of Network in public API (Tobin C. Harding)
3ec5eff56e Add Magic::from_params (Tobin C. Harding)
Pull request description:
Minimize usage of the `Network` enum in the public API.
See #2225 for context, and https://github.com/rust-bitcoin/rust-bitcoin/pull/1291#discussion_r1492993788 for an interpretation of that long discussion.
Close: #2169
ACKs for top commit:
sanket1729:
reACK f6467ac98d.
apoelstra:
ACK f6467ac98d
Tree-SHA512: f12ecd9578371b3162382a9181f7f982e4d0661915af3cfdc21516192cc4abb745e1ff452649a0862445e91232f74287f98eb7e9fc68ed1581ff1a97b7216b6a
A release or so ago we added `non_exhaustive` to the `Network` enum,
this turned out to make usage of the enum un-ergonomic for downstream
users. After much debate we decided that a way forward was to just
minimize the usage of the enum in the public API by instead use
`AsRef<Params>` so that downstream could define their own network enum
based on the networks they support.
Minimize usage of `Network` by using `AsRef<Params>` as a parameter type
instead. "minimize" because the `Network` still appears in some places.
Currently we are using `Self` (in backticks) in the docs to functions
defined by the `do_iml` macro, this is a bit lazy, we can do better than
that.
Use `doc` attribute and the `$ty` macro variable to construct the docs
to use the type name.
Use a code comment to document the calling restrictions of private
function `from_hex_internal`. (Code comment because comment is not well
formed as per convention in this codebase.)
Improve the rustdocs on the private `U256` type by doing:
- Remove link to self within constructors, just use backticks
- Use `U256` instead of `Self` or `self`
- Fix incorrect usage of `CompactTarget` [0]
[0] We knew this was wrong when we merged it but because the docs are
private we elected to do this follow up patch.
Currently `Magic` has per network consts but no way to dynamically get
the magic bytes for a network. Note also that we are currently trying to
reduce the usage of `Network` in the public API.
Add a public constructor to the `Magic` type that accepts a `Params`
parameter to determine the network to use.
The `pow` types implement `fmt::LowerHex` but do not implement hex
parsing.
Add inherent methods `from_hex` and `from_prefixed_hex` to the
`pow` types.
fd6fedc3ad Improve API for max target threshold calculation (Tobin C. Harding)
6e47d57744 Rename difficulty transition threshold functions (Tobin C. Harding)
4121c9a09f Rename Params::pow_limit to max_attainable_target (Tobin C. Harding)
f0f6d3f162 Take Params instead of Network in difficulty function (Tobin C. Harding)
104dee9376 Debug assert that target != zero in difficulty calc (Tobin C. Harding)
c1ba496a07 Document current behaviour of difficulty_float (Tobin C. Harding)
3d01146374 Allow needless-borrows-for-generic-args (Tobin C. Harding)
2a6821b426 Use link to CompactTarget in rustdoc (Tobin C. Harding)
Pull request description:
When computing the maximum difficulty transition threshold we forgot to check that the returned `Target` is not bigger than the maximum. This value is network specific so keep the original logic but with `_unchecked` on the function name.
This was noted in the discussion on #2161
ACKs for top commit:
apoelstra:
ACK fd6fedc3ad
sanket1729:
ACK fd6fedc3ad
Tree-SHA512: 520ee2a07edb251c84b5ce8b48ed6e5a5c1945126dc7bcdb5570e97101ec4a3dc63fa7992725194869e22b21ee4f5955579d5e2499fcb48167637fd1fb3ae74d
The maximum target threshold has a network dependant upper bound.
Currently we are not checking this bound. One complication is that there
is currently heated open debate around the `Network` type.
We can bypass the `Network` issue by using `AsRef<Params>` instead.
Add a function that does the checks based on the `Params` type as well
as an unchecked version.
These two functions calculate the min/max threshold transition which is
a _target_ not a "difficulty" number. Using "difficulty" in the function
name is unnecessarily confusing.
Rename and deprecate the functions.
The maximum "attainable" target is a `rust-bitcoin` thing, Core use max
unattainable.
Deprecated the `Params::pow_limit` field and add a new field
`max_attainable_target`.
The `Params` type is `non_exhaustive` so this is not an API breaking
change.
What we really want is the maximum target, but since this is a const in
`Params` use an `AsRef<Params>` argument in the `difficulty` functions.
Requires implementation of `AsRef<Params> for Params`.
The `difficulty` calculation requires dividing a target value by `self`.
Add an assertion that `self` is not zero to help devs debug this.
Note that this should never really be hit, but its possible there is a
bug somewhere causing the target to be set to zero - so this may help
debugging.
Also, add panics section to rustdocs.
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
d91cdd20bf docs: Document ordered feature (Tobin C. Harding)
3520f550f0 Implement ArbitraryOrd for relative::LockTime (Tobin C. Harding)
Pull request description:
TL;DR As we do for `absolute::LockTime` and for the same reasons; implement `ArbitraryOrd` for `relative::LockTime`.
locktimes do not have a semantic ordering if they differ (blocks, time) so we do not derive `Ord` however it is useful for downstream to be able to order structs that contain lock times. This is exactly what the `ArbitraryOrd` trait is for.
Fix: #2566
ACKs for top commit:
sanket1729:
ACK d91cdd20bf
apoelstra:
ACK d91cdd20bf
Tree-SHA512: 52ace9222e765dfa266d003b4aff3e93e35d1414c9fd579c4a4a36998d6d1b08bf6d4964a6f1c1d769068d65e47a882495daa4aacf254909a35dce8e01c99a9e
af6dc1db02 internals: Bump version to 0.3.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changelog and bump the version number.
Please note, the changelog is pretty terse.
ACKs for top commit:
apoelstra:
ACK af6dc1db02
sanket1729:
ACK af6dc1db02
Tree-SHA512: b70d4b9de7de90aba3cbff90dd7f25c5ac801d020dbdfe3e64af4c079347cba726aa783a94fc777e7bf177db8402b54948c2dfd4a766d90c1a7a7a6bdfd36136
TL;DR As we do for `absolute::LockTime` and for the same reasons;
implement `ArbitraryOrd` for `relative::LockTime`.
locktimes do not have a semantic ordering if they differ (blocks, time)
so we do not derive `Ord` however it is useful for downstream to be able
to order structs that contain lock times. This is exactly what the
`ArbitraryOrd` trait is for.
Update the rustdocs in `relative` and mirror the docs changes in
`absolute`.
Fix: #2566
0ca5a43ce5 hashes: Bump version to v0.14.0 (Tobin C. Harding)
Pull request description:
In preparation for release add a changlelog entry and bump the version.
Note the hashes 0.13.0 dependency stays in the dependency graph because of secp, we can update secp after releasing `hashes` then update the secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0` dependency - phew.
Note we are right to release this immediately, the two open PRs (#2337 and #2541) that touch `hashes` only add a clippy attribute so can safely be ignored.
ACKs for top commit:
apoelstra:
ACK 0ca5a43ce5
sanket1729:
ACK 0ca5a43ce5
Tree-SHA512: d1d26acb8fbf13f785b25add3f1dac05bb392b5bdbad16ead2bc5dd26f3d668824c4b653c373f88c3562a37e775146766680606cedd19db40e0f197b26ca86b8
`require_network` is typically called as part of parsing, often in the
same line of code. Counter to our normal errors, it makes
`require_network` more ergonomic to use if we just return a `ParseError`
variant.
Close: #2507
Done in preparation for adding the `NetworkValidationError` as a variant
of `ParseError`.
Move the `NetworkValidationError` type to beneath `ParseError`.
Code move only, no other changes.
fd040f5e38 Replace TBD with 0.32.0 (Tobin C. Harding)
Pull request description:
We are gearing up for the 0.32.0 release; replace all instances of TBD with the version number of the upcoming release.
ACKs for top commit:
sanket1729:
ACK fd040f5e38
apoelstra:
ACK fd040f5e38
Tree-SHA512: fe73fd47a794557742f618b21434cd3cc18cde0e861216716723bfcc9135accf63590e1ea60bfeda066acec7312c8b9f1bf09e7454e7161ccaba5ebe60af66fd
af49841433 Hide base58::Error internals (Tobin C. Harding)
4f68e79da0 bitcoin: Stop using base58 errors (Tobin C. Harding)
669d5e8fc6 base58: Add InvalidCharacterError for decoding (Tobin C. Harding)
ec8609393b base58: Add error module (Tobin C. Harding)
42fabbab03 base58: Run the formatter (Tobin C. Harding)
Pull request description:
Improve the error code in the new `base58` crate.
ACKs for top commit:
apoelstra:
ACK af49841433
sanket1729:
ACK af49841433
Tree-SHA512: c05479f02a9a58c7c98fd5987e760288562372e16cceeeb655f0a5385b4a8605945a3b6f7fcf473a7132a40f8dc90d204bc5e9e64fd2cc0bdc37dbcabb4ddc5c
c17db32df3 Pub back in deprecated dust_value (Tobin C. Harding)
Pull request description:
When we renamed `dust_value` to `minimal_non_dust` we forgot to keep the original and deprecated it, doing so assists with the upgrade path.
Put back in deprecated `dust_value`, linking to the rename.
Renamed in #2255, found while testing upgrade of downstream software.
ACKs for top commit:
tcharding:
> ACK [c17db32](c17db32df3) I _think_ this matches the behavior of the old version
apoelstra:
ACK c17db32df3 I *think* this matches the behavior of the old version
sanket1729:
ACK c17db32df3
Tree-SHA512: 28e1bd2e1a0fd13c78c70ad2667b72b3bf649c293201b79c86c00f09d0126389ebaeb430b8dd32aeeec3d60cbd8761ae949f5784a5ea7756b1b9ae77ec96ce61
dec05b63e9 Refactor witness_version and is_witness_program (Tobin C. Harding)
dac552b436 Add unit tests for shortest/longest witness program (Tobin C. Harding)
Pull request description:
Refactor `witness_version` and `is_witness_program`.
- Patch 2 adds a couple of preparatory unit tests.
- Patch 2 does the refactor
Fix: #2618
ACKs for top commit:
apoelstra:
ACK dec05b63e9
sanket1729:
ACK dec05b63e9
Tree-SHA512: 3db0a1d8175cbb2fd18f3254854d02db3ad7efa2620b12f08d9727ef6bb5854f0a015917e57023cd2196a36d13276e80536a0e96318c44a1173da4f6793ca370
When we renamed `dust_value` to `minimal_non_dust` we forgot to keep the
original and deprecated it, doing so assists with the upgrade path.
Pub back in deprecated `dust_value`, linking to the rename.
Adds constructors to allow directly creating locktimes from time or
block counts; adds a flooring constructor to Time to match the ceiling
one; adds an explicit constructor to Height since the From<u16> was not
very discoverable.
These two functions are related. We cannot, by definition, get the
witness version from a script that is not a witness program but
currently the code is not linking these two things.
Refactor by doing:
- Move the check of the witness program bip rules to `witness_version`
- Call `witness_version().is_some()` in the predicate
Improve the docs while we are at it to include the bip text in the
rustdoc. Note I didn't bother referencing the segwit bip number, this
bip text is pretty well known.
Add two unit tests that verify we can correctly determine if a
shortest allowed and longest allowed script is a witness program.
Done in preparation for patching the `witness_version` function.
In preparation for release add a changlelog entry and bump the version.
I'm not 100% sure that this release is API breaking, dependencies
definitely changed. The rest might be only additives but I didn't bother
looking exactly because I think its better to bump the minor version and
err on the side of caution.
Note the hashes 0.13.0 dependency stays in the dependency graph because
of secp, we can update secp after releasing `hashes` then update the
secp dependency in `rust-bitcoin` thereby removing the `hashes v0.13.0`
dependency - phew.
We are currently using the `base58::Error` type to create errors in
`bitcoin`, these are bitcoin errors not `base58` errors.
Note that we add what looks like duplicate
`InvalidBase58PayloadLengthError` types but they are different because
of the expected length. This could have been a field but I elected not
to do so for two reasons:
1. We will need to do so anyways if we crate smash more
2. The `crypto::key` one can have one of two values 33 or 34.
With this applied we can remove the now unused error variants from
`base58::Error`.
6b09857f55 base58: Re-name crate to base58ck (Tobin C. Harding)
Pull request description:
The current name `base58check` is taken, as is `base58`. Use `base58ck` instead.
Add a brief section to the readme about the crate naming.
ACKs for top commit:
apoelstra:
ACK 6b09857f55
sanket1729:
ACK 6b09857f55
Tree-SHA512: 86ee08105906a6f3403dc2602e827b0d46226798ecdedb420ad3ac4b657d6a00e25eabcdfbdb9f8e89bdc3a38e608189f1e073e65593f89a2ad853e8ff027f69
b816c0bb01 hash_types: add unit tests for display of all hash types in the library (Andrew Poelstra)
Pull request description:
This can be checked against the 0.29.x branch, and against the commit prior to #1659 (40c246743b^) and you will see that it is consistent EXCEPT:
* In rust-bitcoin 0.29.x we did not have multiple sighash types, only `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and `TapSighash`.
* In #1565 we deliberately changed the display direction of the sighashes, to match BIP 143.
Fixes#2495.
ACKs for top commit:
tcharding:
That's a win. ACK b816c0bb01
Tree-SHA512: 5b44f40165699910ea9ba945657cd1f960cf00a0b4dfa44c513feb3b74cda33ed80d3551042c15b85d6e57c30a54242db202eefd9ec8c8b6e1498b5578e52800
0d517dcfdd Re-export P2shError (Tobin C. Harding)
646ee1a837 Put re-exports in alphabetic order (Tobin C. Harding)
Pull request description:
As with the rest of the errors in `address::error` that are returned by a pubic function from the `address` module.
Note please, this PR just makes the `address/mod.rs` file uniform, debating the merit of the re-exports is out of scope.
ACKs for top commit:
apoelstra:
ACK 0d517dcfdd
Tree-SHA512: a65ebe6de62b83c6d3723c7c97a0953ddb8da51964ef54e69865855502ae5fa51b2b49c6fd408c452414b56518f4c7ab763faca925e8d81f0fe806af1df92aa2
290e4418e6 units: Fix cargo cult programming (Tobin C. Harding)
Pull request description:
When creating the ParseIntError in `hex_u32` I (tobin) just cargo cult programmed the generic stuff without thinking.
- The `is_signed` field is used to denote whether we were attempting to parse a signed or unsigned integer, it should be `false`.
- The `bits` field should be directly set to 32.
ACKs for top commit:
apoelstra:
ACK 290e4418e6
sanket1729:
ACK 290e4418e6
Tree-SHA512: 7dfd9f0cd98eff1c2b27a92dac5c4e2fe0fa4ae724528ef741fe43d8d923e2d31cbdcd4e540ecfba1b953860dc2b728a958e756e2d2012d9a9e715c0ca3c5068
When creating the ParseIntError in `hex_u32` I (Tobin) just cargo cult
programmed the generic stuff without thinking.
- The `is_signed` field is used to denote whether we were attempting to
parse a signed or unsigned integer, it should be `false`.
- The `bits` field should be directly set to 32.
16a813734c Implement consensus deserialize_hex (Tobin C. Harding)
Pull request description:
We have `serialize_hex` and `deserialize` but no `deserialize_hex`, add it.
Move the `IterReader` out of `consensus::serde` to the `consensus` module.
Add some additional logic to the `DecodeError`, I'm not sure why this wasn't there before?
Use the `HexSliceToBytesIter` by way of the `IterReader` to deserialize an arbitrary hex string. Add unit tests to check that we consume all bytes when deserializing a fixed size object (a transaction).
ACKs for top commit:
apoelstra:
ACK 16a813734c
sanket1729:
ACK 16a813734c
Tree-SHA512: 121285cb328ca01bf9fd2a716e6d625fa93113a11613d44c576e3e49a9d06dc181165d2d9bfb9beea7c3d2aff264f64ade4965acd594b05ce0d1660e7493d2e4
cbee9781e8 Move unit types to units (Tobin C. Harding)
5bd0d7194b Remove unused absolute::Error (Tobin C. Harding)
Pull request description:
Move the following unit types to the new `units` crate:
- `locktime::absolute::{Height, Time}`
- `locktime::relative::{Height, Time}`
- `FeeRate`
- `Weight`
Also move the `parse` module as well as constants as required.
Do minimal changes to get things building:
- Feature gate on "alloc" as needed.
- Remove rustdocs that use `bitcoin` types.
- Re-export units types so this is a non-breaking change.
- Fix import paths.
Patch 1 was originally #2526, putting it in via this PR to try and speed up the process.
Close: #2282
ACKs for top commit:
sanket1729:
ACK cbee9781e8
apoelstra:
ACK cbee9781e8 lgtm. this is a good start. I think the LockTime types should follow Height and Time
Tree-SHA512: 6b0d63c7b054008598d7fa81be7d8c112f2778883b5529d79d446617b94b3c196c9ac735f840d1dfb488700894d3161c6976d44ab0e12ac3af4008068eac5f87
0d64ae6eb4 Added tests for PublicKey::from_str (Sh0g0-1758)
Pull request description:
Fixes: #2550
Added some new tests and refactored some older tests.
ACKs for top commit:
sanket1729:
ACK 0d64ae6eb4
apoelstra:
ACK 0d64ae6eb4 thanks for bearing with me!
tcharding:
ACK 0d64ae6eb4
Tree-SHA512: b6792590c56ccac8e8cf6f182e74cb77c4652c537c0357456ff21a7814ebcc8cf48e0fad4c8d47e6e786a50e2cbb48134cb64406bcc900b4fcad9304d9cf4167
41e8fb0863 Support signing taproot in psbt (yu)
Pull request description:
Hi team, I'm from Keystone Wallet team. currently rust-bitcoin does not support signing taproot transactions in psbt.
We think this founction should be included in the psbt module, we submit this PR. Some context and discussion about this PR can be found here: #2418.
For this PR, mostly two new functions are introduced:
- `bip32_sign_schnorr`: sign a taproot input.
- `sighash_taproot`: calculate the sighash message to sign a taproot input along with the sighash type.
Looking forward to your feedback.
ACKs for top commit:
tcharding:
ACK 41e8fb0863
sanket1729:
ACK 41e8fb0863.
Tree-SHA512: 2eb14a3204e6ed848515483778dd7986662aacb332783d187da72d29e207b78a2d427939f2b958135a32de5459221385e6f1f5bae89f491b58d8bc79f202b724
6ecc41d126 Return error when constructing pubkey from slice (Tobin C. Harding)
Pull request description:
This PR fixes a bug introduced by me in #2473, and uncovered by #2563 - amazing that it was found so quickly!
Constructing a pubkey using `PublicKey::from_slice` can fail for reasons other than just incorrect length - we should not be using `expect` but rather returning the error.
A purist might argue that we are now returning a nested error type with an unreachable variant:
`ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)`
Is this acceptable or do we want to further improve this?
ACKs for top commit:
sanket1729:
ACK 6ecc41d126
apoelstra:
ACK 6ecc41d126
Tree-SHA512: ae8299b21c4787a104f98533105308e8e7678cd5a29b78c30012982d741c05ba5f2bb1edd1d61d3a5ce028235d18c1511e1f94207479bc19e88cfec7a7ca1737
Tobin C. Harding
Andrew Poelstra
Jose Storopoli
matthiasdebernardini
Fmt Bot
Divyansh Gupta