In preparation for moving the `Witness` oven to `primitives` use the
`len` function instead of accessing the `witness_elements` field.
No logic change, `Witness::len()` returns `witness_elements`.
da0795e590 primitives: Use doc links for OutPoint (Tobin C. Harding)
b079cbafee Move OutPoint to primitives (Tobin C. Harding)
f5c46cd411 Introduce OutPoint extension traits (Tobin C. Harding)
7e5bd5048d Remove docs on deprecated is_null function (Tobin C. Harding)
97b20a2316 Add additional impl block to OutPoint (Tobin C. Harding)
Pull request description:
Just the minimal move of `OutPoint` to `primitives`.
The last patch closes#3347
ACKs for top commit:
apoelstra:
ACK da0795e590 successfully ran local tests
Tree-SHA512: dced5a6d6bc6af0ce8b4ae4e52c04b45c85eb77a24bb25762ba8ab7deeab1e6b392cc4b258bb14218e8a2af999715dfed45ba94599cb16963a843995a7475917
5fab6b178f Rename iter len unit test (Tobin C. Harding)
f6a74ef4af Refactor the serde Witness unit tests (Tobin C. Harding)
9860453b5b Improve Witness consensus encode unit test (Tobin C. Harding)
7e2899d310 Improve Witness::push unit test (Tobin C. Harding)
fe967279e5 Improve witness unit tests for single empty element (Tobin C. Harding)
Pull request description:
In preparation for moving the `Witness` type over to `primitives` refactor and improve all the unit tests that will be moved, do not touch the ones that will stay behind.
The first five patches are from #3406, the last is just a re-name of the test function I tried to refactor in ac6fe3a881
ACKs for top commit:
apoelstra:
ACK 5fab6b178f successfully ran local tests
Tree-SHA512: bc00f81e3c5cc92ae58dd2fc876d368a487ae6c08cc0735d7227c3a89287e321dbfb5b571b951d0616af0ec7cf9a0ea2d0e724645b1c419933a212ece80a0fbf
2d8c613340 Move the block hash types to primitives (Tobin C. Harding)
6b9429ac7b Remove BlockHash::all_zeros (Tobin C. Harding)
20d8dbd586 Add missing line of whitespace (Tobin C. Harding)
Pull request description:
As an initial step in moving the `block` module, just move over the hash types `BlockHash` and `WitnessCommitment`.
Patch 2 introduces an associated const `BlockHash::GENESIS_PREV_BLOCKHASH` and removes `all_zeros`.
ACKs for top commit:
apoelstra:
ACK 2d8c613340 successfully ran local tests
Tree-SHA512: 64aa0ae81e1c8ab1b5d4cd8cd28e6ef04ed01bf79175dc5b1fd607a6f0967e06b0aaf4c10ad368e2b327edcad3705187b6643d5ca8647716319424f19a838ba1
9ded58fc99 Move merkle_tree hash types to primitives (Tobin C. Harding)
Pull request description:
In preparation for moving the `block::Header` struct over to `primitives` move the `merkle_tree` hash types.
ACKs for top commit:
apoelstra:
ACK 9ded58fc99 successfully ran local tests
Tree-SHA512: 98017cf0403871f01a6efeda22e8f319cc8104b9bc2f3a9bae2d6a31f6df8172307466c6486a9259204166933137fa03e565e08a0c156c278cfeb72cdae09b89
18d8b0e469 Replace VarInt type with ReadExt and WriteExt functions (Steven Roose)
003db025c1 Return encoded length from WriteExt::emit_slice (Steven Roose)
Pull request description:
This the meat and potatoes out of Steven's work in #2133 and also closes#1016
ACKs for top commit:
apoelstra:
ACK 18d8b0e469 successfully ran local tests
Tree-SHA512: 2df96c91e0fbfdc87158bde9bbdd9565f67e3f66601697d0e22341416c0cd45dd69d09637393993f350354a44031bead99fd0d2f006b4fc6e7613aedc4b0a832
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
Move the `OutPoint` type and associated code over to `primitives`. Take
the opportunity to re-order the code so the file reads like a story,
things are implemented below where they are called.
Our `define_extension_trait` macro cannot handle examples in rustdocs,
since these docs are on the deprecated `OutPoint::is_null` function just
remove them.
In preparation for moving the `OutPoint` to `primitives` add an
additional impl block that holds everthing that will stay here in
`bitcoin`.
Internal change only.
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
Rust convention is to not use `test_` prefix on unit tests. Also this
unit test is testing that the `ExactSizedIterator` trait is implemented
and working.
Re-name unit test to `exact_sized_iterator`.
In preparation for moving unit tests to `primitives` give the serde
tests some love by doing:
- Split them up to do one thing only
- Round trip arbitrary witness
- Use better names
Make an effort to clean up the encoding unit test, by doing:
- Remove element accessor assertions (tested already above)
- Add roundtrip encoding assertion
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
The current unit test is incorrect, the indices field of a witness with
a single element starts at 1 because 0 is encode as a single
byte (compact encoded integer).
Fix the debug test and add a test that pushes an empty slice.
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>
Tobin C. Harding
merge-script
Jiri Jakes
Fmt Bot
Christian Lewe
Shing Him Ng
yancy
Chris Hyunhum Cho
Steven Roose