2bde5d002e api: Run just check-api (Jose Storopoli)
d1f84329e4 psbt: implement const for PsbtSighashType::ALL (Jose Storopoli)
Pull request description:
Closes#2751.
I only did the `ALL` which is by far the most common case.
ACKs for top commit:
tcharding:
ACK 2bde5d002e
apoelstra:
ACK 2bde5d002e
Tree-SHA512: 693575de758657a3e172d86ba5114ec0bf3b12b82df598e38c6a7916c99c20cfb5c4e74442108b51ae4e7bb1f1e940fd4a7269145e3f9838f727675c7711c890
We manually implement these methods (and the GeneralHash trait) on newtypes
around sha256t::Hash, because tagged hashes require a bit more work. In
the next commit (API diff) you will see that this affects two hashes,
which are the only things that appear green in the diff.
Users who want to implement their own engine/from_engine types now need
to do it on their own. We do this for the non-Taproot sighash types in
`bitcoin` (though only privately) to demonstrate that it's possible.
This is a continuation of the previous commit, but separated to make
review a little easier. This one replaces test vectors that were
previously computed by hashing garbage into Txids and various other hash
types with new test vectors which are directly-computed garbage
converted to hashes with from_byte_array.
In one case (src/hash_types.rs) this results in changing a bunch of
fixed test vectors. This is okay; this test is supposed to check the
direction of string serialization, which is unaffected by this commit
(or any commit in this PR). The existing test vectors, because they hash
the empty string, result in different bytes depending on the underlying
hash algo (sha256, sha256d, sha256t, etc). The new ones just use the
same fixed test vector for all of them.
This commit also updates a doctest in crypto/sighash.rs which
demonstrates how to manually feed sighash data into a hash engine and
correctly handle the sighash single bug. Because you can no longer
directly get a sighash object from an engine, this particular example
should maybe be rewritten to just encode to a Vec rather than a hash
engine, explaining that maybe you'd do this when implementing a HWW, to
verify the exact data being hashed. Or something.
Unrelatedly, you can check that there are no API changes in this commit
or the last several. The next commit will remove GeneralHash impls and
that's when you'll see changes.
In the next commits we are going to stop exposing the ability to hash
arbitrary data into wrapped hash types like Txid etc. In preparation for
this, stop using these methods internally.
This makes our internal code a little bit uglier and less DRY. An
alternative approach would be to implement the from_engine and engine
methods, but privately (and maybe having a macro to provide this). But I
think this approach is more straightforward.
The one exception is for the Taproot hashes, which are tagged hashes and
currently do not have their own engine type. I will address these in a
later PR because this one is already too big.
4652ce20ed API changes for "delete `all_zeros`" (Andrew Poelstra)
8869f35a69 hashes: drop the `all_zeros` method on arbitrary hashes (Andrew Poelstra)
9f8797f486 API changes for constification of hash constructors (Andrew Poelstra)
154e91af8c hashes: constify a bunch of constructors (Andrew Poelstra)
c155cbf8b2 hashes: use workaround to get constfns on tagged hashes with MSRV (Andrew Poelstra)
Pull request description:
I think these changes are both uncontroversial but they have fairly large diffs so I am PRing them together before making more invasive changes.
ACKs for top commit:
tcharding:
ACK 4652ce20ed
Tree-SHA512: 4560fa397deab50448598894b9364f9d8f8b48169901a84db6a44168cdba795ab69b48ad2cac61caebcee5e227a03271335b405cf5514265290a4d1f2fdf52a2
aebf216619 Use 100 column width for rustdoc (Tobin C. Harding)
c71ae9ac16 Move PushBytes::read_scriptint (Tobin C. Harding)
Pull request description:
The `push_bytes` module has a `private` module that exists solely to protect the invariant on the `PushBytes` inner byte slice. There is a `PushBytes` impl block outside the private module for functions that can not and do not violate the length invariant.
Recently we move the `read_scriptint` method to be on the `PushBytes` but we put it inside the `private` module, since the method only reads off of the slice it cannot invalidate the invariant and does not need to be inside the `private` module.
Move the `read_scriptint` method outside of the `private` module to keep that module as small as possible, helping with its stated aim of being the only place that requires auditing.
Patch 2 is whitespace only.
ACKs for top commit:
Kixunil:
ACK aebf216619
apoelstra:
ACK aebf216619
Tree-SHA512: f41eb691e7118a74767a2582a637a176a46e4a8d15259f1c1f3cef67bc57dd8dce4974407c5a5e31acd0b4665f722b20acad4e381747c9d496f18158a0ef9def
72655607b6 api: Run just check-api (Tobin C. Harding)
e8250cd96a Remove InvalidInternalKey variant from TaprootBuilderError (Tobin C. Harding)
Pull request description:
This variant is unused, remove it.
Done as part of #2883.
ACKs for top commit:
Kixunil:
ACK 72655607b6
apoelstra:
ACK 72655607b6
Tree-SHA512: 5cd27cacebcf078afdf1ceba4e3d51e78f20ee4883000b766efb7a246fd7a166240038a2e7d27249a22049c3258673a393ff2fd62cb4b27a2cade04b28ef2ac9
433fd6bf7e api: Run just check-api (Tobin C. Harding)
8fd583b069 Pass hash types by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the hash types by value.
Second step in the pass-copy-types-by-value work, pulled out of #2404.
ACKs for top commit:
apoelstra:
ACK 433fd6bf7e
Kixunil:
ACK 433fd6bf7e
Tree-SHA512: 999d12f60550cacc4ae19b4cbf505b25c1eed803820f22b1a706e9f95da1b7e7b422f393f4115d579927c0c476cd504036a39b3cdc06a1d6befbcff5513f7433
39df0a9fbe update api (Divyansh Gupta)
3a5f2932a4 create constants for ChildNumber enum (Divyansh Gupta)
Pull request description:
this aims to fix#2750
ACKs for top commit:
tcharding:
ACK 39df0a9fbe
Tree-SHA512: e1c38568facd2b9aa55b1b1ec0d5d5f68ff38ca3fe68962bc316c060a062299935aa51bcfc1c255a7f5c9ad97435cab22e2c160d3fd3f52a46f6b5cbb7d5743f
Manually implement it for Wtxid, Txid and BlockHash, where the all-zero
"hash" has a consensus meaning. But in general we should not be
implementing this method unless we have a good reason to do so. It can
be emulated or implemeted in terms of from_byte_array.
The use of Wtxid::all_zeros is obscure and specific enough that I am
tempted to drop it. But for txid and blockhash, the 0 hash appears in
actual blockdata and we should keep it.
All other uses of all_zeros were either in test code or in places where
the specific hash was not important and [u8; 32] was a more appropriate
type.
We use 100 column width for rustdoc in this project, while not a super
hard rule the docs on `read_scriptint` are long, using the 100 column
width reduces the line count a reasonable amount.
No text changes, only whitespace.
The `push_bytes` module has a `private` module that exists solely to
protect the invariant on the `PushBytes` inner byte slice. There is a
`PushBytes` impl block outside the private module for functions that can
not and do not violate the length invariant.
Recently we move the `read_scriptint` method to be on the `PushBytes`
but we put it inside the `private` module, since the method only reads
off of the slice it cannot invalidate the invariant and does not need
to be inside the `private` module.
Move the `read_scriptint` method outside of the `private` module to keep
that module as small as possible, helping with its stated aim of being
the only place that requires auditing.
a42bcdc22e Remove usage of blockdata from paths (Tobin C. Harding)
Pull request description:
the `blockdata` directory is code organisation thing, all the types/modules are re-exported from other places. In preparation for, and to make easier, the `primitives` crate smashing work - remove all explicit usage of `blockdata`.
Note that the few instances remain as they seem required e.g.,
`pub(in crate::blockdata::script)`
Refactor only, no logic changes.
Done as part of #2883
ACKs for top commit:
apoelstra:
ACK a42bcdc22e lgtm! I consider this "trivial" and will one-ACK merge it
Tree-SHA512: 310605e5203cf04aaeb91fe5512677b8f1438b183916686ba2cdc41ffdc18af7a0676206724e8a14c50ce6ed8faa9d48c69a2d5149eb1f56ae9c5f276fc5200f
16e4d22693 clarify the meaning of Height & Time based locktime. (Divyansh Gupta)
Pull request description:
this pr aims to fix : #2697
ACKs for top commit:
apoelstra:
ACK 16e4d22693
tcharding:
ACK 16e4d22693
Tree-SHA512: 55757d7e593cb284aff7040cf3298931c7f3d8e9e36d7328bd748a39be743e5c4202c55505add0219b2766d35d1660affc5ed4a7b9480b3a3bfb89982fe3970a
the `blockdata` directory is code organisation thing, all the
types/modules are re-exported from other places. In preparation for, and
to make easier, the `primitives` crate smashing work - remove all
explicit usage of `blockdata`.
Note that the few instances remain as they seem required e.g.,
`pub(in crate::blockdata::script)`
Refactor only, no logic changes.
Drop recursion, reduce memory usage to be logarithmic in size of tree
rather than linear, and put it all in one function rather than three.
Also make the method an trait method on MerkleNode which makes it a
easier on type inference, by writing e.g. TxMerkleNode::calculate_root.
Currently we are defining the two merkle tree hash types in the `block`
module, a better home for them is the `merkle_tree` module.
This is an API breaking change because the types were public in the
`block` module, however the change should/could be unnoticeable to users
if they use the crate level re-export - which is maintained.
e7f33a2a12 Update PushBytes::read_scriptint(x) to x.read_scriptint() (Shing Him Ng)
Pull request description:
Fixes#2849
There were a few other usages of PushBytes::read_scriptint(_) in the tests such as [this](406e3486ab/bitcoin/src/blockdata/script/tests.rs (L315)), but they couldn't be updated as cleanly as the changes in this PR. I wasn't sure if the intention of this issue was to fix those as well, but I can update this PR if needed
ACKs for top commit:
tcharding:
ACK e7f33a2a12
Kixunil:
ACK e7f33a2a12
Tree-SHA512: 4620f4972c40b0bf7333dbe302d1dabc5dbcb39749c734cc297a019d36983757f59659d76ae40b4a6121e51d0bde1e2b7883a0c77536be18927d1cfef53ba579
There are two limits that the Bitcoin network enforces in regard to
hashing scripts
- For P2SH the redeem script must be less than 520 bytes
- For P2WSH the witness script must be less than 10,000 bytes
Currently we are only enforcing the p2sh limit when creating an address
with `Address::p2sh`.
There are various ways to create addresses from script hashes and if
users manually hash a script then use the `ScriptHash` (or
`WScritpHash`) our APIs assume the script that was hashed is valid. This
means there is the potential for users to get burned by creating
addresses that cannot be spent, something we would like to avoid.
- Add fallible constructors to `ScriptHash` and `WScriptHash`
- Add `TryFrom` impls as well to both types
- Remove the `From` impls
The script in a p2wsh is typically referred to as the witness script not
the redeem script - rename the local test variable to follow suit.
Refactor only, no logic changes.
This test does an odd combination of function calls, its not obvious
what it is supposed to be testing. The `to_p2wsh.is_p2wsh` is already
tested above. The leading `to_p2sh` does not prove anything, one can put
currently pass any script to `to_p2wsh` so this tests nothing.
In preparation for patching the script hashing functionality first
remove this odd test.
3f4eb07769 Add a comment to regression test (Jamil Lambert, PhD)
fc2876ba10 Move use statements to top of module (Jamil Lambert, PhD)
778a44dd64 Refactor merkle block test (Jamil Lambert, PhD)
c4c1252a9e Change encode path (Jamil Lambert, PhD)
Pull request description:
Refactored the `extract_matches_from_merkleblock()` test function following https://github.com/rust-bitcoin/rust-bitcoin/pull/2859#issuecomment-2161710169.
Moved use statements to the top of the test module and changed it to use one level of path instead of importing the function names. e.g. `encode::serialize()` instead of `serialize()`.
Added the missing comment to the `regression_2606()` test. I was not sure where the hex value came from that was used to test that the deserialization fails. The comment was generated by copilot and may need to be edited, it does fit with the error given by deserialize: `OversizedVectorAllocation { requested: 12811880876963004416, max: 4000000 }`.
ACKs for top commit:
tcharding:
ACK 3f4eb07769
apoelstra:
ACK 3f4eb07769 the `prelude::*` is fine since it was already there since #298, but FYI I would not have accepted it today
Tree-SHA512: 203a30eee51ea91051cb10d5d7dd55b560d9d4d785120143c9fb29ea26ec77696124adc9c5bcb8cd736a7d293b897e665958bec5f66626a5c1c95c98b6029e0d
9f01871c11 api: Run just check-api (Tobin C. Harding)
7929b51640 Pass keys by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the key types by value.
This is patch 1 from #2404
ACKs for top commit:
apoelstra:
ACK 9f01871c11 this will annoy some people but I think we should do it
Tree-SHA512: 18afab537edf4ade4dc1c1e5992e50060b8935531f1e3cbe1d3b94b2fcb87aafa39947f342e0e762835bda3b4091dd35b3b74ea79f4dbb3b21660ffd21d1f82e
18b2788a5a api: Run just check-api (Tobin C. Harding)
6b7d02e5ae Add inherent functions to hashes (Tobin C. Harding)
Pull request description:
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`, and other use cases. However, it is unegonomic for users who just want to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make progress in the hashes API re-write. We can come back and de-dublicate later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and `all_zeros` const where easily possible.
ACKs for top commit:
apoelstra:
ACK 18b2788a5a
Tree-SHA512: 6b7a8d8a8501e981416d767040e5bd9fa8d1134be2ca133b5c53aa55f65c8456dccb63b642e30d0d571ca838c6f9eaeff6527d92a9b4212819a49ce619c4e093
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with
the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
8d256b4e79 Moved doctest to unit test (Jamil Lambert, PhD)
Pull request description:
Moved the doctest from the private module to a unit test to fix: #2840
ACKs for top commit:
apoelstra:
ACK 8d256b4e79
tcharding:
ACK 8d256b4e79
Tree-SHA512: 233961288579381cb5c51235331c78c9980046f0317a2062cb108a2e604adde322e2dd834e724d1fe05f54f92721fe6c9f5344dcf930f99aa3756a698ddf9732
fcf27a62a2 update api (Divyansh Gupta)
531aba0cf1 make `difficulty_float` general to all network (Divyansh Gupta)
Pull request description:
Fix#2783
ACKs for top commit:
tcharding:
ACK fcf27a62a2
apoelstra:
ACK fcf27a62a2
Tree-SHA512: aec9648b057677b89f4f397e1fa703ed6496436bd6e1e8052d5b7f52ef52c95b286babd425d97fe7d4f903cd0a3d68ae589d8fb255fc68f423714918cd485873
091d614aad bitcoin: Remove "std" feature from examples (Tobin C. Harding)
Pull request description:
The "rand-std" feature enables "std" but we use it in examples still. FTR I added this a while ago thinking the explicitness was clearer but in hindsight I think that was wrong and that it makes usage of our features _less_ clear.
No logic changes.
(Pulled out of #2756.)
ACKs for top commit:
storopoli:
ACK 091d614aad
apoelstra:
ACK 091d614aad
Tree-SHA512: d72eec37e3a434a1f850cb4257529fc18540cb5075bc7d3bd494cba59b08404c6c86223361a3c3a3de8d50a1168b656ff1123d28f7d2dcdf05c404caff716b1a
2db88a62fd Update bitcoinconsensus version (Jamil Lambert, PhD)
Pull request description:
Updated bitcoinconsensus version to 0.106.0+26.0 in bitcoin/Cargo.toml
The new version supports taproot and has a new parameter for `spent_outputs` in the `verify()` and `verify_with_flags()` functions.
The validation module was changed to keep the existing functionality by adding `None` as the `spent_outputs` and the flag `VERIFY_ALL_PRE_TAPROOT`.
This method does not add taproot features to the verify functions.
ACKs for top commit:
tcharding:
ACK 2db88a62fd
apoelstra:
ACK 2db88a62fd agreed, this is a good step forward
Tree-SHA512: b6cef395e065cfe859a7896ee2deb2f2d255051566751c10d06092064f6523338ca912aad81b789d6ca94e6f6164f2eef874ad6086c6ebe1384ce520d6eba366
8aa893ebd0 Remove repetition from sha256t_hash_newtype macro (Tobin C. Harding)
Pull request description:
The `sha256t_hash_newtype` macro is hard to reason about because we allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
Fix#2811
ACKs for top commit:
apoelstra:
ACK 8aa893ebd0 nice, small diff
Tree-SHA512: b38e7c307ac7288b4a5c1c3170ad6aa54c62bd3198922ec8bb091867b230bb9149f7dc996766fc8fa20a1af18b318c475b3e83e2689d322b7f4af0d5cb588e50
Updated bitcoinconsensus version to 0.106.0+26.0.
The new version supports taproot and has a new parameter for spent outputs in the `verify()` and `verify_with_flags()` functions.
The validation module was changed to keep the existing functionality by adding `None` as the `spent_outputs` and the flag `VERIFY_ALL_PRE_TAPROOT`.
ce585dc529 api: Run just check-api (Liu-Cheng Xu)
61565957ad Add API for extracting the inner payload of RawNetworkMessage (Liu-Cheng Xu)
Pull request description:
I'd like to take out the `payload` of `RawNetworkMessage` and then send it to the actual network message processor, but find there is no way to do it. This commit adds such an API to expose all the inner parts (UPD: so that I don't have to do an unnecessary clone to obtain the owned value of `payload`).
ACKs for top commit:
apoelstra:
ACK ce585dc529
tcharding:
ACK ce585dc529
Tree-SHA512: 89c5f1361a8c2d0ecf928325e9a37b26d39cf32c3a023b78742555a5a1e59e1579522e942ff6f48b07b2d3b81b48c5e8182fa6e88b57fc9c33ce13e313ba2982
The `sha256t_hash_newtype` macro is hard to reason about because we
allow repetition so which tag goes with which type is slightly obscure.
Remove repetition and call the macro three times.
Internal change in `bitcoin`, API change in `hashes`.
Andrew Poelstra
Jose Storopoli
Tobin C. Harding
Divyansh Gupta
Shing Him Ng
Fmt Bot
Jamil Lambert, PhD