865ba3fc39 Move serde string macros to internals (Tobin C. Harding)
4a2b13fcde internals: Feature gate whole serde module (Tobin C. Harding)
Pull request description:
The macros are internal things and can live in `internals`. This will help with future crate smashing.
ACKs for top commit:
apoelstra:
ACK 865ba3fc39
Kixunil:
ACK 865ba3fc39
Tree-SHA512: 7b3f029206c690ecf2894e0ad099d391312f7f8ec65ac9b5d4d9f25e6827f92075dcc851d0940a0faf1e27e7d0a305b575c8cc790939b3f222d7a2920d4d24fe
c717f7f424 Improve docs on private Witness fields (Tobin C. Harding)
Pull request description:
The `Witness` type is a reasonable complex data structure, make an effort to clarify its structure in the docs on the private fields.
Private docs only.
(Original idea pulled out of #2133.)
ACKs for top commit:
Kixunil:
ACK c717f7f424
apoelstra:
ACK c717f7f424 much clearer, thanks!
Tree-SHA512: 9d54b7eeefec97e584fb5f275049dbac0473c949fae8ab05c6961d6fc424c17a058af7037c2220ef1446af294d78c68bfee741cfeca1b18ecc402935d8069dab
d099b9c195 Remove wildcard from prelude import (Jamil Lambert, PhD)
Pull request description:
This patch replaces `prelude::*` wildcard imports with the types actually used. In a couple of cases `DisplayHex` was previously imported by the wildcard but was only used in the test module, an additional import was added to the test module instead of at the top where it causes an unused import warning.
Close: #2875
ACKs for top commit:
Kixunil:
ACK d099b9c195
tcharding:
ACK d099b9c195
Tree-SHA512: d59dfac0961d2649d509039a11c1b5574d81d05fef567a624cf15be2f587de796ea960ba5a08bef788199331c2f790fb06f7b393182538c7d8b1891ded119efc
The `Witness` type is a reasonable complex data structure, make an
effort to clarify its structure in the docs on the private fields.
Private docs only.
010141ecc9 api: Run just check-api (Tobin C. Harding)
cf800a1b07 Remove bech32 dependency from blockdata (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 010141ecc9
apoelstra:
ACK 010141ecc9
Tree-SHA512: 2d368ebf64ab7197b421e0dec48623f3fb03243081a988ff9ed2070135c8741efe24520c2aab496d54c52595808f10ee39816eaab8e3e4a7e64955cd25285670
a7731b2f33 api: Run just check-api (Marko Bencun)
47cba7a655 bip32: add from_32_slice method to DerivationPath (Marko Bencun)
Pull request description:
ChildNumber already has a `From<u32>` impl, but converting `&[u32]` to a `DerivationPath` was still difficult.
ACKs for top commit:
Kixunil:
ACK a7731b2f33
tcharding:
ACK a7731b2f33
Tree-SHA512: 2db94ee035e686102b8201f637422bb96bd79858aeffdb007594d722902d31a20f27e61b88fae8c854c80f785d9e7837b0158a046639ff8cc2d20d8883391842
dc10a49876 api: Run just check-api (Tobin C. Harding)
5e8f204581 Pass sigs and associated types by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference.
Currently this is not done in secp, but lets do it here in bitcoin.
Pass by value:
- `SerializedSignature`
- bitcoin sigs
- secp sigs
- secp `Message`
This is a continuation of the work to split up #2404 into manageable PRs.
ACKs for top commit:
apoelstra:
ACK dc10a49876
Kixunil:
ACK dc10a49876
Tree-SHA512: 8736eba067c74edb951c92357f5b3d0fc99c4fa6dc3beea579c10b3150873b74e8ec46c2c01f18818b37fca6e77c6b6edddeb6340edde6a9d8c28a4e69164c8c
ebb862cbaf Automated update to Github CI to rustc nightly-2024-06-26 (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 ebb862cbaf
Tree-SHA512: ec7ba43d784ff6245abda82ab7c27b247d1bfc654335fe384cb0990a4b36d752467282990452f485c911f996a95f8a7c163fe44a177ef1bd4cdcffc6f0e5ed06
6a7f780018 api: Run just check-api (Tobin C. Harding)
0c9223ac05 Manually format write_err statement (Tobin C. Harding)
43d7c750cc taproot: Add error types (Tobin C. Harding)
afe41c8a39 taproot: Split errors up (Tobin C. Harding)
Pull request description:
Currently there are a couple of errors in the `taproot` module that are too general, resulting in functions that return a general error type when a specific one would do.
Split two errors out and use them for for enum variants and function returns as possible.
Done as part of #2883
ACKs for top commit:
Kixunil:
ACK 6a7f780018
apoelstra:
ACK 6a7f780018
Tree-SHA512: bf5ed50dd8f913280d007f03124c7918c4b6cd642e67726bc3ffff23d9897f764a4391e167115668c3a1e951197485eba280fdbb8a0ce1bb7efb051388d13997
Wildcards have been replaced with what is actually used.
In a couple of cases an additional use statement was added to the test
module to import `DisplayHex` which is only used in test, but
previously imported with the wildcard at the top.
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.
10440b36a2 api: Run just check-api (Shing Him Ng)
452a7cc091 Re-export public functions, enums, and structs from units::parse in the bitcoin crate root (Shing Him Ng)
Pull request description:
Re-export UnprefixedHexError in the bitcoin crate root
Fixes#2874
ACKs for top commit:
tcharding:
ACK 10440b36a2
Tree-SHA512: 78a0676aa6d6fe63d810eeeebd3bf053d679d966d0aebf752760bfb440b2d4e831b0ff6b54b0306b390126b3f96a70c0846b4a1ec20aca96994d4a7b75e438e3
31a2324bfd Fix fuzz script (Tobin C. Harding)
Pull request description:
Currently the `fuzz.sh` script fails with "unbound variable" if called without any arguments, this has gone undetected since we added `set -euox pipefail` because in CI we always call it with an argument.
Use chatGPT to fix the bug.
Fix: #2924
ACKs for top commit:
apoelstra:
ACK 31a2324bfd
Tree-SHA512: 28e018bafd02c4034863c84e3b491c83962379d41bddeae184c4e35b889cb3e5aed9605b01749a97c60e3090525db52786574e487ff20cbf61fbdc8eaa8ab5a0
8ee1744b9b Make 'use core::fmt' calls consistent (Shing Him Ng)
Pull request description:
I started taking a look at #2869 and looked for everything that was implementing the `Display` trait:
```rust
impl fmt::Display for _
```
but found some places where the imports weren't consistent:
```rust
impl Display for _
```
There were only a few instances of the latter, so I went ahead and cleaned those up before starting #2869
I started pulling this thread when I saw the same thing was happening for `fmt::Debug` and `fmt::Formatter` so I updated the rest of the `use core::fmt::*` statements with a few exceptions:
- No updates to `use core::fmt::*` if it was being called from within a function since I felt like the function scope was small enough to not cause confusion
- No updates to `use core::fmt::{self, Write as _};`
ACKs for top commit:
Kixunil:
ACK 8ee1744b9b
tcharding:
ACK 8ee1744b9b
Tree-SHA512: 33eb6ea0c4e808ef78bc87de6547144b756bde206c50d80488f740e97cd8d11f1abcb8936c487d7bfd29be5e21c7f40ff88f82acdaaec9aacb4b6362ffc4c680
bc25ed35d5 Order serde feature list alphabetically (Tobin C. Harding)
5bd3387c15 Move package metadata to be underneath package section (Tobin C. Harding)
a2a9f193fe Put workspace crates in alphabetical order (Tobin C. Harding)
05931cc0fa Run the formatter (Tobin C. Harding)
Pull request description:
We are getting an increasing number of crates in the repo, clean up the manifests a bit in an endevour to help keep things manageable.
All patches are trivial and the PR makes no logic changes.
ACKs for top commit:
Kixunil:
ACK bc25ed35d5
apoelstra:
ACK bc25ed35d5
Tree-SHA512: a9850449a6f71ac5d53f501e36175e900bf4986f44c7636d3b1b55df80804b92bb10d8da7798f6bb866722aa2354ad2880ab5c0f5c4633f198c137d2ca42b7c9
Currently the `fuzz.sh` script fails with "unbound variable" if called
without any arguments, this has gone undetected since we added `set
-euox pipefail` because in CI we always call it with an argument.
Use chatGPT to fix the bug.
Fix: #2924
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
Currently there are a couple of errors in the `taproot` module that are
too general, resulting in functions that return a general error type
when a specific one would do.
Split two errors out and use them for for enum variants and function
returns as possible.
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
The package metatadata never changes and is not necessary to look at
basically ever, put it down the bottom of the manifest out of the way.
Helps to keep features and dependencies closer together.
Refactor only, no logic changes.
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.
Update Nightly Rustc Bot
merge-script
Tobin C. Harding
Jamil Lambert, PhD
Marko Bencun
Shing Him Ng