We have a single usage of the `bech32` crate inside the `blockdata`
module, to convert a `WitnessVersion` to a `Fe32`. We then have a single
call site where we use the conversion in the `address` module.
This code was written without thinking to hard about the introduced
dependency on `bech32`, in hindsite it shouldn't have been added.
In preparation for splitting a bunch of code in `blockdata` out into the
`primitives` crate remove the `bech32` stuff from the `witness_version`
module.
1147e66896 Remove unnecessary braces (Jamil Lambert)
Pull request description:
Remove the unnecessary braces to eliminate the rust-analyzer warning,.
This is trivial, but it causes rust-analyzer to constantly flag a warning, and a simple change removes it.
I also added a comma at the end, again trivial but as someone who speaks Rust as a second language I have read this is what is normally done and I only mention it to get feedback on if that is correct or not.
ACKs for top commit:
Kixunil:
ACK 1147e66896
apoelstra:
ACK 1147e66896
Tree-SHA512: 454996e1ac8ac2a66ad855fe94eed63c9d0582f688a88ebfcda57a333129e7e50004d20c44819b97d2d56dc4bffca6580407262b39c37c4e878cff90092c76dc
872ba938cc api: Run just check-api (Tobin C. Harding)
264d080054 hashes: Add hash type alias' (Tobin C. Harding)
d2632d3d8a hashes: Re-export Hkdf (Tobin C. Harding)
Pull request description:
In an effort to make the `hashes` crate more ergonomic to use add a bunch of type alias' to the crate root.
We intentionally do not rename the `foo::Hash` types so that uses have a choice of either using the module path to differentiate or to use the alias.
Update the crate level docs to use the alias' because they are more terse with no loss of clarity.
ACKs for top commit:
Kixunil:
ACK 872ba938cc
Tree-SHA512: 3635a72e0b9a1d05e49e83ba24bed80c87b8f5fa0b3260e8dd5a5fc130b6cc22e041e2b1798cd04d78c759f3c6e839c754c63afd9b7c3d5de5319a8abc8f5b12
362c3b8772 api: Run just check-api (Jamil Lambert, PhD)
9ea8c58ad6 Fix case sensitivity of denomination (Jamil Lambert, PhD)
67569ca632 Remove denominations < 1 satoshi (Jamil Lambert, PhD)
Pull request description:
The denomination `MilliSatoshi` has been removed from `Amount`.
`NanoBitcoin` and `PicoBitcoin` have also been removed since they are also less than 1 satoshi and the same reason for removing milliSatoshi in Issue #2820 should apply to them.
The second patch fixes the way the denominations with various combinations of upper and lower case are handled when converted from a string.
Close: #2820
ACKs for top commit:
Kixunil:
ACK 362c3b8772
tcharding:
ACK 362c3b8772
Tree-SHA512: d2dc23366023dae66705352822fbd25b90567971cbe0139ab6937b4dc419a1fd4681af1380232f39cd7844422e069d7861274eb2fd9bfe6730798271f1b50349
In an effort to make the `hashes` crate more ergonomic to use add a
bunch of alias' to the crate root - use re-exports where possible and
type alias' where required.
We intentionally do not rename the `foo::Hash` types so that uses have a
choice of either using the module path to differentiate or to use the
alias.
Update the crate level docs to use the alias' because they are more
terse with no loss of clarity.
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
8bd5c64433 api changes for "drop GeneralHash from wrapped hash types" (Andrew Poelstra)
91265977f8 hashes: stop exposing engine/from_engine and general hashing methods in hash_newtype (Andrew Poelstra)
8c4899f2cc bitcoin: remove all direct use of hashing/engines in unit tests (Andrew Poelstra)
b8d85a1df0 bitcoin: remove all use of engine/from_engine on opaque hash types (Andrew Poelstra)
0aa539f836 hashes: remove engine/from_engine from embedded test (Andrew Poelstra)
46dad847f2 api changes for split between Hash/GeneralHash (Andrew Poelstra)
73dcc79763 hashes: split Hash trait into two (Andrew Poelstra)
1fe4c63986 hashes: remove unused Hash import in embedded test (Andrew Poelstra)
Pull request description:
I'm not thrilled with these names. Personally I would prefer having `ByteArrayWrapper` (for non-general hashes) and `Hash` (for general hashes). But using `Hash` and `GeneralHash` greatly minimizes the diff because most of our use of the `Hash` trait was only for non-general stuff.
Maybe that tradeoff will change as we move stuff to inherents? I hope to do that in the next PR (or maybe the one after that, since I still have some "drop `GeneralHash` work to do for tagged hashes). And after that the hashing API should be "clean" enough that we can figure out HashEngine, possibly even folding GeneralHash into it. But that's the part of #2770 that we didn't finish nailing down so I'm not sure.
But other than naming, I like this PR. I think, if you approve of this PR except the naming, it would be best to ACK it and then we can do a followup rename-only PR, rather than dealing with the review pain of mass-renaming in rebases.
ACKs for top commit:
tcharding:
ACK 8bd5c64433
Kixunil:
ACK 8bd5c64433
Tree-SHA512: 3754f0f7ffae487e9f826fb6ecc48a6d9f606204a981bc56b98e118813a881b905e38a3cf5d6c3b3142aa0876ce2efc2ab75ec5a2f59fcc36fd86d148ab253bb
There are a few green lines for the Tap*Hash types, which are tagged
hashes, and whose engine/from_engine impls are replaced. The rest is
red, for the other hashtypes where these methods are just dropped.
A later PR will also drop the methods for the Tap*Hash types but that
seemed different enough from the rest to warrant its own PR.
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.
This commit illustrates the transformation I intend to make everywhere
we use newtyped hashes as "general hashes". *Within the module that the
newtype is defined* I encapsulate engine calls, which I do by calling
engine methods on the underlying general hash function. So within the
module there is a slight reduction in type safety, in the sense that I
need to make sure that I'm wrapping stuff properly.
But outside of the module, there will be no difference except that I
will no longer export engine/from_engine/hash/etc on newtyped hashes.
Instead callers will need to compute the newtyped hash only in ways
supported by the API.
In theory we could have a macro to produce engine/from_engine/etc for
newtypes that want to act as general hashes. But AFAICT there is no use
case for this.
Alternately, we could have a macro that produces *private* Engine types
and private engine/from_engine/etc methods for the hashes, which could
be used within the module and would provide stronger type safety within
the module. But in practice, raw hashing is usually only used within a
couple of methods, so all this infrastructure is way overkill and will
just make maintenance harder for everybody.
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
76f97677f7 Automated update to Github CI to rustc nightly-2024-06-23 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 76f97677f7
Tree-SHA512: 153743100b41c64542ac1d576d6ef0675e5bc4783b50c76c7231f3b7e00d38bb7b1e965f4cf53820dd1bca40ebbaf2e41caab7363b6cc150afa337fcd106fe21
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.
As you can see from the - lines in the API diff, there is no reduction
in API surface (we just remove the T:Tag bound from the sha256t::Tag
type, which is not strictly necessary but maybe we would prefer to keep).
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.
7e3b2ebb74 CI: Pin cargo-public-api version (Tobin C. Harding)
Pull request description:
So that updates don't break our CI setup used a specific version of `cargo-public-api`, this means we will have to update it manually periodically though.
For now, since the latest release just broke us, use the release from a month ago.
ACKs for top commit:
Kixunil:
ACK 7e3b2ebb74
apoelstra:
ACK 7e3b2ebb74
Tree-SHA512: 9266f1c193395e25b1fc7945b2ed10fa7418c73f483ee3c6ad6c713b41a9eb9a01e1e23a58df5720e53811d470ac7465cffc235471236447f9c985dea5af21bb
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
So that updates don't break our CI setup used a specific version of
`cargo-public-api`, this means we will have to update it manually
periodically though.
For now, since the latest release just broke us, use the release from a
month ago.
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.
a0c139d40c Automated update to Github CI to rustc nightly-2024-06-19 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK a0c139d40c
Tree-SHA512: c2cfc3c9235c50d15ab248c2eb56378300a1684983b6c832de62287c4d1dcb87be45de9dbceaeaa68f176fdbbb7aa1547d91b72636b0db6943ce64563dbae2c3
Tobin C. Harding
merge-script
Jamil Lambert
Jose Storopoli
Update Nightly Rustc Bot
Divyansh Gupta