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`.
d649c06238 Move script types to primitives (Tobin C. Harding)
ec4635904b Inline bytes_to_asm_fmt into Script::Display impl (Tobin C. Harding)
Pull request description:
First patch removes `bytes_to_asm_fmt` as requested by Kix here: https://github.com/rust-bitcoin/rust-bitcoin/pull/3194#discussion_r1756557768
Second patch does the move. The move is minimal but there is quite a bit of code moved in `script/mod.rs` - I believe it is as minimal as required as well.
ACKs for top commit:
apoelstra:
ACK d649c06238 successfully ran local tests
Tree-SHA512: 329a23948ac5617402a724b734d81cde8ab1f57ddd4860f858880618e260ea8b5cc89315de1fd93ae32787d5e8508fd604a41f003b1f5772a773b5b1648d382c
c41a6e9b1b feat: add sign fn for sign_message (ChrisCho-H)
Pull request description:
While it's not hard to create the signature using `secp256k1` modules with `signed_msg_hash`, it's much more convenient and safe to provide one-way function to generate signed signature(even without the understanding about the semantics of bitcoin message signing).
ACKs for top commit:
apoelstra:
ACK c41a6e9b1b successfully ran local tests
tcharding:
ACK c41a6e9b1b
Tree-SHA512: 84caea275059381040c71100badb54556dd105722f79ac43d2df7eb0e5428cf8e7acc2d7f262625dd008837099a928c3c8be858f9ab11838c2eec1786e9f1844
8f79a0560e Remove unused import (yancy)
Pull request description:
Ran `./maintainer-tools/ci/run_task.sh stable` without error locally, so I don't think this import is needed.
ACKs for top commit:
tcharding:
ACK 8f79a0560e
apoelstra:
ACK 8f79a0560e successfully ran local tests
Tree-SHA512: 255b97066eb8b75bf33a0846ecda739695441273ff98703516b46983a9fe5c76e76c79ab2548bd64fd654dd8659ea5392b8079142b447e67a6a31d75883aa8d1
3565f70df9 feat: replace ENABLE_RBF_NO_LOCKTIME with ENABLE_LOCKTIME_AND_RBF (ChrisCho-H)
Pull request description:
follow up https://github.com/rust-bitcoin/rust-bitcoin/pull/3455.
Replace all `ENABLE_RBF_NO_LOCKTIME`(deprecated) with `ENABLE_LOCKTIME_AND_RBF`
ACKs for top commit:
apoelstra:
ACK 3565f70df9 successfully ran local tests
tcharding:
ACK 3565f70df9
Tree-SHA512: 4c6c6ad9ac89efe042cf239ce68594e715dbc827c1ae430819e2f16d2191f82d81f1e55b348f8176fc21454cf9d25fc9da1a414982cd3483d5f9f39834441f6c
9e6b8faf84 feat: add version three variant to transaction version (Rob N)
Pull request description:
Topologically restricted transactions are now considered standard as of Bitcoin 28.0.
ACKs for top commit:
apoelstra:
ACK 9e6b8faf84 successfully ran local tests
tcharding:
ACK 9e6b8faf84
Tree-SHA512: 289b986304e206802f04cee6607087167f6d91d8a81d4fc49ed01c430f4f6ad00b44646fbefdd000148fc5bfe2d257f92b386bfaf4405c482e4e438d830ab586
a250c8eee4 Fix unused imports (Shing Him Ng)
Pull request description:
Found some unused imports while working on something unrelated
ACKs for top commit:
apoelstra:
ACK a250c8eee4 successfully ran local tests
tcharding:
ACK a250c8eee4
Tree-SHA512: ffa4fcddb91c849df7885a6d734405ab704e0353293fa3462a981e179ea49091990df4482eee2324e7b5ef250a0890b2e8983d53ec288ca6e8ada77e30c102dc
0c824c9c68 Implement Arbitrary for Block (Shing Him Ng)
Pull request description:
Implementing `Arbitrary` for `Block` and its child types
ACKs for top commit:
apoelstra:
ACK 0c824c9c68 successfully ran local tests
tcharding:
ACK 0c824c9c68
Tree-SHA512: 407acd4155ca7496bf7c59af19f103b22b4c74dd013c2e2c42aae690ba7264ecd42ed034355cf8895f2532f8cef77dfdbeac250301a6973afd5f3920be7e4310
323e706113 Add rustfmt config option style_edition (Tobin C. Harding)
2e4179ed0f Run the formatter (Tobin C. Harding)
2c40b4f4ec Configure formmater to skip read_compact_size (Tobin C. Harding)
Pull request description:
`rustfmt` is emitting:
Warning: the `version` option is deprecated. Use `style_edition` instead.
As suggested add a config option and set it to 2021.
- Patch 1: Manually configure rustfmt to skip some code
- Patch 2: Run the formmater with current configuration
- Patch 3: Add the new config option (remove old one), introduces no new formatting requirements
ACKs for top commit:
apoelstra:
ACK 323e706113 successfully ran local tests
Tree-SHA512: 7f80cc89f86d2d50936e51704344955fa00532424c29c0ee3fae1a6836e24030f909b770d28da13e1c5efde3d49ad7d52c6d909d120fb09c33abf1755f62cd38
3b7ba4f977 Remove the SliceIndex implementation from hash types (Tobin C. Harding)
Pull request description:
If folk really want to index into a hash they can us `as_byte_array` then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this is because otherwise `secp` won't build since we are breaking an API that is used in the current release of secp.
Fix: #3115
ACKs for top commit:
apoelstra:
ACK 3b7ba4f977 successfully ran local tests
Tree-SHA512: 0ba93268cd8133fe683183c5e39ab8b3bf25c15bfa5767d2934d67a5f6a0d2f65f6c9304952315fe8a33abfce488d810a8d28400a28facfb658879ed06acca63
70264bfcec Move block::Version to primitives (Tobin C. Harding)
819d8d72e8 Stop using private Version constructor (Tobin C. Harding)
Pull request description:
This is a straight up move of the whole type because there are only three methods, a getter, a setter, and `is_signalling_soft_fork`.
If we use an extension trait for `is_signalling_soft_fork` then we have to make the two private associated consts public which is in my opinion worse.
Patch 1 is preparation, use getter/setter, patch 2 does the move.
ACKs for top commit:
apoelstra:
ACK 70264bfcec successfully ran local tests
Tree-SHA512: b61e853b4b96cb1cc56c7bfb67cc6c3ba7c631cb9e540393eb780dcf63bd2d934058794f2ac0145b4b5404a6a68887c3a1d225b2d87b57415474c05d3ef2811f
Currently we use `u8` for key type but it was pointed out that we should
be using a `u64` and encoding it as a compact type. The reason our code
works now is because the compact type encoding for a `u8` (less than
253) is the same as for a `u8`.
This breaks the `serde` impl, as shown by changes to the regression tests.
aa1fd5f44c Update the doc for InputWeightPrediction::weight() (spacebear)
8fd53c8ecf Add nested P2WPKH helpers to InputWeightPrediction (spacebear)
Pull request description:
Following up on https://github.com/rust-bitcoin/rust-bitcoin/issues/630#issuecomment-2392050517.
I amended the docstring on `InputWeightPrediction::weight()` to clarify that it only returns the signature weight. I'm not sure what the rationale was for only returning partial input weight, and if there are any arguments against something like `InputWeightPrediction::total_weight()` which returns `self.weight() + Weight::from_non_witness_data_size(32 + 4 + 4)` (and maybe deprecate `weight()` eventually to prevent misuse).
ACKs for top commit:
apoelstra:
ACK aa1fd5f44c successfully ran local tests
tcharding:
ACK aa1fd5f44c
Tree-SHA512: 9bdd882164a80a1a1a44c593750aa59e470f2928c0380d4d53a0409add597844cf11945785264ebd6b281029b3f2ef1be6c77e1e9cd148192dbd601df30a60ae
2424b654d5 feat: rust-bitcoin supports testnet4 (BinChengZhao)
afa91a2030 Introduce TestnetVersion enum with only TestnetV3 (BinChengZhao)
Pull request description:
Fixed: https://github.com/rust-bitcoin/rust-bitcoin/issues/2749
1. Adjust `Network` to support `Testnet4`. (Based on https://github.com/rust-bitcoin/rust-bitcoin/issues/2749#issuecomment-2152366608 as an implementation idea.)
2. Handle conflicting procedure macros and declaration macros, since the original macros can't handle the new `Network::Testnet(TestnetVersion)` structure properly, and make internal implementations compatible with previous versions (e.g., `testnet` for `Testnet3`, `testnet4` for `Testnet4`, to minimize user-side effects).
3. add `Testnet4` related Params, Block.
4. optimize compatibility test cases.
5. Regarding `fn: genesis_block`, calling `bitcoin_genesis_tx` with any Network type internally gets a reasonable `merkle_root`, but not `Testnet4`. (By comparison, I confirmed that the built-in transaction data provided is correct, and combined with the fixed `merkle_root` of `Testnet4` the correct block hash can be computed, which is not possible if the transaction data is incorrect.) **I'd appreciate it if some developer could guide me on what the problem is, and I'll work on it.**
Of course, all these changes are based on https://github.com/rust-bitcoin/rust-bitcoin/issues/2749 , and the consensus of the discussion, if you have any comments, please leave a message, I will actively improve, and promote the support of `Testnet4`.
ACKs for top commit:
tcharding:
ACK 2424b654d5
apoelstra:
ACK 2424b654d5 successfully ran local tests
Tree-SHA512: a501f0a320460afdad2eb12ef6ff315b6357b7779dfe7c7028ea090da567019c947599006a06b3834265db1481129752f015629ef72fea6f862d01323ce9d024
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`.
If folk really want to index into a hash they can us `as_byte_array`
then index that.
Includes a bump to the version number of `hashes` to `v0.15.0` - this
is because otherwise `secp` won't build since we are breaking an API
that is used in the current release of secp.
Fix: #3115
In preparation for releasing `io v0.2.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 `io`.
This is a straight up move of the whole type because there are only
three methods, a getter, a setter, and `is_signalling_soft_fork`.
If we use an extension trait for `is_signalling_soft_fork` then we
have to make the two private associated consts public which is in my
opinion worse.
In preparation for moving the `block::Version` type over to `primitives`
stop using the private constructor and inner field. Use the public
getter/setter instead (`to_consensus`and `from_consensus` respectively).
07a529a132 Bump version of bitcoin-units to 0.2.0 (Tobin C. Harding)
148711a4c6 units: Use double ## in changelog entries (Tobin C. Harding)
80e600ba0c units: Copy 0.1.2 release notes (Tobin C. Harding)
Pull request description:
In preparation for releasing `units v0.2.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 `units`.
Close: #3095
ACKs for top commit:
apoelstra:
ACK 07a529a132 successfully ran local tests
Tree-SHA512: 98a75d485ded6225551a5fc4b4a14d8efecc76911a720f959044cdd62781024a8787f258f171ed297705f5ab470f9a88a81ad5d255c9e03c1e22857615ad2e6d
3f3f30d6c7 Use iter instead of accessing content field (Tobin C. Harding)
6389d1cbb3 Stop using push_slice (Tobin C. Harding)
be163eec99 Use Witness::len instead of accessing field (Tobin C. Harding)
Pull request description:
Prepare `Witness` to be moved to `primitives`.
This is the first three patches out of #3406. Patch 1 and 2 are internal changes, path 3 is also internal but introduces a slight perf hit by doing multiple writes.
ACKs for top commit:
apoelstra:
ACK 3f3f30d6c7 successfully ran local tests
Tree-SHA512: 25e2570a22797dbfa15d6e5af9b72938243192ca264bc5fe82c2f327486e52a73993b3b61ee1161e5d17b01a7f9843960cb4031e6550561de4ecafb9f935e375
We would like to move the `Witness` to `primitives` however in the
`Encodable` implementation we are currently accessing the private
`content` field.
Instead of accessing `content` we can iterate over the witness elements
and write each individually, this has the same result but does a bunch
of additional calls to `Write::write_all` (via `emit_slice`).
This patch effects performance negatively but makes no changes to the
encoding.
The `Witness::push_slice` function is called by `Witness::push` after
calling `as_ref`, hence is equivalent for all types that implement
`AsRef<[u8]>`. Also, `push_slice` is a private method on `Witness`.
In preparation for moving `Witness` over to `primitives` stop using
`push_slice` in favour of `push`.
Internal change only.
In preparation for moving the `Witness` oven to `primitives` use the
`len` function instead of accessing the `witness_elements` field.
No logic change, `Witness::len()` returns `witness_elements`.
da0795e590 primitives: Use doc links for OutPoint (Tobin C. Harding)
b079cbafee Move OutPoint to primitives (Tobin C. Harding)
f5c46cd411 Introduce OutPoint extension traits (Tobin C. Harding)
7e5bd5048d Remove docs on deprecated is_null function (Tobin C. Harding)
97b20a2316 Add additional impl block to OutPoint (Tobin C. Harding)
Pull request description:
Just the minimal move of `OutPoint` to `primitives`.
The last patch closes#3347
ACKs for top commit:
apoelstra:
ACK da0795e590 successfully ran local tests
Tree-SHA512: dced5a6d6bc6af0ce8b4ae4e52c04b45c85eb77a24bb25762ba8ab7deeab1e6b392cc4b258bb14218e8a2af999715dfed45ba94599cb16963a843995a7475917
5fab6b178f Rename iter len unit test (Tobin C. Harding)
f6a74ef4af Refactor the serde Witness unit tests (Tobin C. Harding)
9860453b5b Improve Witness consensus encode unit test (Tobin C. Harding)
7e2899d310 Improve Witness::push unit test (Tobin C. Harding)
fe967279e5 Improve witness unit tests for single empty element (Tobin C. Harding)
Pull request description:
In preparation for moving the `Witness` type over to `primitives` refactor and improve all the unit tests that will be moved, do not touch the ones that will stay behind.
The first five patches are from #3406, the last is just a re-name of the test function I tried to refactor in ac6fe3a881
ACKs for top commit:
apoelstra:
ACK 5fab6b178f successfully ran local tests
Tree-SHA512: bc00f81e3c5cc92ae58dd2fc876d368a487ae6c08cc0735d7227c3a89287e321dbfb5b571b951d0616af0ec7cf9a0ea2d0e724645b1c419933a212ece80a0fbf
2d8c613340 Move the block hash types to primitives (Tobin C. Harding)
6b9429ac7b Remove BlockHash::all_zeros (Tobin C. Harding)
20d8dbd586 Add missing line of whitespace (Tobin C. Harding)
Pull request description:
As an initial step in moving the `block` module, just move over the hash types `BlockHash` and `WitnessCommitment`.
Patch 2 introduces an associated const `BlockHash::GENESIS_PREV_BLOCKHASH` and removes `all_zeros`.
ACKs for top commit:
apoelstra:
ACK 2d8c613340 successfully ran local tests
Tree-SHA512: 64aa0ae81e1c8ab1b5d4cd8cd28e6ef04ed01bf79175dc5b1fd607a6f0967e06b0aaf4c10ad368e2b327edcad3705187b6643d5ca8647716319424f19a838ba1
9ded58fc99 Move merkle_tree hash types to primitives (Tobin C. Harding)
Pull request description:
In preparation for moving the `block::Header` struct over to `primitives` move the `merkle_tree` hash types.
ACKs for top commit:
apoelstra:
ACK 9ded58fc99 successfully ran local tests
Tree-SHA512: 98017cf0403871f01a6efeda22e8f319cc8104b9bc2f3a9bae2d6a31f6df8172307466c6486a9259204166933137fa03e565e08a0c156c278cfeb72cdae09b89
18d8b0e469 Replace VarInt type with ReadExt and WriteExt functions (Steven Roose)
003db025c1 Return encoded length from WriteExt::emit_slice (Steven Roose)
Pull request description:
This the meat and potatoes out of Steven's work in #2133 and also closes#1016
ACKs for top commit:
apoelstra:
ACK 18d8b0e469 successfully ran local tests
Tree-SHA512: 2df96c91e0fbfdc87158bde9bbdd9565f67e3f66601697d0e22341416c0cd45dd69d09637393993f350354a44031bead99fd0d2f006b4fc6e7613aedc4b0a832
e215a39dba Improve documentation of Xpub::from_xpriv (Jiri Jakes)
0dcba98382 Add Xpriv::to_xpub (Jiri Jakes)
5a9341bfc5 Improve naming of methods on Xpub and Xpriv (Jiri Jakes)
Pull request description:
Adds `Xpriv::to_xpub` and makes naming of methods related to extended and non-extended keys on `Xpub` and `Xpriv` consistent and easier to discover:
- if method takes or returns extended key, its name uses `xpriv` or `xpub`
- if method takes or returns non-extended key, its name uses `public_key` or `private_key`
Previous naming of the methods was confusing and unclear and this PR deprecates them.
Closes#3327.
### Notes for reviewers
- `xpriv` and `xpub` could be without `x`, I opted for this version to remove any ambiguity
- also considered `raw` or similar name (as suggested) for methods with non-extended keys, however `raw` is usually not used in this context and `to_public_key` vs. `to_x_only_public_key` did not look alright with any other name
- if splitting this PR into two would be desirable, please let me know
- is there a reason why `ckd_priv` is private and `ckd_pub` is public?
ACKs for top commit:
apoelstra:
ACK e215a39dba successfully ran local tests
tcharding:
ACK e215a39dba
Tree-SHA512: f76b22740b06df80dd9d01fbb991149243e47619e0bd5f1699b446456259cc6e97ecb9fca7b4881e921a9d7341ca92c6ee2dae90a417f702aff5eb760439ca42
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.
This change makes method names on Xpub and Xpriv more consistent and
easier to discover by following two patterns:
- if the method deals with extended key, it contains 'xpub' or
'xpriv' in its name
- if the method deals with non-extended key, it contains
'public_key' or 'private_key'
One exception is 'ckd_*' methods, which are lower-level and their names
come from BIP32; these keep using 'priv' and 'pub'.
Move the `OutPoint` type and associated code over to `primitives`. Take
the opportunity to re-order the code so the file reads like a story,
things are implemented below where they are called.
Our `define_extension_trait` macro cannot handle examples in rustdocs,
since these docs are on the deprecated `OutPoint::is_null` function just
remove them.
In preparation for moving the `OutPoint` to `primitives` add an
additional impl block that holds everthing that will stay here in
`bitcoin`.
Internal change only.
Rust convention is to not use `test_` prefix on unit tests. Also this
unit test is testing that the `ExactSizedIterator` trait is implemented
and working.
Re-name unit test to `exact_sized_iterator`.
In preparation for moving unit tests to `primitives` give the serde
tests some love by doing:
- Split them up to do one thing only
- Round trip arbitrary witness
- Use better names
Make an effort to clean up the encoding unit test, by doing:
- Remove element accessor assertions (tested already above)
- Add roundtrip encoding assertion
33edaf935d fix: check overflow for push_int with push_int_unchecked (Chris Hyunhum Cho)
876146154b refactor: use push_lock_time instead of push_int (Chris Hyunhum Cho)
Pull request description:
Fix the issue https://github.com/rust-bitcoin/rust-bitcoin/issues/1530. In the discussion of https://github.com/rust-bitcoin/rust-bitcoin/issues/1530, the suggested solution is to implement `ScriptInt` type to embrace the various type of integer(`i32, u32, i64, u64, i128, u128, isize, usize...`) to support both script number and locktime number.
However, as `push_locktime` and `push_sequence` implemented, there’s no need to support `u32` of lock time for `push_int` anymore. Therefore, I’ve just changed the type of parameter to `i32`, and only check if it’s `i32::MIN`(which overflows 4 bytes sign-magnitude integer).
~I also added push_uint method to use internally for `push_locktime` and `push_sequence`, which have a dependency on `push_int` method.~
UPDATE: also add `push_int_unchecked` for those who want to push the integer out of range(and helper for `push_locktime` and `push_sequence`, which has the same functionality of former `push_int`.
ACKs for top commit:
tcharding:
ACK 33edaf935d
Tree-SHA512: 89b02bd3faf1e0a1ed530b7210250f0db33886d2acd553d07761f4aef0bb6388b22ddc06a88de05acfe465305db4cf34822fb6547576aae2aa224b4d0045fa07
In preparation for releasing `units v0.2.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 `units`.
The current unit test is incorrect, the indices field of a witness with
a single element starts at 1 because 0 is encode as a single
byte (compact encoded integer).
Fix the debug test and add a test that pushes an empty slice.
Recently we removed the `all_zeros` function from `OutPoint` in favour
of a more meaningfully named associated const. We can do the same for
`BlockHash`, the all zeros has is used for the previous blockhash of the
genesis block, add a const named as such.
In test code where we use the `all_zeros` function, just use the more
explicit form `from_byte_array([0; 32])`.
Currently we provide `Default` implementations for a couple of types in
the `transaction` module, the values returned are meaningless and it
seems these impls were added to make writing test code easier. In
hindsight this was the wrong thing to do.
Break the API and remove the `Default` implementations for `OutPoint`
and `TxIn`.
Add an associated const `TxIn::EMPTY_COINBASE` that is, as the name
suggests, an empty transaction input with the prevout set to all
zeros as for the coinbase transaction.
At some stage we named the compact encoding `VarInt` (which makes sense
because the compact size encoding is a variable length integer encoding).
However it turns out the term "varint" is used in Core for a different
encoding so this may lead to confusion.
While we fix this naming thing observe also that the `VarInt` type is
unnecessarily complicated, all we need to be able to do is encode and
decode integers in compact form as specified by Core. We can do this
simply by extending our `WriteExt` and `ReadExt` traits.
Add `emit_compact_size` and `read_compact_size` to emit and read compact
endcodings respectively.
Includes addition of `internals::compact_size::encoded_size_const`.
Patch originally written by Steven, Tobin cherry-picked and did a bunch
of impovements after the varint vs compact_size thing (#1016).
ref: https://en.bitcoin.it/wiki/Protocol_documentation#Variable_length_integer
Co-developed-by: Tobin C. Harding <me@tobin.cc>
We would like to add a `emit_varint` function, however doing so requires
that we can get access to the length of a slice when we are encoding it
so we can use `emit_slice` to implement `emit_varint`. It would be
easier to do so if `emit_slice` returned the length of the slice.
In preparation for adding `emit_varint` (and removing the `VarInt` type)
return the encoded length of a slice from `WriteExt::emit_slice`.
(Patch originally written by Steven, cherry-pick and patch description
written by Tobin.)
Co-developed-by: Tobin C. Harding <me@tobin.cc>
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
46386337b0 Ignore doc compile error (Jamil Lambert, PhD)
Pull request description:
With the stricter doc tests required to pick up unused imports etc. the code under the `compile_fail` tag also creates an Error.
Changed `compile_fail` to `ignore` to remove the Error.
ACKs for top commit:
tcharding:
ACK 46386337b0
apoelstra:
ACK 46386337b0 successfully ran local tests
Tree-SHA512: 67513464dede8d4bd9f9fa4363ccb58e774b180f331edc4486290784f61e931195225765768030d2709f618ba4afe17d034d4bbc4d649a02e7a34b47b48bd297
f6abdcc001 Allow unused in `macros.rs` docs (Jamil Lambert, PhD)
fd89ddf401 Remove or fix unused variables and methods in docs (Jamil Lambert, PhD)
ff6b1d4f19 Remove unused variables and methods from docs (Jamil Lambert, PhD)
e58cda6f92 Remove `unused_imports` in docs (Jamil Lambert, PhD)
Pull request description:
As mentioned in #3362 examples in documentation are not linted in the same way as other code, but should still contain correctly written code.
#![doc(test(attr(warn(unused))))] has been added to all lib.rs files
In the docs throughout all crates:
- Unused imports have been removed.
- Unused variables, structs and enums have been used e.g. with an `assert_eq!` or prefixed with `_`
- Unused methods have been called in the example code.
ACKs for top commit:
tcharding:
ACK f6abdcc001
apoelstra:
ACK f6abdcc001 successfully ran local tests
Tree-SHA512: c3de1775ecde6971056e9fed2c9fa1621785787a6a6ccbf3a6dbd11e18d42d4956949f3f8adfc75d94fd25db998b04adb1c346cc2c2ba47f4dc37402e1388277
With the stricter doc tests required to pick up unused imports etc. the
`compile_fail` tag also creates an Error.
Changed `compile_fail` to `ignore` to remove the Error.
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout the bitcoin crate unused variables have either been prefixed
with _ or an assert used. And unused methods have been used in the
example code.
5b4e81b379 Implement Arbitrary for Transaction (Shing Him Ng)
Pull request description:
Implement `Arbitrary` for Transaction and its child types
ACKs for top commit:
apoelstra:
ACK 5b4e81b379 successfully ran local tests
tcharding:
ACK 5b4e81b379
Tree-SHA512: 411b97344e01e2bf38fff994902acda51f9a4fe748573e600fbdd643951a0a56b37395594349706b13c744304bd5396fd8ee31c27e5b1ff97e1a3cebd479e7c8
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
unused_imports in docs have been removed in bitcoin, and a warn
attribute added to lib.rs.
25d906d936 Use UFCS in macros (Liam Aharon)
Pull request description:
Closes#3304
ACKs for top commit:
apoelstra:
ACK 25d906d936 successfully ran local tests
tcharding:
ACK 25d906d936
Tree-SHA512: a02a8507bcec73dab1ec8d49e45eab327d989c276d931520e0fff312faf7d3165292c300fb9414314f9b94b1255ee49a0fb64303df6d971c9089226b6873c36a
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`.
8ec3571d80 Implement GetKey for Vec<Xpriv> (Nadav Ivgi)
Pull request description:
It appears that the `BTreeSet<Xpriv>`/`HashSet<Xpriv>` sets currently implementing `GetKey` cannot actually be constructed, because `Xpriv` does not implement `Ord` nor `Hash`. (And that the rust-bitcoin code referencing these sets should not even compile? yet evidently it does 👀 )
This PR adds support for `Vec<Xpriv>` to enable signing with multiple `Xpriv`s, but does not address the issue with the existing sets.
The added test case demonstrates the issue:
```rust
error[E0277]: the trait bound `bip32::Xpriv: std:#️⃣:Hash` is not satisfied
--> bitcoin/src/psbt/mod.rs:2301:24
|
2301 | HashSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `std:#️⃣:Hash` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::HashSet::<T, S>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/collections/hash/set.rs:888:5
error[E0277]: the trait bound `bip32::Xpriv: Ord` is not satisfied
--> bitcoin/src/psbt/mod.rs:2302:25
|
2302 | BTreeSet::new().insert(xpriv.clone());
| ^^^^^^ the trait `Ord` is not implemented for `bip32::Xpriv`
|
note: required by a bound in `std::collections::BTreeSet::<T, A>::insert`
--> /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/alloc/src/collections/btree/set.rs:899:5
```
ACKs for top commit:
apoelstra:
ACK 8ec3571d80 successfully ran local tests
tcharding:
ACK 8ec3571d80
Tree-SHA512: aceb95f8eaf11f91c6829e0b5e1c0264ebffbf587fd420145a22e924cb45678b2f4334f0b7de6ed99b57f0ce24c3d61f9e5c1e348e1b40975bc515e8fd16b75d
d942882b36 Document the magic bytes for witness commitment (Peter Ryszkiewicz)
Pull request description:
ACKs for top commit:
apoelstra:
ACK d942882b36 successfully ran local tests; neat! I don't think I was aware of this
tcharding:
ACK d942882b36
Tree-SHA512: 89766e986574e49b6c5fd00844db679eb8894a591463029f7f88079fa9ca65bd56265a42dd8fa5aad20ef4744bca3259c06f8a461abd1c695fb89433ed5cc145
b593c886e3 Support GetKey where the Xpriv is a direct child of the looked up KeySource (Nadav Ivgi)
055aa9d4dc Refactor GetKey to take the KeyRequest by reference (Nadav Ivgi)
d15c57bd1f Refactor GetKey for sets to internally use Xpriv::get_key() (Nadav Ivgi)
d25c62bf45 Fix GetKey for sets to properly compare the fingerprint (Nadav Ivgi)
Pull request description:
- The first commit is the simplest fix for a bug where the fingerprint wasn't compared correctly.
- The second & third commits are optional refactoring to reuse `Xpriv::get_key` for `$set<Xpriv>::get_key`, so the Xpriv matching logic only has to be maintained in one place.
- The forth commit adds support for signing with `Xpriv`s that are direct children of the `KeySource` -- possibly what the original (buggy) code author had in mind?
Of course, feel free to take just the first commit if the others seem unnecessary. The last one is kind of meh, not sure if really useful.
Note that multi-`Xpriv` signing does not actually work until #2850 is addressed too.
ACKs for top commit:
apoelstra:
ACK b593c886e3 successfully ran local tests
tcharding:
ACK b593c886e3
Tree-SHA512: a6f84fe12b68ddb71fc722649b1596d1561ab08a07d47f2345bd8932e155571f2b1e23c32a9c03d588fb697540340cb8ca38275dfe11f4910fd48265a993762a
70ccd6b629 signed_msg_hash takes impl AsRef<[u8]> (Liam Aharon)
Pull request description:
Closes#3249
Seems not required for Kixunil's immediate use case anymore, but figured I'd open a PR anyways.
ACKs for top commit:
apoelstra:
ACK 70ccd6b629 successfully ran local tests
Kixunil:
ACK 70ccd6b629
Tree-SHA512: b15bafd1af0749cbac36652a4e4c9ab3f91b3980bb6c3cbc1c977d339810fc15075af5d4bc6b210de364bedc496ce3759eaf4539e40d215ab66a122ba69dd56a
8f2f4cbb3c Re-order optional dependencies (Tobin C. Harding)
95f2a8dab6 Do not access ScriptBuf inner from builder (Tobin C. Harding)
900af453ff Stop accessing inner ScriptBuf field when encoding (Tobin C. Harding)
8b82363d97 Use Script::as_bytes instead of inner when indexing (Tobin C. Harding)
b0675a4a4f Use Script::len instead of inner field (Tobin C. Harding)
374c6118dc Deprecate Script::fmt_asm and to_asm_str (Tobin C. Harding)
Pull request description:
Move the `Script` and `ScriptBuf` types to `primitives`. There were still a few preparations required, things we had missed while creating the extension traits.
Note also please, in the last patch, we enable `hex` from the `serder` feature. This is not the final state we want but like we did for `alloc` it is a step to reduce the size of the diff.
ACKs for top commit:
Kixunil:
ACK 8f2f4cbb3c
apoelstra:
ACK 8f2f4cbb3c successfully ran local tests
Tree-SHA512: 62a5f3c253ecb54d95c37fdc7eb955f3952909dc3bca20444b85c44665f54d5a0c48daf729bed0dd60ff3e9571b41deed039984c8b757b075ac6e136cacd17d7
0403e52ce3 Move the transaction hash types over to primitives (Tobin C. Harding)
7e454d756d Define extension traits for txid types (Tobin C. Harding)
832b726d03 Stop using all_zeros (Tobin C. Harding)
d69c241b5c Improve docs on associated consts (Tobin C. Harding)
68c9e28165 Do not use private constructor for txid hash types (Tobin C. Harding)
98328b5a7b Use as_byte_array to encode hash type (Tobin C. Harding)
Pull request description:
Move the `Txid` and `Wtxid` hash wrapper types over to `primitives`. This introduces to `primitves` an unconditional dependency on `hashes`.
ACKs for top commit:
Kixunil:
ACK 0403e52ce3
apoelstra:
ACK 0403e52ce3 successfully ran local tests
Tree-SHA512: d14fa95bc12c2399d30d4d640b5a3fce625d51adf587a8037158f7d7e7b6288170b2d4418ca2cb68f612086ea0bdd0fae3b577f84f0d60627072fdb2217a6531
e064686397 Deprecate OutPoint::new constructor (Tobin C. Harding)
Pull request description:
The `OutPoint` type has two public fields, providing a `new` constructor that just sets these two fields adds no value.
Done after discussion in #3340 as part of `primitives` work.
ACKs for top commit:
Kixunil:
ACK e064686397
apoelstra:
ACK e064686397 successfully ran local tests
Tree-SHA512: d80cac85093946b3678883f4e3ad7fd2052d858dbd3fab4127916b0eb6153999d30827c84ccf8c6e4412fbc3842bd127a2e4d3ca0248d2307d0bca467a2555ce
f811e0adb6 Stop using deprecated OutPoint functions (Tobin C. Harding)
Pull request description:
Either our CI is failing us or `deprecated` does not work as expected, either way we should not be using the `OutPoint::null()` or `is_null` functions any more because we deprecated them already.
ACKs for top commit:
Kixunil:
ACK f811e0adb6
apoelstra:
ACK f811e0adb6 successfully ran local tests
Tree-SHA512: 64e75601ce8062da78bbd9aea97792e35c728aa31255348413fe9b57ee585974370d3f59b5467d7e4a65dc05d2718aea18bb381e128c8410fbfde4a851006416
The optional dependencies are ordered and separated by whitspace in a
manner that may not be obvious (or even have a reason).
Some of this is because since use of `?` deps changed name.
Put all the optional deps together in alphabetic order.
The `Builder` is staying in `bitcoin` while the `ScriptBuf` is moving to
`primitives`, so we cannot access the inner field of `ScriptBuf`.
Use the new `as_byte_vec` hack to mutate the inner `ScriptBuf` field.
In preparation for moving the `ScriptBuf` type to `primitives` stop
accessing the inner field when encoding/decoding, use `as_script`
and `from_bytes` instead.
In preparation for moving the `Script` type to `primitives` stop
accessing the inner field before doing slice operations, use `as_bytes`
to first get at the slice.
The `Script::fmt_asm` function is a legacy from days yore before
`Display` printed asm. We no longer need it.
Deprecate `Script::fmt_asm` and use the private `bytes_to_asm_fmt` or
`Display` impls.
Use the `define_extension_trait` macro to define two extension traits
for the two txid types. Each trait holds the deprecated `all_zeros`
function. There are no users of this trait in the code base.
Recently we deprecated the `all_zeros` functions on `Wtxid` and
`Txid` but for some reason our usage of them is not triggering a lint
warning.
Note please that this changes logic slightly, for example by using an
array of `0xFF` bytes instead of all zeros. Done in an effort to make it
even more obvious that the value is a dummy value and not mix it up with
the all zeros being used for coinbase thing.
In #3308 we added associated consts to the `Txid`, `Wtxid`, and
`OutPoint` types. During review and afterwards we realised the docs
could do with improving. Since we now want to move the types we should
do this first.
Close: #3331
Instead of accessing the inner type of a hash wrapper type when
consensus encoding we can call `as_byte_array()`.
Done in preparation for moving `Txid` and `Wtxid` to `primitives`.
Internal change only.
d65de7c7de Introduce and use new compact_size module (Tobin C. Harding)
Pull request description:
We would like to move the witness module to `primitives` but there is a bunch of usage of `VarInt`.
Introduce a module that does the encoding and decoding instead, note that while the functionality is internal decoding returns an error which may one day end up in the public API. So put the module in `primitives` and make it public.
Adds the module to `primitives`, adds a public `MAX_ENCODABLE_SIZE` variable that is commented with an issue link.
https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
ACKs for top commit:
apoelstra:
ACK d65de7c7de successfully ran local tests
Kixunil:
ACK d65de7c7de
Tree-SHA512: d9483c29b2b324e27460564a23f4639dde4037e6e773f4356216b02ebdea893a6361c342002b8e93a54de47b71ac69369431554f8cd0a2522fc451bf8493c81c
Either our CI is failing us or `deprecated` does not work as expected,
either way we should not be using the `OutPoint::null()` or `is_null`
functions any more because we deprecated them already.
733505148c Add tests for witness_program (Shing Him Ng)
Pull request description:
Add tests for witness_program
ACKs for top commit:
tcharding:
ACK 733505148c
Kixunil:
ACK 733505148c
apoelstra:
ACK 733505148c successfully ran local tests
Tree-SHA512: c1ee82edf22c7b39bc110d03836ba5ebfa785a63185a75c1a61781180ff907c1ea0c491c963629450f360152766845dacedccc6cf56bc3d8c607e66281427dd3
ae93e226e3 Remove hashes io feature (Tobin C. Harding)
Pull request description:
Currently we only get `std::io::Write` impls when the `bitcoin-io` dependency is used. This is overly restrictive, it would be nice to have `std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes` but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the `impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they get `std::io::Write` impls and if they enable `bitcoin-io` they get `bitcoin_io::Write` impls as well.
ACKs for top commit:
Kixunil:
ACK ae93e226e3
apoelstra:
ACK ae93e226e3 successfully ran local tests
Tree-SHA512: d47c9c060750e8a024c46cbf7afe8d0d1245fa1f5e575f36b3a11e2460d3620ad9def1a6331dafe77d46affc99b043ec9679e619ce8ddfa32436a5826ece09e4
fe46225ed0 Allow unused imports when running bench code (Tobin C. Harding)
eb67e873e0 Allow unused variables in release mode (Tobin C. Harding)
Pull request description:
Two patches to clear the million warnings when running the bench code.
ACKs for top commit:
apoelstra:
ACK fe46225ed0 successfully ran local tests; though in the first commit you could also use `cfg_attr` FWIW
Kixunil:
ACK fe46225ed0
Tree-SHA512: 3f705e0441d8c0e41e9ceb5473572810ff2513f7e5531c1b7889418a3a85ac8622e50e271c7a3b5c386fb3f5629b85d4bd79739c4a02b51d58da86890721d8d2
0f897f80a5 Re-write (and re-name) read_uint_iter (Tobin C. Harding)
Pull request description:
The `UintError` type (returned by `read_uint_iter`) is not that useful because one variant is unreachable. Re-write the function by doing:n
- Re-write the function to reduce the error cases returned.
- Re-name it to `read_push_data_len`
- Move it to `internals`
- Use `PushDataLenLen` enum instead of an int parameter
ACKs for top commit:
apoelstra:
ACK 0f897f80a5 successfully ran local tests; lol so much better than the old code
Kixunil:
ACK 0f897f80a5
Tree-SHA512: 095017a32c2d5bb2268cb1a059d0022e122faf8b41295f14970e7968374dd1c35c3b95357aba5aabaa17843439aebc237000009015ea9b8bc58ab1b337e8e1bc
ea2efc155e Add coinbase associated consts (Tobin C. Harding)
Pull request description:
Currently we have `all_zeros` functions and `null` functions but we can do better.
Add associated consts and improve the names to better describe what these dummy zero hashes are used for.
Deprecate related functions.
ACKs for top commit:
Kixunil:
ACK ea2efc155e
apoelstra:
ACK ea2efc155e successfully ran local tests
Tree-SHA512: bc7e840622a558bc46798e3606452ad24c16b7d23e7fe7a68fdf8a719326eb9d6d872ec1647620506f1de76b8086ae36cce0e1399e55e50bbd794efb8b4dda47
Currently we only get `std::io::Write` impls when the `bitcoin-io`
dependency is used. This is overly restrictive, it would be nice to have
`std::io::Write` imlps even without the `bitcoin-io` dependency.
Copy the logic out of the `bitcoin_io::impl_write` macro into `hashes`
but feature gate it differently.
Call the new macro inside `hash_type` (and in `hmac`), remove the
`impls` module, and move the tests to the integration test directory.
Remove the `io` feature from `hashes`, now if users enable `std` they
get `std::io::Write` impls and if they enable `bitcoin-io` they get
`bitcoin_io::Write` impls as well.
The `UintError` type (returned by `read_uint_iter`) is not that useful
because one variant is unreachable. Re-write the function by doing:n
- Re-write the function to reduce the error cases returned.
- Re-name it to `read_push_data_len`
- Move it to `internals`
- Use `PushDataLenLen` enum instead of an int parameter
c00afe8d52 Change MessageSignatureError to secp256k1::Error (Jamil Lambert, PhD)
a20d0bc4eb Deprecate `from_slice()` in sha256.rs (Jamil Lambert, PhD)
089043546f Deprecate `from_slice` methods in favor of arrays (Jamil Lambert, PhD)
Pull request description:
As brought up in issue #3102 support for Rust arrays is now much better so slice-accepting methods that require a fixed length can be replaced with a method that accepts an array.
`from_slice()` methods that require a fixed length have been deprecated and where needed a `from_byte_array()` method created that accepts an array.
There are still `from_slice` methods that rely on changes to external crates before they can be changed to arrays.
ACKs for top commit:
apoelstra:
ACK c00afe8d52 successfully ran local tests
tcharding:
ACK c00afe8d52
Kixunil:
ACK c00afe8d52
Tree-SHA512: c505b78d5ca57e7e1004df6761ac6760d5d9b63c93edc6ac1f9adf047bc67011883575f835b06f6d35d7f6c2b6a4c6c7f0a82a3f0e293bfb4ef58123b75d3809
333c8ab297 Add additional docs to Witness (Tobin C. Harding)
Pull request description:
The `Witness` struct is non-trivial, in particular it is not immediately obvious where and when the compact size encode value for each witness element is stored.
Make an effort to improve the docs on `Witness` in relation to the compact size encoded length of each witness element.
ACKs for top commit:
apoelstra:
ACK 333c8ab297 successfully ran local tests
Kixunil:
ACK 333c8ab297
Tree-SHA512: 1c61a9ad071c035d5ad2e54446120d29ebf8cc4a779c96f04eda825890687dcbd53accc17522f57ef4ffb226eb1d85c6a3a115f27bebcfc7ad3c677033a8a414
`from_byte_array` cannot error due to InvalidLength so the returned
MessageSignatureError has been changed to return a secp256k1::Error,
which is the only error type returned by the function.
Support for Rust arrays is now much better so slice-accepting methods
that require a fixed length can be replaced with a method that accepts
an array.
`from_slice()` has been deprecated and replaced with `from_byte_array()`
Currently we have `all_zeros` functions and `null` functions but we can
do better.
Add associated consts and improve the names to better describe what
these dummy zero hashes are used for.
Deprecate related functions.
In release mode we have a few unused variable warnings, lets just
allow them.
Found when running bench code, interestingly `cargo bench` must build
in release mode.
We would like to move the witness module to `primitives` but there is
a bunch of usage of `VarInt`.
Introduce a module that does the encoding and decoding instead, this
code is internal so put it in `internals`.
Note we add an unused public `MAX_ENCODABLE_SIZE` variable that is
commented with an issue link. Done like this because its quite
important that we see to it and it makes it clear that we are not and
we know about it.
https://github.com/rust-bitcoin/rust-bitcoin/issues/3264
The `Witness` struct is non-trivial, in particular it is not immediately
obvious where and when the compact size encode value for each witness
element is stored.
Make an effort to improve the docs on `Witness` in relation to the
compact size encoded length of each witness element.
c48d9d6523 Move transaction::Version to primitives (Tobin C. Harding)
f490222068 Introduce the VersionExt trait (Tobin C. Harding)
fb89974b82 Run the formatter (Tobin C. Harding)
bb3a3ecbaa Introduce temporary module for Version (Tobin C. Harding)
1fde868f51 Separate Version impl blocks (Tobin C. Harding)
Pull request description:
As per title, in tiny small chunks, move the `transaction::Version` over to `primitives`. Only the type, its associated consts, and its `Display` impl are moved. The two methods are left in an extension trait.
Was originally attempted in #3253
ACKs for top commit:
Kixunil:
ACK c48d9d6523
apoelstra:
ACK c48d9d6523 successfully ran local tests
Tree-SHA512: 83415cf0762dca5c263deb743734fc7abede804a6daac31df3d0101b51c6261e6d54452eb744727ae680cacce9e4ef726a6fa253d86c4e7a5d8ec789b137566c
We would like to move the `Transaction` type to `primitives`, as a step
towards this move the `transaction::Version` and its trait imps (just
`Display`) over there.
In preparation for adding an extension trait; separate the
`transaction::Version` impl blocks into stuff that will stay here and
stuff that will go to `primitives`.
Refactor only, no logic changes.
30bb93c676 Implement impl_to_hex_from_lower_hex macro for types that implement fmt::LowerHex (Shing Him Ng)
Pull request description:
Created a macro that implements `to_hex` for types that currently have `core::fmt::LowerHex` and called it on types that have `core::fmt::LowerHex` implemented. I put the macro in the `internals` crate since there are types across the whole project that can potentially use this.
Resolves#2869
ACKs for top commit:
Kixunil:
ACK 30bb93c676
apoelstra:
ACK 30bb93c676 successfully ran local tests
Tree-SHA512: d3ebc7b5c0c23f1a8f8eef4379c1b475e8c23845e18ce514cb1e98eb63fc4f215e6bc4425f97c7303053df13374ef931ae9d9373badd7ca1975a55b0d00d0e40
b6371b5801 Fix clippy rustdocs warnings (Tobin C. Harding)
Pull request description:
A new nightly version (`nightly-2024-08-28`) introduces a few warnings because of our rustdocs. These are valid warnings and should be fixed, thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
ACKs for top commit:
apoelstra:
ACK b6371b5801 successfully ran local tests
Kixunil:
ACK b6371b5801
Tree-SHA512: 503fb9d48772b74a5acdb26c0f77a85c52323c03360f983204fccee0f28bedeff142237b067caa1ce6ea04ea9842cc493e0d06dc141ca00a98151fa002b62392
345d3daa72 fix: deprecate wrong and unused max script num (ChrisCho-H)
Pull request description:
~~Script number can be up to 2^39 - 1 to encode locktime.~~
~~If it's only for the integer operation besides locktime, it must be 2^31 - 1, not 2^31.~~
I agree with apoelstra opinion to deprecate this value.
ACKs for top commit:
apoelstra:
ACK 345d3daa72 successfully ran local tests
Kixunil:
ACK 345d3daa72
Tree-SHA512: df4205415634d8db1412f16a75d1dbc110ed69930cefa085bdaa268a7fcaf47776dd1d8ed6965e6bbb476825b5f80e5c00bb375730ad102bae8868abe5894068
fa71b0e044 2024-09-01 automated rustfmt nightly (Fmt Bot)
Pull request description:
Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
apoelstra:
ACK fa71b0e044 successfully ran local tests; a bit big this week
Tree-SHA512: 875e8d54a06a22ccbef690a21f9d48a0acc1e6ea073a2d16632dd7ce63400cc7f2522339b7cb808653dade407517cd03d4fece4c8dec24abacf5303418aeaa75
9db6234ea9 Show compressed public key in Debug for CompressedPublicKey (Jiri Jakes)
Pull request description:
Currently `CompressedPublicKey` debug produces output of form:
```
CompressedPublicKey(PublicKey(2f8b18dc0adcb73d75f7934d9523ea7347083e41c48115398cb37e295a0a6ffe86e0bf8b1ef65888c880c8d8813a30e69e466380cbe2daec18f3ed1e7a553ff2))
```
Although it shows real internal structure together with inner uncompressed public key, it is not too helpful for the purpose of debugging _compressed_ public key.
After this patch, `Debug` output will be equal to `Display` (it, in fact, delegates rendering to `Display`), prepended by the name of the struct:
```
CompressedPublicKey(02fe6f0a5a297eb38c391581c4413e084773ea23954d93f7753db7dc0adc188b2f)
```
ACKs for top commit:
Kixunil:
ACK 9db6234ea9 unless other maintainers agree to drop the struct name.
apoelstra:
ACK 9db6234ea9 successfully ran local tests; we should keep the struct name since it is canonical for Debug output
Tree-SHA512: e2626678a4144357d3ff4ddd888459ae1283ea50bc7c8ef86deb9902d7d543de79109ac42883e5538f7e2f6a29426224bb43eb44a34f366821e47a9aca563753
3c7c8c44b6 Improve const_assert (Tobin C. Harding)
Pull request description:
Now that we can panic in const context we can improve the `const_assert` macro by adding a message string.
Original idea by Kix:
https://github.com/rust-bitcoin/rust-bitcoin/pull/2972#discussion_r1726328228
ACKs for top commit:
Kixunil:
ACK 3c7c8c44b6 in the sense that it does what it's supposed to with the only tiny issue being that the `bool` looks weird but not broken in any other way I can think of.
apoelstra:
ACK 3c7c8c44b6 successfully ran local tests
Tree-SHA512: 5ff721c0056f87d42c934818da6f780cd945f235291eb4b044752d67405a74f992d7f85853fec129e794ec3fcda1f319cc40daabc6a349d21bbdc977640d2572
a184066660 Move import inside feature gate (Tobin C. Harding)
Pull request description:
The `String` type is only used if the `serde` feature is enabled, move the import statement inside the already feature gated block.
ACKs for top commit:
apoelstra:
ACK a184066660 successfully ran local tests; sure
Kixunil:
ACK a184066660
Tree-SHA512: 688adb1e4489b0242856ac36ed9cf3503b147b84954cf1e6fd34b5e708ed6c1dd92da91e739243face1c1b4e4c8b19ce88e215117bef2a7af2e732859a6e108a
dae42bef9d do not enable bitcoin-io by default (Antoni Spaanderman)
a14cdaf859 don't enable std by default when testing (Antoni Spaanderman)
e83830dcfc use slice instead of array to not have to hardcode the length (Antoni Spaanderman)
55749d6f61 use `hash.to_byte_array` to check equality with `test.output` (Antoni Spaanderman)
969864e3b0 use fixed size array if possible, otherwise `&'static [u8]` (Antoni Spaanderman)
28ccf70fa6 remove unnecesarry borrow operator (`&`) (Antoni Spaanderman)
fa3a3afd02 remove unnecessary slicing (Antoni Spaanderman)
22e42ab86c fix test code being unnecessarily feature gated (Antoni Spaanderman)
Pull request description:
- remove 2 unnecessary cfg attributes from tests left over from #3167 (it made them not dependent on `alloc` anymore)
- simplify assertion logic by removing unnecessary conversions before comparing
- make tests `no_std` compatible by adding imports to alloc or std
- feature gate tests behind the `alloc` feature if they use anything from the alloc crate (like the `format!` macro)
- `schemars` feature enables `alloc` because (for example) its trait wants implementations to return `String`
- fix `bitcoin-io` always enabling when `std` is enabled (only useful if people depend on `hashes` only, `bitcoin` depends on `bitcoin-io` already)
ACKs for top commit:
tcharding:
ACK dae42bef9d
Kixunil:
ACK dae42bef9d
apoelstra:
ACK dae42bef9d successfully ran local tests
Tree-SHA512: 622fd4963ef21530a98af89bcfc71abe8723aac12d363ab88d9bd30dcf2f75392711bec10e2901fab5f1a30e11897d1aae36e22892738aa1e5670166f91fddd4
A new nightly version (`nightly-2024-08-28`) introduces a few warnings
because of our rustdocs. These are valid warnings and should be fixed,
thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
cf129ad314 fix: re-implement (de)serialization from/to readers/writers (elsirion)
Pull request description:
Fixes#3250.
The serialization is less than ideal and still allocates a lot. I can understand not wanting to (ab)use the consensus encoding traits, but they have a pretty good interface, copying it and creating some `EncodePsbt` and `DecodePsbt` traits with similar interfaces would have been nice imo.
ACKs for top commit:
Kixunil:
ACK cf129ad314
apoelstra:
ACK cf129ad314 successfully ran local tests; LGTM -- I believe this is non-breaking, as does cargo-semver-checks, so we can backport this to 0.32
Tree-SHA512: d7f218164d772db3a9fb4436953c3b5fd3677b92078d0843233197629df7d852d80615a3ff38c5b70771381ba1aeb30defdc98ee63653e570bb75dc553400cad
a76d76eca1 Change `T::from_str(s)` to `s.parse::<T>()` (Jamil Lambert, PhD)
Pull request description:
As mentioned in issue #3234 `s.parse::<T>()` is more idiomatic and produces more helpful error messages.
This has been changed in the main codebase, not including examples, rustdocs, and in the `test` modules.
`use std::str::FromStr;` has been removed where this change makes it unnecessary.
To close the issue it may also need to be changed in the examples and the `test` modules and `contributing.md` updated.
ACKs for top commit:
Kixunil:
ACK a76d76eca1
storopoli:
ACK a76d76eca1
tcharding:
ACK a76d76eca1
Tree-SHA512: 18be389579b5c2d0ba567dfb02b9c8d71c108a749ec5168c60a50e7af9d7f498e364c8f4c0ce1698d960e7d146486899cd10214ebc9c96708ed158a04dfff425
`s.parse` is more idiomatic and produces more helpful error messages.
This has been changed repo wide in the main codebase, not including
examples, rustdocs, and in the test module.
`use std::str::FromStr;` has been removed where this change makes
it unnecessary.
The two `TxOut` fields are public and there are no construtors or
getters to move, only the associated const `NULL`.
Add a tmp module around the big impl block so we can trick the formatter
into indenting before we add the extension trait.
3e034d5ede Add Arbitrary dependency (yancy)
Pull request description:
Adds an example draft showing what is needed to use Arbitrary for coin selection.
Shot out to how nice Arbitrary is for fuzzing a target by taking unstructured randomness and creating structured rust-bitcoin types for fuzzing. Is there a way we could add this to rust-bitcoin for structuring the fuzz data needed?
This is then the example to fuzz test a SRD algo (after applying this PR to rust-bitcoin) using rust-bitcoin types :)
```
#![no_main]
use arbitrary::Arbitrary;
use bitcoin::{Amount, FeeRate};
use bitcoin_coin_selection::{select_coins_srd, WeightedUtxo};
use libfuzzer_sys::fuzz_target;
use rand::thread_rng;
#[derive(Arbitrary, Debug)]
pub struct Params {
target: Amount,
fee_rate: FeeRate,
weighted_utxos: Vec<WeightedUtxo>,
}
fuzz_target!(|params: Params| {
let Params { target: t, fee_rate: f, weighted_utxos: wu } = params;
select_coins_srd(t, f, &wu, &mut thread_rng());
});
```
ACKs for top commit:
tcharding:
ACK 3e034d5ede
Kixunil:
ACK 3e034d5ede
apoelstra:
ACK 3e034d5ede successfully ran local tests
Tree-SHA512: accd565815de3b37730d2ff12a24fcfc84e52ad357e5c940b1500a1e0bb17f4ff5fd6e52d31e8e96bb5290ee4fa050cfd2a9bbd6bbae13fc378f43093b64177f
9fb5edb39e ecdsa: Improve error types (Tobin C. Harding)
Pull request description:
There are a couple of issues around the ECDSA signature decoding / parsing code. We have duplicate code in `from_str` and `from_slice` and both use the same error type even though it is impossible to get a hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `FromStrError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
ACKs for top commit:
Kixunil:
ACK 9fb5edb39e
apoelstra:
ACK 9fb5edb39e successfully ran local tests
Tree-SHA512: 3b3ae31887d603f1739d261b491b99f7847987f94dbbfefb9aa84d4250736eba2d007d28746bbb064946d3055e4cca01510677bf2cdbb11bbf83d7388dbd2620
c427d8b213 bitcoin: Compile time assert on index size (Tobin C. Harding)
49a6acc1a0 internals: Remove double parenthesis in const_assert (Tobin C. Harding)
2300b285ef units: Remove compile time pointer width check (Tobin C. Harding)
Pull request description:
3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately.
- Patch 1: Don't check pointer width in `units` because its not consensus code
- Patch 2: Modify internal macro `const_assert`
- Patch 3: Use index size to enforce not building on a 16 bit machine
ACKs for top commit:
Kixunil:
ACK c427d8b213 though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later.
apoelstra:
ACK c427d8b213 successfully ran local tests; unsure if we want to merg this or wait for #3215
Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8
a2be82c0c9 Use TBD in deprecated attribute (Tobin C. Harding)
Pull request description:
Our `release` job checks for 'TBD', I can't remember exactly why but I thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when we used TBD - clearly this is not the case now because we have a bunch of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
ACKs for top commit:
Kixunil:
ACK a2be82c0c9
apoelstra:
ACK a2be82c0c9 successfully ran local tests
Tree-SHA512: b383cc4095484291a7b4dca593ad5e017e3a9de9bfae9d6e9447ae36da32aa1c0d1fd593f49fd52c04db5ca5cdbaae8b30a772f792df13542f0a157a86295746
96e0e720fd feat(bip158): compute canonical filter hash (Rob N)
Pull request description:
From [BIP-157](https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#filter-headers)
> The canonical hash of a block filter is the double-SHA256 of the serialized filter.
If a user forgets the "double" in double-SHA256 they will be computing a nonsensical filter hash when this is easily handled by the API.
ACKs for top commit:
Kixunil:
ACK 96e0e720fd
tcharding:
ACK 96e0e720fd
Tree-SHA512: 5fc0b1632e2327adacbd0ab56b4cd7edce0774f8be2f819782519c51b9691a748f4b3c01887f3bf1146dee49a35f9dfac833f53ae69ee7a53858bd2cedcec01a
There are a couple of issues around the ECDSA signature decoding /
parsing code. We have duplicate code in `from_str` and `from_slice`
and both use the same error type even though it is impossible to get a
hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `ParseSignatureError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
Our `release` job checks for 'TBD', I can't remember exactly why but I
thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when
we used TBD - clearly this is not the case now because we have a bunch
of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
c97389596b Remove stale docs from sha256t_hash_newtype (Tobin C. Harding)
39f7dcb816 Reduce API surface of tagged wrapped hash types (Tobin C. Harding)
Pull request description:
Recently we made it so that wrapper types created with `hash_newtype` were not general purpose hash types i.e., one could not easily hash arbitrary data into them. We would like to do the same for tagged wrapped hash types.
In `hashes` do:
- Create a new macro `sha256_tag` that does just the tag/engine stuff out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256_tag` and `hash_newtype` to create tagged wrapped hash types.
Note that we do not add private helper functions `engine` and `from_engine` to the tagged wrapper types as we do for legacy/segwit in `sighash`. Can be done later if wanted/needed.
Fix: #3135
ACKs for top commit:
Kixunil:
ACK c97389596b
apoelstra:
ACK c97389596b successfully ran local tests
Tree-SHA512: d937a8eac1a77298231f946f9dfbc2f7739af8da00f2075b0b54803b4111c0cec810bc6564515153769193056cf102a9c954e216664f055b249d4a6153b14bca
Recently we made it so that wrapper types created with `hash_newtype`
were not general purpose hash types i.e., one could not easily hash
arbitrary data into them. We would like to do the same for tagged
wrapped hash types.
In `hashes` do:
- Create a new macro `sha256t_tag` that does just the tag/engine stuff
out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256t_tag` and `hash_newtype` to create tagged
wrapped hash types.
Note that we do not add private helper functions `engine` and
`from_engine` to the tagged wrapper types as we do for legacy/segwit in
`sighash`. Can be done later if wanted/needed.
c9053511b2 Remove misleading version metadata (Martin Habovstiak)
Pull request description:
The metadata in dependency specification was misleading because the version was not guaranteed to be the same anyway this was correctly linted but nobody so far cared to fix it. This change fixes it and adds a hint how to get the real version since some people seem still confused about how these things work.
ACKs for top commit:
apoelstra:
ACK c9053511b2 successfully ran local tests
tcharding:
ACK c9053511b2
Tree-SHA512: 99d849b1c1b202a429bc7eb25394ff84ba720b3ca6e73bce615f767e0924f5ef63a87553214f0f4fadd3b68ea84246ab8284d75df56cbbe794da7dea0d169dd9
2bb90b8203 Introduce two extensions traits for ScriptBuf (Tobin C. Harding)
ae0a5bd64a Run cargo fmt (Tobin C. Harding)
3fdc574851 Add temporary script buf modules (Tobin C. Harding)
4ff5d6886b Add private ScriptBufAsVec type (Tobin C. Harding)
c81fb93359 Make push_slice_no_opt pub(crate) (Tobin C. Harding)
1001a33f19 Add second ScriptBuf impl block (Tobin C. Harding)
3625d74e8b Make pub in crate functions pub crate (Tobin C. Harding)
b368384317 Separate ScriptBuf POD methods (Tobin C. Harding)
Pull request description:
Similar to #3155 but for `ScriptBuf`, however it is a little more involved.
Note:
- the change to use `impl` syntax (and addition of #3179)
- mad trickery of `ScriptBufAsVec` (props to Kix)
- widening of scope of private functions
Onward and upward!
ACKs for top commit:
Kixunil:
ACK 2bb90b8203
apoelstra:
ACK 2bb90b8203 successfully ran local tests
Tree-SHA512: 7209d8dc436e52b23e1dbfd9db8432df225ebdb701f465e4d1b55328e22988c98a0f28efdf2a8b3edbafc754354d718ab36bd2f5b1621d12e061b2dadaf49a05
The metadata in dependency specification was misleading because the
version was not guaranteed to be the same anyway this was correctly
linted but nobody so far cared to fix it. This change fixes it and adds
a hint how to get the real version for people who are mistakenly
investigating wrong file.
ad34a98c61 Refactor Rust version checking (Martin Habovstiak)
7d5ce89dad Fix type ambiguity in IO tests (Martin Habovstiak)
Pull request description:
Conditional compilation depending on Rust version using `cfg` had the disadvantage that we had to write the same code multiple times, compile it multiple times, execute it multiple times, update it multiple times... Apart from obvious maintenance issues the build script wasn't generating the list of allowed `cfg`s so those had to be maintained manually in `Cargo.toml`. This was fixable by printing an appropriate line but it's best to do it together with the other changes.
Because we cannot export `cfg` flags from a crate to different crates we take a completely different approach: we define a macro called `rust_version` that takes a very naturally looking condition such as `if >= 1.70 {}`. This macro is auto-generated so that it produces different results based on the compiler version - it either expands to first block or the second block (after `else`).
This way, the other crates can simply call the macro when needed.
Unfortunately some minimal maintenance is still needed: to update the max version number when a newer version is used. (Note that code will still work with higher versions, it only limits which conditions can be used in downstream code.) This can be automated with the pin update script or we could just put the pin file into the `internals` directory and read the value from there. Not automating isn't terrible either since anyone adding a cfg with higher version will see a nice error about unknown version of Rust and can update it manually.
Because this changes syntax to a more naturally looking version number, as a side effect the `cond_const` macro could be also updated to use the new macro under the hood, providing much nicer experience - it is no longer needed to provide human-readable version of the version string to put in the note about `const`ness requiring a newer version. As such the note is now always there using a single source of truth.
It's also a great moment to introduce this change right now since there's currently no conditional compilation used in `bitcoin` crate making the changes minimal. However it is not yet added to `bitcoin-io` since `bitcoin-io` is not depending on `internals`. It might be a reason to start depending on it but that's for later discussion.
ACKs for top commit:
apoelstra:
ACK ad34a98c61 successfully ran local tests
tcharding:
ACK ad34a98c61
Tree-SHA512: 7a82017fc51ba1b473ca638c7bdbf5c893da0a78c70ea8f1a0241049e7874592fad328abd60b3e09a00439b771e7cfc5ebad5e874314d15a48271ec6cb2d7bb7
In preparation for moving the `ScritpBuf` type to `primitives` add a
public and private extension trait for the functions we want to leave
here in `bitcoin`.
Note, includes a change to the `difine_extension_trait` metavariable
used on `$gent` from `ident` to `path` to support the generic
`AsRef<PushBytes>`.
Conditional compilation depending on Rust version using `cfg` had the
disadvantage that we had to write the same code multiple times, compile
it multiple times, execute it multiple times, update it multiple
times... Apart from obvious maintenance issues the build script wasn't
generating the list of allowed `cfg`s so those had to be maintained
manually in `Cargo.toml`. This was fixable by printing an appropriate
line but it's best to do it together with the other changes.
Because we cannot export `cfg` flags from a crate to different crates we
take a completely different approach: we define a macro called
`rust_version` that takes a very naturally looking condition such as
`if >= 1.70 {}`. This macro is auto-generated so that it produces
different results based on the compiler version - it either expands to
first block or the second block (after `else`).
This way, the other crates can simply call the macro when needed.
Unfortunately some minimal maintenance is still needed: to update the
max version number when a newer version is used. (Note that code will
still work with higher versions, it only limits which conditions can be
used in downstream code.) This can be automated with the pin update
script or we could just put the pin file into the `internals` directory
and read the value from there. Not automating isn't terrible either
since anyone adding a cfg with higher version will see a nice error
about unknown version of Rust and can update it manually.
Because this changes syntax to a more naturally looking version number,
as a side effect the `cond_const` macro could be also updated to use the
new macro under the hood, providing much nicer experience - it is no
longer needed to provide human-readable version of the version string to
put in the note about `const`ness requiring a newer version. As such the
note is now always there using a single source of truth.
It's also a great moment to introduce this change right now since
there's currently no conditional compilation used in `bitcoin` crate
making the changes minimal.
Add a private type that allows us to mutate the inner vector of a
`ScriptBuf` only using public functions and never touching the inner
field.
Done in preparation for moving the `ScriptBuf` to `primitives`.
Mad hackery by Kix!
In preparation for adding script buf extension make the
`push_slice_no_opt` have the same scope as the other private functions,
this will be the scope of the private extension trait.
In preparation for adding a private extension trait change the scope to
`pub(crate)` because the more specific `pub(in ...)` is not currently
supported by our `define_extension_trait` macro.
In preparation for moving the `ScriptBuf` as a plain old datatype to
`primitives`; separate the POD methods into their own impl block.
Refactor only, no logic changes.
b8067da934 Parse MSRV minor version number from env (yancy)
Pull request description:
Parse MSRV minor version number from env
replaces: https://github.com/rust-bitcoin/rust-bitcoin/pull/3145
ACKs for top commit:
Kixunil:
ACK b8067da934
tcharding:
ACK b8067da934
apoelstra:
ACK b8067da934 successfully ran local tests
Tree-SHA512: 60b3898c9b2739ca4858218b21d5c136b91b6ef45b1dc8f798d97aac861c42d046f114efcefdfa79f514d6da392f715a715ee6b3f230eb459f34e554b48ccf0e
0857697665 Replace impl blocks with extension traits (Martin Habovstiak)
b99bdcfdd6 Format `Script` blocks (Martin Habovstiak)
b027edffe7 Wrap `Script` impl blocks in temporary modules (Martin Habovstiak)
5a461545c7 Separate private `Script` methods (Martin Habovstiak)
27adc09e9f Generalize fn params in `define_extension_trait` (Martin Habovstiak)
fcc3cb03f0 Support non-doc attrs in extension trait macro (Martin Habovstiak)
ca1735f24c Separate POD methods (Tobin C. Harding)
Pull request description:
This moves methods from `Script` to extension traits in steps that should be easy to follow.
Moving to `primitives` requires doing the same with `ScriptBuf` so I'm holding off until this approach gets concept ACK (or alternatively someone else can do it :))
Closes#3161
ACKs for top commit:
tcharding:
ACK 0857697665
apoelstra:
ACK 0857697665 successfully ran local tests
Tree-SHA512: 3768d879e36139cf971c1921d3236141cbe87d707fd4bab7852f6ed8857b7867fa4146dfe720bd54e3d8cc50ecdc93886a10254cf9a82246358253f0312ffb47
2ec901fd63 Move the CompactTarget type to primitives (Tobin C. Harding)
a00bd7cc4d Introduce CompactTargetExt trait (Tobin C. Harding)
100ce03643 Run cargo +nightly fmt (Tobin C. Harding)
9c4a629659 Wrap CompactTarget impl block in temporary module (Tobin C. Harding)
578143c09e Separate CompactTarget impl blocks (Tobin C. Harding)
22d5646f7b Stop using CompactTarget inner field (Tobin C. Harding)
244d7dbe6c Remove generic test impl (Tobin C. Harding)
3d85ee3a02 primitives: Fix alloc feature (Tobin C. Harding)
Pull request description:
Done in preparation for moving `BlockHash` and `block::Header` to `primitives`.
- Patch 1 introduces an extension trait using `define_extension_trait!`
- Patch 2 is the trivial copy and past to move the type to `primitives`
This one shouldn't be to arduous to review, thanks.
ACKs for top commit:
Kixunil:
ACK 2ec901fd63
apoelstra:
ACK 2ec901fd63 successfully ran local tests
Tree-SHA512: b0e4f1af0b268e249a056cae71d7cafd1b025c4a079e5393ce80cd0b9c9bb6d2c6306531dc6786d986ff8a094b61866a86285b20d54037ef1395d127876bfd9c
In preparation to move script types to `primitives` we replace impl
block with extension traits by replacing the temporary modules with
`define_extension_trait`.
`rustfmt` is unable to format macro calls so instead we wrap the impl
blocks in modules to enable formatting in the next commit. We need to
change the visibility of the methods but that's OK since they're
internal.
The macro was trying to "parse" the parameters of functions defined in
extension trait. This was not needed and it was causing problems around
the `self` parameter. In this commit we change the macro to just pass
the parameters through.
Potentially the whole `pow` module will move to `primitives` but this
is not possible easily right now. However, we would like to be able to
move the `BlockHash` and `block::Header` types over to `primitives`
and doing so requires the `CompactTarget` to be there.
Move the `CompactTarget` type to `primitives` and re-export it from the
`primitives` crate root.
Note also, we re-export the type publicly from `bitcoin::pow`.
In preparation for moving the `CompactTarget` type to `primitives` stop
using the inner field in code that will stay behind in the
`bitcoin::pow` module.
In preparation for moving the `CompactTarget` to `primitives` remove the
generic `Into` impl and explicitly implement for just the `From` impls
that the `pow` unit tests use.
Test code only.
The `define_extension_trait` macro originally didn't support `#[inline]`
or other attributes for simplicity. We still want them so this commit
adds basic support for it. It adds the `doc` attributes to trait
*definition* only and adds all other attributes to the *implementation*
only. This should support `#[inline]` and other attributes. The downside
is it doesn't support adding non-doc attributes to trait *definition*
but I can't think of any relevant ones that we would want and we can
find a solution later if we do.
34e8212594 Replace &self with self: &Self (Tobin C. Harding)
Pull request description:
`foo(&self)` is syntax sugar for `foo(self: &Self)`.
The `define_extension_trait` is currently large, ugly, and not that expressive. If we use `self: &Self` then the macro is greatly simplified.
(Also consuming version `self: Self`)
De-sugar only, no logic changes.
ACKs for top commit:
apoelstra:
ACK 34e8212594 successfully ran local tests; lol this looks so much better
Kixunil:
ACK 34e8212594
Tree-SHA512: 7ec81bee99ede328d73a661c9e683a774ba14404ceb89ecb06765bedddf04dc9721672775b9ad3a9e3356d510c76681848f24ce4392a59d53216d23e6a27d9ad
579b76b7cb Introduce ToU64 conversion trait (Tobin C. Harding)
Pull request description:
The idea for this was pulled out of Steven's work in #2133
We already explicitly do not support 16 bit machines.
Also, because Rust supports `u182`s one cannot infallibly convert from a `usize` to a `u64`. This is unergonomic and results in a ton of casts.
We can instead limit our code to running only on machines where `usize` is less that or equal to 64 bits then the infallible conversion is possible.
Since 128 bit machines are not a thing yet this does not in reality introduce any limitations on the library.
Add a "private" trait to the `internals` crate to do infallible conversion to a `u64` from `usize`.
Implement it for all unsigned integers smaller than `u64` as well so we have the option to use the trait instead of `u32::from(foo)`.
ACKs for top commit:
Kixunil:
ACK 579b76b7cb
apoelstra:
ACK 579b76b7cb successfully ran local tests
Tree-SHA512: 2eaddfff995987a346e052386c6dfef3510e4732e674e3a2cfab60ee391b4cce1bf7ba4fb2dfd4926f8203d7251eea2198ccb61f0b40332e624c88fda4fa7f48
191897f9ea Manually format (Tobin C. Harding)
Pull request description:
Run `rustfmt` and manually fix the places where comments are moved to the wrong place.
ACKs for top commit:
Kixunil:
ACK 191897f9ea
apoelstra:
ACK 191897f9ea successfully ran local tests
Tree-SHA512: f977ff373d1d410012734208c090bfcd8f9dbda414d0b19400acf8f552df481b4a2bc20d77c61538895a6fb66197be13cbdadf74956d67fd4d055b99ba8ab356
We already explicitly do not support 16 bit machines.
Also, because Rust supports `u182`s one cannot infallibly convert from a
`usize` to a `u64`. This is unergonomic and results in a ton of casts.
We can instead limit our code to running only on machines where `usize`
is less that or equal to 64 bits then the infallible conversion is
possible.
Since 128 bit machines are not a thing yet this does not in reality
introduce any limitations on the library.
Add a "private" trait to the `internals` crate to do infallible
conversion to a `u64` from `usize`.
Implement it for all unsigned integers smaller than `u64` as well so
we have the option to use the trait instead of `u32::from(foo)`.
`foo(&self)` is syntax sugar for `foo(self: &Self)`.
The `define_extension_trait` is currently large, ugly, and not that
expressive. If we use `self: &Self` then the macro is greatly
simplified.
De-sugar only, no logic changes.
6836de9ee6 Remove catch all pattern (Tobin C. Harding)
Pull request description:
The `PushBytes` type enforces len is less than 0x100000000 so we do not need to panic in a catch all pattern after explicitly matching against less than 0x100000000.
Refactor only because of the invariant on `PushBytes` - no logic changes.
ACKs for top commit:
apoelstra:
ACK 6836de9ee6 successfully ran local tests
Kixunil:
ACK 6836de9ee6
Tree-SHA512: a7cdb31683a8c00eecbdd0879886bec48133f9029f899b5279f1f5294ef40320592db196bfcafdeef4507636fc785a7ab87879b25b6d1b4905ae573b545f1ff4
The `PushBytes` type enforces len is less than 0x100000000 so we do
not need to panic in a catch all pattern after explicitly matching
against less than 0x100000000.
Refactor only because of the invariant on `PushBytes` - no logic
changes.
84df3438ca Fix markdown list items (Tobin C. Harding)
0a45c68cf8 Introduce helper function name policy (Tobin C. Harding)
Pull request description:
As much as it hurts the C hacker inside me we have settled on using `_internal` to mark private function names that clash with a public function of the same name.
Introduce a policy section and rename one instance, I did not grep the codebase looking for other violations.
This came up because I had to look at what `_inner` implied when reading the function name somewhere else.
ACKs for top commit:
storopoli:
ACK 84df3438ca
apoelstra:
ACK 84df3438ca successfully ran local tests; sgtm. should probably update rust-miniscript which uses a `real_` prefix
Tree-SHA512: aece73fac54c4f34bdba72c08721586eb6d76dc9191442bbca43301f2af3d3b3e3217383f1ad20752fa7654e7f5d09927d1a4b72602aa064e76d550b5d6e81bd
As much as it hurts the C hacker inside me we have settled on using
`_internal` to mark private function names that clash with a public
function of the same name.
Introduce a policy section and rename one instance, I did not grep the
codebase looking for other violations.
This came up because I had to look at what `_inner` implied when reading
the function name somewhere else.
In an effort to reduce the cognitive load of reading code we are
removing casts unless they are useful or obvious.
Move the cast onto the call to `min` and comment it for good measure.
This allows us to call infallible `from` for conversion when needed.
Refactor only, no logic changes.
In an effort to remove unnecessary casts use `u128::from` to convert
from `u64`s. Leave the cast to `u64` in there because it is right after
a shift right and is brain-dead obvious.
Currently we enforce that our code only runs on machines with a
certain pointer width (32 or 64 by failing to compile if pointer size
width is 16). One of the underlying reasons is because of requirements
in consensus code in Bitcoin Core which requires containers with more
than 2^16 (65536) items [0].
We can better express our requirements by asserting on Rust's index
size (the `usize` type).
As a side benefit, there is active work [1] to make Rust support
architectures where pointer width != idex size. With this patch applied
`rust-bitcoin` will function correctly even if that work progresses.
- [0] https://github.com/rust-bitcoin/rust-bitcoin/pull/2929#discussion_r1659399813
- [1] https://github.com/rust-lang/rust/issues/65473
merge-script
Jamil Lambert, PhD
Chris Hyunhum Cho
Tobin C. Harding
Fmt Bot
yancy
Rob N
Shing Him Ng
BinChengZhao
spacebear
Jiri Jakes
Steven Roose
Peter Ryszkiewicz
Nadav Ivgi
Liam Aharon
elsirion
Antoni Spaanderman
Martin Habovstiak
leichak