2d8c613340 Move the block hash types to primitives (Tobin C. Harding)
6b9429ac7b Remove BlockHash::all_zeros (Tobin C. Harding)
20d8dbd586 Add missing line of whitespace (Tobin C. Harding)
Pull request description:
As an initial step in moving the `block` module, just move over the hash types `BlockHash` and `WitnessCommitment`.
Patch 2 introduces an associated const `BlockHash::GENESIS_PREV_BLOCKHASH` and removes `all_zeros`.
ACKs for top commit:
apoelstra:
ACK 2d8c613340 successfully ran local tests
Tree-SHA512: 64aa0ae81e1c8ab1b5d4cd8cd28e6ef04ed01bf79175dc5b1fd607a6f0967e06b0aaf4c10ad368e2b327edcad3705187b6643d5ca8647716319424f19a838ba1
9ded58fc99 Move merkle_tree hash types to primitives (Tobin C. Harding)
Pull request description:
In preparation for moving the `block::Header` struct over to `primitives` move the `merkle_tree` hash types.
ACKs for top commit:
apoelstra:
ACK 9ded58fc99 successfully ran local tests
Tree-SHA512: 98017cf0403871f01a6efeda22e8f319cc8104b9bc2f3a9bae2d6a31f6df8172307466c6486a9259204166933137fa03e565e08a0c156c278cfeb72cdae09b89
18d8b0e469 Replace VarInt type with ReadExt and WriteExt functions (Steven Roose)
003db025c1 Return encoded length from WriteExt::emit_slice (Steven Roose)
Pull request description:
This the meat and potatoes out of Steven's work in #2133 and also closes#1016
ACKs for top commit:
apoelstra:
ACK 18d8b0e469 successfully ran local tests
Tree-SHA512: 2df96c91e0fbfdc87158bde9bbdd9565f67e3f66601697d0e22341416c0cd45dd69d09637393993f350354a44031bead99fd0d2f006b4fc6e7613aedc4b0a832
de319670ae feat: Create relative lock times at compile time (Christian Lewe)
53e1fb6b0c feat: Create absolute lock time at compile time (Christian Lewe)
Pull request description:
I noticed that I cannot create lock times as compile-time constants. This PR tries to remedy this issue by marking lock time constructors as `const`.
Because the `internals` crate depends on the `units` crate in a way that I don't fully understand yet, this PR updates the `units` crate only.
If `from_consensus` is being kept non-`const` by design, to keep the API flexible to future changes, then please close this PR. In this case, I overlooked existing discussions.
ACKs for top commit:
apoelstra:
ACK de319670ae successfully ran local tests; good start; should backport
tcharding:
ACK de319670ae
Tree-SHA512: 69f9147707bcf8f91f755dd6d1be5ed08945e775ee46918e33d77a9d07ce474047a80ed1226134a3914ead51d1ddbbc657552ca934dc3c079b92ad3d50b13152
e215a39dba Improve documentation of Xpub::from_xpriv (Jiri Jakes)
0dcba98382 Add Xpriv::to_xpub (Jiri Jakes)
5a9341bfc5 Improve naming of methods on Xpub and Xpriv (Jiri Jakes)
Pull request description:
Adds `Xpriv::to_xpub` and makes naming of methods related to extended and non-extended keys on `Xpub` and `Xpriv` consistent and easier to discover:
- if method takes or returns extended key, its name uses `xpriv` or `xpub`
- if method takes or returns non-extended key, its name uses `public_key` or `private_key`
Previous naming of the methods was confusing and unclear and this PR deprecates them.
Closes#3327.
### Notes for reviewers
- `xpriv` and `xpub` could be without `x`, I opted for this version to remove any ambiguity
- also considered `raw` or similar name (as suggested) for methods with non-extended keys, however `raw` is usually not used in this context and `to_public_key` vs. `to_x_only_public_key` did not look alright with any other name
- if splitting this PR into two would be desirable, please let me know
- is there a reason why `ckd_priv` is private and `ckd_pub` is public?
ACKs for top commit:
apoelstra:
ACK e215a39dba successfully ran local tests
tcharding:
ACK e215a39dba
Tree-SHA512: f76b22740b06df80dd9d01fbb991149243e47619e0bd5f1699b446456259cc6e97ecb9fca7b4881e921a9d7341ca92c6ee2dae90a417f702aff5eb760439ca42
This change makes method names on Xpub and Xpriv more consistent and
easier to discover by following two patterns:
- if the method deals with extended key, it contains 'xpub' or
'xpriv' in its name
- if the method deals with non-extended key, it contains
'public_key' or 'private_key'
One exception is 'ckd_*' methods, which are lower-level and their names
come from BIP32; these keep using 'priv' and 'pub'.
Also mark these methods as const. Because <u32 as From<u16>>::from
is not available in const contexts, I had to cast u16 as u32. I try to
avoid casts as much as I can, but in this case a cast seems unavoidable.
Casting u16 as u32 should be safe on all architectures.
Mark the from_consensus and to_consensus methods of the absolute
lock time structs as const. In theory. these methods do some sanity
checking and wrap a u32 value in a newtype. This should be possible
to do in const. Marking the methods as const should not break existing
call sites.
74a992a5c4 Implement Arbitrary for signature types (Shing Him Ng)
Pull request description:
Implementing `Arbitrary` for some signature types in preparation for #3366
ACKs for top commit:
tcharding:
ACK 74a992a5c4
apoelstra:
ACK 74a992a5c4 successfully ran local tests
Tree-SHA512: 8bfab4d70b6c8d51374177764323fe4b2bac5c0ac25ef39162fb95040948f27465f488df2094488aab28bdb9656521269b65f8ad7952ed2c5f91589bce084560
d39334e651 Refactor Arbitrary for Sequence (yancy)
Pull request description:
Personally, I think this is a bit nicer and easier to reason about.
ACKs for top commit:
tcharding:
ACK d39334e651
apoelstra:
ACK d39334e651 successfully ran local tests
Tree-SHA512: 9816e81873c7c052e7204877f3cfc605f20feb399575af4c4527899bbdfe20031dd06d681539fe21226b4926891d1a69c5c3299976e553c7c75878e07f5ed068
9d9872711f Refine Sequence and Version Arbitrary impls (Shing Him Ng)
Pull request description:
Refine `Sequence` and `Version` `Arbitrary` impls with an equal weighting for special cases and the arbitrary case
Idea originally posted here: https://github.com/rust-bitcoin/rust-bitcoin/pull/3363#issuecomment-2352946108
ACKs for top commit:
tcharding:
ACK 9d9872711f
apoelstra:
ACK 9d9872711f successfully ran local tests
Tree-SHA512: 3274e4423515009187abc5d7e8dd0390b2fe4295c674ae4d784981fa2ac698f63ee69453ecbb9d6d36d6ff421753b38f7148fa41b11bc5460673a603ccfeb8c5
a33bcd3654 test: ensure push_int check i32::MIN of overflow error (Chris Hyunhum Cho)
c9988ba8cb refactor: use match for OP_N push in push_int_unchecked (Chris Hyunhum Cho)
Pull request description:
Follow up https://github.com/rust-bitcoin/rust-bitcoin/pull/3392c9988ba8cb
- refactor `push_int_unchecked` with match expression for cleaner code(many thanks for tcharding https://github.com/rust-bitcoin/rust-bitcoin/issues/3407).
a33bcd3654
- ensure newly introduced safe `push_int` function as expected, testing if returns `Error::NumericOverflow` when `n` is `i32::MIN`
ACKs for top commit:
tcharding:
ACK a33bcd3654
apoelstra:
ACK a33bcd3654 successfully ran local tests
Tree-SHA512: 14f19d37f35b47e148b40c5017f0270c534c136d86be0c061cb476e1693130c5fc1bfc45a6f7c75a473022490c5f4e061cbc02640b1a616619ae721116e3cd54
216f6e3c1b Automated update to Github CI to rustc nightly-2024-09-25 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 216f6e3c1b
Tree-SHA512: c892afaa2424245eaf0fe914e4c2659cd2cf26001512d1c907d9b11e984532772741b06123925b3df8e74c7c62d707efcd43bbbbf007a103f6a34c6bfa9bf193
b8dab65c3d CI: Use latest run_task version (Tobin C. Harding)
Pull request description:
Recently we discovered a bug in the `run_task` script where we forgot to use `--no-default-features` in a subset of test runs (the feature matrix stuff).
Upgrade to use the latest version of `run_task.sh`.
ACKs for top commit:
apoelstra:
ACK b8dab65c3d successfully ran local tests; nice
Tree-SHA512: 9b8e6eba81a81658a98f0c8e355f97f2b27ee9a730cb3b008d0c60e81a308cdb2af11587a5e63fe2a9fc35fedef0a9e00f8b721b91cf182214dbe738f8315bec
33edaf935d fix: check overflow for push_int with push_int_unchecked (Chris Hyunhum Cho)
876146154b refactor: use push_lock_time instead of push_int (Chris Hyunhum Cho)
Pull request description:
Fix the issue https://github.com/rust-bitcoin/rust-bitcoin/issues/1530. In the discussion of https://github.com/rust-bitcoin/rust-bitcoin/issues/1530, the suggested solution is to implement `ScriptInt` type to embrace the various type of integer(`i32, u32, i64, u64, i128, u128, isize, usize...`) to support both script number and locktime number.
However, as `push_locktime` and `push_sequence` implemented, there’s no need to support `u32` of lock time for `push_int` anymore. Therefore, I’ve just changed the type of parameter to `i32`, and only check if it’s `i32::MIN`(which overflows 4 bytes sign-magnitude integer).
~I also added push_uint method to use internally for `push_locktime` and `push_sequence`, which have a dependency on `push_int` method.~
UPDATE: also add `push_int_unchecked` for those who want to push the integer out of range(and helper for `push_locktime` and `push_sequence`, which has the same functionality of former `push_int`.
ACKs for top commit:
tcharding:
ACK 33edaf935d
Tree-SHA512: 89b02bd3faf1e0a1ed530b7210250f0db33886d2acd553d07761f4aef0bb6388b22ddc06a88de05acfe465305db4cf34822fb6547576aae2aa224b4d0045fa07
d0a30096b4 transaction: Remove Default implementations (Tobin C. Harding)
Pull request description:
Currently we provide `Default` implementations for a couple of types in the `transaction` module, the values returned are meaningless and it seems these impls were added to make writing test code easier. In hindsight this was the wrong thing to do.
Break the API and remove the `Default` implementations for `OutPoint` and `TxIn`.
Add an associated const `TxIn::EMPTY_COINBASE` that is, as the name suggests, an empty transaction input with the prevout set to all zeros as for the coinbase transaction.
ACKs for top commit:
apoelstra:
ACK d0a30096b4 successfully ran local tests
Tree-SHA512: f76c2984d5f57fb6cb0792b4feb555434dd4ad3439926d30fe1be42432ace990888ad61d724fe701cb30b6f201f66b4670b7c59cc8de82d7a61ee71883a7fbd7
8b9d657667 hashes: Remove unused file (Tobin C. Harding)
Pull request description:
Recently we moved the tests from `impls` to `tests/io.rs` but forgot to delete the original file.
Remove the already unused `impls.rs` file.
ACKs for top commit:
apoelstra:
ACK 8b9d657667 successfully ran local tests
Tree-SHA512: f701825239aff18faa0cd7adf69e1a8830dfb532df9d52c6d545e6352054ae6cb8c625c9fc05b7db374822d3478acf16858bc38ffbecf45575f555eb80f010e1
cb2146d5fa Implement FeeRate checked_sub (yancy)
212a751929 Implement FeeRate checked_add (yancy)
c967eabd43 Implement FeeRate SubAssign (yancy)
c3a8bfa98d Implement FeeRate AddAssign (yancy)
0e70870056 Implement FeeRate subtraction (yancy)
86359fe364 Implement FeeRate addition (yancy)
Pull request description:
I can't think of a reason to not support addition and subtraction with `fee_rate`. If two fee rates are added, the resulting units `sat/wu` units are viable.
Without this, I have to do some shenanigans like:
```rust
let tmp = fee_rate.to_sat_per_kwu();
fee_rate = FeeRate::from_sat_per_kwu(tmp - 1);
```
ACKs for top commit:
apoelstra:
ACK cb2146d5fa successfully ran local tests
tcharding:
ACK cb2146d5fa
Tree-SHA512: 582c5e75e836df4a8aa00b928a9e81a04ba8c14a8986fac323f608d05468531b17034f42db23af57468b3220151bb6b37b90da4f1621571f011b89491070c619
Recently we removed the `all_zeros` function from `OutPoint` in favour
of a more meaningfully named associated const. We can do the same for
`BlockHash`, the all zeros has is used for the previous blockhash of the
genesis block, add a const named as such.
In test code where we use the `all_zeros` function, just use the more
explicit form `from_byte_array([0; 32])`.
Currently we provide `Default` implementations for a couple of types in
the `transaction` module, the values returned are meaningless and it
seems these impls were added to make writing test code easier. In
hindsight this was the wrong thing to do.
Break the API and remove the `Default` implementations for `OutPoint`
and `TxIn`.
Add an associated const `TxIn::EMPTY_COINBASE` that is, as the name
suggests, an empty transaction input with the prevout set to all
zeros as for the coinbase transaction.
At some stage we named the compact encoding `VarInt` (which makes sense
because the compact size encoding is a variable length integer encoding).
However it turns out the term "varint" is used in Core for a different
encoding so this may lead to confusion.
While we fix this naming thing observe also that the `VarInt` type is
unnecessarily complicated, all we need to be able to do is encode and
decode integers in compact form as specified by Core. We can do this
simply by extending our `WriteExt` and `ReadExt` traits.
Add `emit_compact_size` and `read_compact_size` to emit and read compact
endcodings respectively.
Includes addition of `internals::compact_size::encoded_size_const`.
Patch originally written by Steven, Tobin cherry-picked and did a bunch
of impovements after the varint vs compact_size thing (#1016).
ref: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
Co-developed-by: Tobin C. Harding <me@tobin.cc>
We would like to add a `emit_varint` function, however doing so requires
that we can get access to the length of a slice when we are encoding it
so we can use `emit_slice` to implement `emit_varint`. It would be
easier to do so if `emit_slice` returned the length of the slice.
In preparation for adding `emit_varint` (and removing the `VarInt` type)
return the encoded length of a slice from `WriteExt::emit_slice`.
(Patch originally written by Steven, cherry-pick and patch description
written by Tobin.)
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Recently we discovered a bug in the `run_task` script where we forgot to
use `--no-default-features` in a subset of test runs (the feature matrix
stuff).
Upgrade to use the latest version of `run_task.sh`.
58704c2eff Remove schemars all together (Tobin C. Harding)
Pull request description:
We introduced schemars as a personal favor to a user, and it broke our CI repeatedly but eventually it seemed like it was stable (mainly, our MSRV caught up with its MSRV) so we just let it slide. In the end having schemars on hashes but nowhere else in the rust-bitcoin ecosystem did not prove that useful.
Remove schemars all together.
Fix: #3393
ACKs for top commit:
apoelstra:
ACK 58704c2eff successfully ran local tests
Tree-SHA512: 11c136797f28903c7d6b5199ad55d86bc4bc29ee8dd6f0d575e029f4dbebebabed57ebce6cf773b286297ea84f18d0b6cc58e150299e99457e048226478b49cc
7d2c57d345 Soften wording on MSRV (Tobin C. Harding)
Pull request description:
Currently we use the word "always" in the MSRV docs, we mean to say "there are no set of features that will break the build" but this usage of "always" may be interpreted as "always, for all time" which is definitely not what we mean.
Soften the wording on MSRV by removing the word always.
ACKs for top commit:
apoelstra:
ACK 7d2c57d345 successfully ran local tests; will one-ACK merge because it's trivially correct
Tree-SHA512: 8d85d748e90e8082861005e6787d1c6be95745ebb442c98f638c5efb9940fc5ac7860d786b47bff88ba59c17a2a8c115d2c740108772dd63dc24f3e911555473
cbfddb0394 hashes: Rename length field and use u64 (Tobin C. Harding)
Pull request description:
The hash engine types have a `length` field that is used to cache the number of bytes hashed so far, as such it is an arbitrary number and could use a `u64` instead of `usize`.
While we are at it rename `length` to `bytes_hashed` to remove any ambiguity of what this field is. Note this field is private, we already have the public getter `n_bytes_hashes` to get the value.
Introduce a private function `incomplete_block_size`, the purpose of this function is to put all the casts in one place so they can be well documented and easily understood.
Fix: #3016
ACKs for top commit:
apoelstra:
ACK cbfddb0394 successfully ran local tests
Tree-SHA512: a9d932938afcbd6dfb9db471a02fa7e3fff8f0659906627001ad241390b9af57088fd34afeae551c70c2c49783e6296f110b57ff9de6fed2609f4648ec8fd934
18110a51f2 Bump version of internals to 0.4.0 (Tobin C. Harding)
Pull request description:
In preparation for releasing `internals v0.4.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 `internals`.
ACKs for top commit:
apoelstra:
ACK 18110a51f2 successfully ran local tests; lots of nice stuff here
Tree-SHA512: a4d3d5279b7d7fa993cbc3b7b34fc6dc4024dd54c0bfa1ecd0f0d5f09b984871f156c3695092a1f6c44b7571f8b2051699040f5f77636d44d4cae6c972ab597f
184dfbcdb6 primitives: Move optional dependency to correct place (Tobin C. Harding)
Pull request description:
We keep optional deps in a separate group to non-optional deps, move the new `arbitrary` dependency.
Internal change only.
ACKs for top commit:
shinghim:
ACK 184dfbcdb6
apoelstra:
ACK 184dfbcdb6 successfully ran local tests
Tree-SHA512: 8540ee9ce9663d60d1a8d08e94738c7a3e3341b640ec6cbc04c304cb12daa4c95ae6cd1e4ad9fde361065510b2e34c65f6f08db1ae305976fce4525aa98756a3
Currently we use the word "always" in the MSRV docs, we mean to say
"there are no set of features that will break the build" but this usage
of "always" may be interpreted as "always, for all time" which is
definitely not what we mean.
Soften the wording on MSRV by removing the word always.
merge-script
Jiri Jakes
Fmt Bot
Christian Lewe
Shing Him Ng
yancy
merge-script
Chris Hyunhum Cho
Steven Roose