The macro is non-trivial, so documenting it well is very useful. This
change improves both user-facing and developer-facing code with
appropriate warnings about the limitations of the code and Rust macro
system.
Currently we have an associated type on hash types `Inner` with
accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we
provide a way to create new wrapped hash types. The use of 'inner'
becomes ambiguous with the addition of wrapped types because the inner
could be the inner hash type or the `Inner` byte array of the inner
wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert
owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this
trait bound for the conversion methods to be correctly named according
to convention.
Because of the dependency hole created by `secp256k1` this patch changes
the secp dependency to a git tag dependency that includes changes to the
hashes calls required so that we can get green lights on CI in this
repo.
Hash types can be converted into a `Message` because `Message`
implements `From` for any type that implements `ThirtyTwoByteHash`,
which hash types do.
Use `into` to convert the hash argument to a message to sign.
090dad770f Improve string parsing (Tobin C. Harding)
Pull request description:
Currently we implement string parsing for height/time from the `absolute` module but not the `relative` module.
Improve the macros used to implement string parsing and use the new versions to implement string parsing for the height and time types in `relative`.
Done while reviewing data structures in relation to `serde`.
ACKs for top commit:
apoelstra:
ACK 090dad770f
Kixunil:
ACK 090dad770f
Tree-SHA512: bfa88efbaf5dc35755eb46df373a08e223f112860e8a65f58db9fdd77e2c01dc9377da735b33ef58940004fe5fe11690ac09be19591fded2c9fd04cd7d2bdf73
f62885890d Accept borrowed values in InputWeightPrediction::new() (Harshil Jani)
Pull request description:
The updated implementation accepts borrowed values and now it is convenient to use with slices.
closes#1669
ACKs for top commit:
Kixunil:
ACK f62885890d
apoelstra:
ACK f62885890d
Tree-SHA512: 59459e640a9676f3c293b10227026635b06fe55d31ffd2da606c0c70aac614694f48ba2f43bb3e8dce047e35e007fb2b09528d7dcfd0772e6b678680e2d781ea
d71c31c235 Create Address::matches_script_pubkey method (hashmap)
Pull request description:
to check if an address creates a particular script without allocating.
fixesrust-bitcoin/rust-bitcoin#1604
ACKs for top commit:
Kixunil:
ACK d71c31c235
apoelstra:
ACK d71c31c235
Tree-SHA512: cb60a53ae2be7c47dcd27415c883a73c81d57cbbf0bc92eaf76243d79d9c8e2c2efe91bef65a7e67ed26fec376f11325709ff27025d054813b1907ea2bf4961c
06f1f027ab Make `hash_newtype` evocative of the output (Martin Habovstiak)
b018f3e90b Remove the `$len` argument from `hash_newtype` (Martin Habovstiak)
752817e20d Stop using `$len` in `hash_newtype` (Martin Habovstiak)
Pull request description:
The API guidelines say macro input should be evocative of the output.
`hash_newtype` didn't have this property.
This change makes it look exactly like the resulting struct, `$len`
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.
The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.
Closes#1648
ACKs for top commit:
apoelstra:
ACK 06f1f027ab
Tree-SHA512: 9762db1eca9cd749980e5d68ca286f6c926620295a602f62365f199c7b333334b976db25ba25c64e56403cd1ba046b21b99e1c73cc528ad95612ef8901f216e5
596e756d01 Pin syn dependency for MSRV toolchain (Tobin C. Harding)
Pull request description:
CI change only, can this merge without 2 acks?
The recent 1.0.108 `syn` update violated our MSRV, pin `syn` in the CI script.
ACKs for top commit:
apoelstra:
ACK 596e756d01
Tree-SHA512: b27d8c18eae36025ac86bbad8f6a8bab3fccc8263e45b7421f216a0b1c6c318e02588c67094d8bd69a23efb32da9dfd10c2a6ce2bd5435133613a38906d83a38
438ee45691 Show cache construction in rustdoc (Tobin C. Harding)
Pull request description:
To make it more clear what the cache is show the cache construction line in rustdoc.
ACKs for top commit:
apoelstra:
ACK 438ee45691
Kixunil:
ACK 438ee45691
Tree-SHA512: d6da6bad57fddf9e2f4bcfb7c9b87df38bf4b2bb914e92e52d5ae8afa3405a9793536d7164223021ab6d183ddde732cf6889370834e36f37bae470127b0271fa
2620f3b69d Put optional = true at the end (Tobin C. Harding)
d86ef3b01b Put non-optional dependency with other non-optional (Tobin C. Harding)
Pull request description:
Late stage, super anal, manifest cleanup.
- Patch 1: put non-optional dependencies together
- Patch 2: put `optional = true` at the end of the line
ACKs for top commit:
apoelstra:
ACK 2620f3b69d
Kixunil:
ACK 2620f3b69d
Tree-SHA512: bca2bd2eb9a50aea04cab7c6eab7c264e91937ff20473ccf4496ba380897d1d50696719aa3379bd3f29b8a3cbe43d5485ac068f0e2c126462f361fcfe9941548
f69363d71a adding suggested documentation for path arg (Lorenzo Maturano)
673ca2d2fe changing docs and examples to use reference to slice in `derive_pub` (Lorenzo Maturano)
Pull request description:
`P: AsRef<[ChildNumber]>` means that path can be borrowed as a reference to a slice of `ChildNumber`.
While it does work with `Vec`, the documentation and examples mislead the user into thinking that he/she has to use a vec, which is not true. A simple reference to a slice of `ChildNumber` also works and does not consume heap
ACKs for top commit:
tcharding:
ACK f69363d71a
Kixunil:
ACK f69363d71a
Tree-SHA512: 9f0a2764c676ecaa45de69663c83c3008ee01f534c324ceca627c69d689d6ca6d0a3dda9361eaa402bacc68975e998840b78a3856e0572db3de10325b354ac6a
The API guidelines say macro input should be evocative of the output.
`hash_newtype` didn't have this property.
This change makes it look exactly like the resulting struct, `$len`
parameter was removed since it's not needed, reversing is controlled
using an attribute. The macro is also better documented and ready to be
extended in the future.
The tagged SHA256 newtype is not yet modified because it has a more
complicated input parameters.
Closes#1648
d83739a980 Clarify the intention of strange condition (Martin Habovstiak)
Pull request description:
It may not be obvious why the condition in `push_bytes` module checks for negation of 16 and 32 bit architectures rather than 64 bit. This adds a comment about it being conservative.
Addresses https://github.com/rust-bitcoin/rust-bitcoin/pull/1190#discussion_r1111342671
ACKs for top commit:
apoelstra:
ACK d83739a980
tcharding:
ACK d83739a980
Tree-SHA512: 8a674b9869c580a007c189b0d9b0a57023b26eff3afab25322d966e0ccaff767d19dd499243d438832972f351e1e7ea4b8e387a5e3c74569816814fea258b20b
It may not be obvious why the condition in `push_bytes` module checks
for negation of 16 and 32 bit architectures rather than 64 bit. This
adds a comment about it being conservative.
6fb2d12373 Get rid of BadFormat error (hashmap)
Pull request description:
add additional variants instead.
as discussed in https://github.com/rust-bitcoin/rust-bitcoin/pull/1365
ACKs for top commit:
Kixunil:
ACK 6fb2d12373
apoelstra:
ACK 6fb2d12373
Tree-SHA512: 2cf9146670c372a3a482448f84a30943cd2ff2fa4e724075d67a52dba5ac0ad38f99ca2af3dd3494e13584653f2b23e913e6421328d40be52e868a107fffe03b
be7b3754a9 Rename schnorr module to taproot (Tobin C. Harding)
9f39e872bc Rename SchnorrSighashType to TapSighashType (Tobin C. Harding)
f5c26693c5 Make match arms more terse (Tobin C. Harding)
40c246743b Split Sighash into LegacySighash and SegwitV0Sighash (Tobin C. Harding)
e38d843536 Do not use deprecated function in rustdoc example (Tobin C. Harding)
98130f49f1 Rename TapSighashHash to TapSighash (Tobin C. Harding)
7e4da3c0ab Move taproot keys to the keys module (Tobin C. Harding)
c5fe315a93 Move sighash to crypto module (Tobin C. Harding)
Pull request description:
This PR is now part 1 of great sighash clean up. It does not attempt to split ecdsa into two parts as discussed below, that is WIP and will be done in a separate PR after this upcoming release. Does however create `LegacySighash` and `SegwitV0Sighash` types.
This PR moves us along the road by doing:
- Move `sighash` to the `crypto` module, where I should have put it ages ago :)
- Move the taproot keys to the `keys` module
- Rename the `schnorr` module -> `taproot`
- Rename `TapSighashHash` -> `TapSighash`
- Rename `SchnorrSighashType` -> `TapSighashType`
- Fix a bunch of other schnorr usage (including pub/priv methods).
This leaves us in a situation where we have taproot sig and sighash type, and non-taproot ones prefixed with "ecdsa". Its not uniform but it kind of makes sense.
Fix: #1542Fix: #1550
ACKs for top commit:
Kixunil:
ACK be7b3754a9
apoelstra:
ACK be7b3754a9
Tree-SHA512: 4c52bea9ba71713afa181d625153d73d3c5b7c6f5786248960958982332a6a79501cd26e13a66bfb9700d9b8f59717446abb795ecbfbd8d60d9d59a38c1d718b
"schnorr" is a dirty word; the current `schnorr` module defines a
`Signature` that includes a sighash type, this sighash type is a bitcoin
specific construct related to taproot. Therefore the `Signature` is
better named `taproot::Signature`. Note also that the usage of `schnorr`
in `secp256k1` is probably justified because the
`secp256::schnorr::Signature` is just doing the crypto.
While we are at it, update docs and error messages to use "taproot"
instead of "schnorr". Also change function names and identifiers that
use "schnorr".
Currently we have `TapSighash` that is used for taproot sighashes but
for non-taproot sighashes we use `hash_types::Sighash`. We can improve
the API by creating a `LegacySighash`, and `SegwitV0Sighash`.
Copy the original `Sighash` macro calls to create the two new types in
the `sighash` module.
While we are at it, put the `TapSighash` and `TapSighashTag` into the
`sighash` module also.
There is never any use for the `sighash` module unless one is signing,
which requires the `crypto` module. The `sighash` module should
therefore live in the `crypto` module. This is not an API breaking
change because we reexport it at the crate root.
bcd95fa036 Add a newtype for script-pushable slice (Martin Habovstiak)
8fedbcbf13 Add `ecdsa::SerializedSignature` (Martin Habovstiak)
26fc4152ec Use `PushDataLenLen` to improve confidence (Martin Habovstiak)
bb2c7ec790 Introduce `hex_lit` crate (Martin Habovstiak)
Pull request description:
The code previously contained undocumented panic when trying to push
slice. This is now solved by making a newtype that guarantees limited
length.
Closes#1186Closes#1189
This is done on top of unsized script change. I only request light review of the design for now. I'd like to improve it to use a macro for all unsized types.
ACKs for top commit:
tcharding:
ACK bcd95fa036
apoelstra:
ACK bcd95fa036
Tree-SHA512: 193bee9b72e850a1a827124e6774111dc9bbc41a3999ae777acbf201a2ca062cad96debb27b1fd3374c89bacb95aab22638e967b7a11b3cbe955f2d5521f64d9
2eb68caf81 Remove the rustfmt required_version config option (Tobin C. Harding)
Pull request description:
The `rustfmt` config option `required_version` causes grief in CI because everytime `rustfmt` is update our `nightly` job fails. Since we require nightly anyways to run `rustfmt` the config option is basically redundant.
ACKs for top commit:
apoelstra:
ACK 2eb68caf81
Kixunil:
ACK 2eb68caf81
Tree-SHA512: fcd4f8b13e22f7094fdea0f3fa5cf7644d27c17da59a65ab7c43bdbdb2f4c672834d10cb848814aaa9cc9c626fa21ff2e759ae25d3249d52f02d7969352d1443
`Signature` only supported serialization into `Vec` which required a
heap allocation as well as prevented statically proving maximum length.
Adding a specialized type that holds a byte array and size solves this.
The solution is very similar to `secp256k1::ecdsa::SerializedSignature`.
The difference is that serialized signature in this crate contains
sighash bytes flag while in `secp256k1` it doesn't.
Script parsing is composed of several functions which implicitly rely on
various properties. Adding a type that restricts the valid values makes
local review easier.
So far we deserialized hex into `Vec<u8>` at run time. This was mainly
in tests where it had negligible performance cost. However moving the
computation to compile time has a few benefits: it allows proving the
length of the decoded bytes and identifies potential typos before the
code goes through LLVM and other compilation machinery which makes
feedback faster.
This change uses the `hex_lit` crate to move computation to compile
time. It is implemented as `const` declarative macro which doesn't blow
up compilation time.
70bff0de8d ci: Remove incorrect code comment and cargo clean (Tobin C. Harding)
Pull request description:
Recently while trying to fix CI I (Tobin) added a call to `cargo clean` and a code comment justifying it. This was merged because while not incorrect it is redundant since calling `cargo` with different `RUSTFLAGS` triggers a rebuild.
ACKs for top commit:
apoelstra:
ACK 70bff0de8d
Kixunil:
ACK 70bff0de8d
Tree-SHA512: 5bed107aedf9d6d240fa885335bfe04099436c0d30c045554644b43a06900c415708dfcedbd27037564d296c0ab12e4c4d4dc1e25ab64b4cc07ca7b46c46628a
a121e19e94 hashes: Implement AsRef for fixed size arrays (Tobin C. Harding)
Pull request description:
Implement `AsRef<[u8; X]>` for hash types including wrapped hash types. Doing so means at times the compiler can no longer infer the type because we have `AsRef<[u8]` implemented also but we can use `into_inner` and `as_inner` to get the inner array if needed.
Fix: #1462
## Note
This touches code that will likely be changed by #1577 and when we do #1491 but I believe its a step forward.
ACKs for top commit:
arturomf94:
ACK [`a121e19`](a121e19e94)
apoelstra:
ACK a121e19e94
Kixunil:
ACK a121e19e94
Tree-SHA512: 257c44826c7649db25bb3a6f023f68b2f17b70c546a056afad044bc8a16bf61f654c3846222505aaf5e6f9a0ad1d2113272d61317b407d0ac83702e41060a1ee
c3cc9e52ab Fix absolute lock time examples and tests (Tobin C. Harding)
Pull request description:
An absolute lock time of 100 is nonsensical because we are well past block 100. This value was used because it makes sense for _relative_ locktimes but for absolute lock times it makes the examples and tests slightly confusing.
ACKs for top commit:
apoelstra:
ACK c3cc9e52ab
Kixunil:
ACK c3cc9e52ab
Tree-SHA512: f490ef111bce0989c4ce8300c507c21b454448af4a91b9ef7a2fc05407411ca8721c9caa3dd1f0e8c0c133c4892c5c512f2d881af2cc67ae843d87eacae76ef1
4a03e2e721 psbt: Remove unused error variant (Tobin C. Harding)
Pull request description:
Remove an unused error variant for PSBT code (API breaking because the error type is public).
Woops, somehow I managed to get what was patch 1 of this series merged yesterday, I thought I left it out. Anyways, this is just the remove unused error variant now. No changes to that patch from previous versions of the PR.
ACKs for top commit:
apoelstra:
ACK 4a03e2e721
Kixunil:
ACK 4a03e2e721
Tree-SHA512: 228c661b97c6656db5a2bcc9ceb494ea485363b7f7262a97c677ee1639b5209c92ec3715ff48fdb108c95c828bfc83b6c475aa66f0ce8c5b0f286bfa7cc19554
An absolute lock time of 100 is nonsensical because we are well past
block 100. This value was used because it makes sense for _relative_
locktimes but for absolute lock times it makes the examples and tests
slightly confusing.
Martin Habovstiak
benthecarman
Tobin C. Harding
Andrew Poelstra
Harshil Jani
hashmap
Lorenzo Maturano