06afd52a12 Improve hashes::Error (Tobin C. Harding)
Pull request description:
We are trying to make error types stable on the way to v1.0
The current `hashes::Error` is a "general" enum error type with a single variant, better to use a struct and make the error usecase specific.
Improve the `hashes::Error` by doing:
- Make it a struct
- Rename to `FromSliceError`
- Move it to the crate root (remove `error` module)
Includes usage in `bitcoin`.
ACKs for top commit:
apoelstra:
ACK 06afd52a12
Kixunil:
ACK 06afd52a12
Tree-SHA512: 20a517daaf3e9e09744e2a65cde6e238c8f2d1224899a6c04142a3a4e635d54112b0a2e846d25256071bb27cb70f7482380f98e9a535a5498aa4c7dc0d52cc54
0f74eb6876 Remove the unused crate::Error (Tobin C. Harding)
74154c2294 Add block::ValidationError (Tobin C. Harding)
3a9b5526b3 Move BlockHash From impls (Tobin C. Harding)
Pull request description:
Remove the `crate::Error` and replace its usage with `block::ValidationError`.
ACKs for top commit:
apoelstra:
ACK 0f74eb6876
Kixunil:
ACK 0f74eb6876
Tree-SHA512: 80b2c98d3d8f7c3f060c8ea2d94e1ebe118c07d0dcf91f6d13aed00df2cb0b15bf5e295ec0976d88d81e029cf7d3e8e4a1fe70120db57e49bbd8dd229291836b
0046bb8ad8 Fix usage of cfg(rust_1_53) (Tobin C. Harding)
c3450f3913 Remove stale usage of doc(cfg) (Tobin C. Harding)
Pull request description:
These build cfg options are not features, fix broken usage. And remove stale docsrs attribute while we are at it. Bad rust-bitcoin devs.
Found while reviewing #1870
ACKs for top commit:
apoelstra:
ACK 0046bb8ad8
Kixunil:
ACK 0046bb8ad8
Tree-SHA512: 7053affef6654ff203c93590bf081e165f019feb040aa8c55259ffe4e15eaf0e7522f6e5a4f6f62e8f578420b0313f4b7b17c46b741b7fcfd05750e5c5976589
3af9258025 embedded: Document how to clean up linker flags (Tobin C. Harding)
Pull request description:
Our embedded crate includes instructions to source a shell script that sets the `RUSTFLAGS` env var. Having the env var set like this in ones environment breaks linkage when trying to do "regular" builds.
Document how to clean up.
ACKs for top commit:
apoelstra:
ACK 3af9258025
Kixunil:
ACK 3af9258025
Tree-SHA512: 72758fba4dede873da299f01d75fd64b549fe21f954c2720ba3b7fc3c29fd4ed28fd0a749bbe987a7de1551aa32192253dd0033a18c877e877c9960343a5b07c
Our embedded crate includes instructions to source a shell script that
sets the `RUSTFLAGS` env var. Having the env var set like this in ones
environment breaks linkage when trying to do "regular" builds.
Document how to clean up.
Add a `ValidationError` to the `block` module and remove the two
variants out of `crate::Error`.
This error is only used by the `validate_pow` function, a specific error
better serves our purposes.
We are trying to make error types stable on the way to v1.0
The current `hashes::Error` is a "general" enum error type with a single
variant, better to use a struct and make the error usecase specific.
Improve the `hashes::Error` by doing:
- Make it a struct
- Rename to `FromSliceError`
- Move it to the crate root (remove `error` module)
Includes usage in `bitcoin`.
No idea why this re-export is here, the `Prevouts` type is not even used
in the `psbt` module.
Remove the re-export of `crate::sighash::Prevouts` from `pstb`.
013dffa65d tests: Use script hash methods (Tobin C. Harding)
Pull request description:
The `ScriptBuf` type can be serialized using it's `to_bytes` function. Do not use the `psbt::Serialize` trait to do so in test code.
No logic changes, since the impl of `psbt::Serialize` for `ScriptBuf` just calls `to_bytes`.
ACKs for top commit:
Kixunil:
ACK 013dffa65d
apoelstra:
ACK 013dffa65d
Tree-SHA512: 08959e065f1528f2ca69c12d5e34bceea3ddd17eefcee45094f071b3fa7357dbf289f6fe54d18fea02eecd1d0a7cd04598bbf014a5f10676387dbe27bb490395
b03c24db8c Add a checked version of weight mul fee_rate (yancy)
Pull request description:
Add a checked version of fee_rate * weight. While I like the trait version of just being able to multiply `feerate * weight`, it's not really very useful imo since a large input feerate could cause an overflow. Instead of changing the trait in https://github.com/rust-bitcoin/rust-bitcoin/pull/1849 (not idiomatic enough I guess) I added a `checked_weight_mul` method to `FeeRate`.
ACKs for top commit:
apoelstra:
ACK b03c24db8c
Kixunil:
ACK b03c24db8c
Tree-SHA512: 231ade94291becadcea9ea2a40a5daf96b77f01a29cca2494d7bbe4f7de5b412fa8fc816ea249268569f5378410185d9349fd687533bf3a422a752997e107a2b
We have methods to convert a script to a `WScritpHash` and `ScriptHash`,
no need to do this manually, let alone use the `psbt::Serialize` trait
to do so.
During the last round of releases (bitcoin 0.30, hashes 0.12) we removed
the `FromHex` implementation from all types except vecs and arrays. We
added `FromStr` impls for types that roundtrip with `Display`.
We never added a changelog mention to either `bitcoin` or `hashes`, lets
retroactively add an entry.
Fix: #1747
These constants had an error that they had `script_size` set to 0 which
was incorrect because it's not length of the script but serialized size.
Rather than just bumping the value this uses the `from_slice` method
which is less error-prone.
This also deletes a useless test of the constants.
Closes#1834
8835d5d2f1 make bip21 schema lowercase (Riccardo Casatta)
Pull request description:
The spec RFC3986 specifies the scheme is case insensitive and we were uppercasing to optimize QR code representation.
Unfortunately, common platform such as Android seems to fail to recognize uppercase schema, so for compatibility reason we use lowercase.
close#1843
ACKs for top commit:
Kixunil:
ACK 8835d5d2f1
apoelstra:
ACK 8835d5d2f1
Tree-SHA512: 02d228b52fe4df20edb71ba8e2ab8a2bae4b912252e30a3150ee3af74e65a6e91b165c9579273b57e894366c9792a8312ea973723cd8c5d98037aaba80d7cf07
6c6a89b1d1 Add sub-sat fractions parsing regression test (Martin Habovstiak)
f1a3dc6719 Allow parsing sub-sat denoms with decimal points (Martin Habovstiak)
b3d9a267ea Add a few more amount parsing tests (Martin Habovstiak)
Pull request description:
Numbers with only zeros after decimal points are valid if they are also
multiples of `10^precision` (e.g. 1000 for msats). These were
artificially disallowed as "too precise" which was at least misleading.
This change allows parsing such numbers.
And yes, I know this is not perfectly efficient (unless the compiler figures out some magic opts) but so isn't the rest of the code. TBH this parsing code drives me crazy and I'd love to rewrite it to be more efficient and readable.
ACKs for top commit:
apoelstra:
ACK 6c6a89b1d1
Tree-SHA512: 03cf4b416f2eac25e0aac57ef964ed06fa36c7fe8244bdcf97852cc58e1613b1ec6132379b834da58ad3240fdd61508a384202f63aa9ffa335c18cd7b2b724d3
The spec RFC3986 specifies the scheme is case insensitive and we were uppercasing
to optimize QR code representation.
Unfortunately, common platform such as Android seems to fail to recognize
uppercase schema, so for compatibility reason we use lowercase.
75b3f19b96 Move and rename TxOut default trait to a const called NULL (yancy)
Pull request description:
Create an associated constant `const TxOut::NULL` for consensus signing code and remove the default trait. Note I tried to deprecate the `default()` fn instead of just removing it but it doesn't seem to be possible. Also because `TxOut::NULL` is `const`, `ScriptBuf::new()` needed to be changed to `const fn`.
ACKs for top commit:
apoelstra:
ACK 75b3f19b96
Kixunil:
ACK 75b3f19b96
Tree-SHA512: ff61a2b1641a1ba32f183c27205af2d868dbc2eb47cf758c3d8315329d2c23e0b8a82ea0ab59d1de9add0d238f927165e2e4df014aab1ef066d74d4feda0700b
995c797e0d feat: generate PrivateKey (kshitjj)
Pull request description:
added a function to generate a private key
Resolves: #1823
ACKs for top commit:
apoelstra:
ACK 995c797e0d
tcharding:
ACK 995c797e0d
Tree-SHA512: 29ba54be8cb777e71a4683835686cbf2978b23736f629d7bbff468074235fece261ca170c23f358d1bd878987566d09e4488c3f1a106c59a5c8bdf52b98abffe
Numbers with only zeros after decimal points are valid if they are also
multiples of `10^precision` (e.g. 1000 for msats). These were
artificially disallowed as "too precise" which was at least misleading.
This change allows parsing such numbers.
8e6f953aa7 Expose valid (min, max) difficulty transition thresholds (Wilmer Paulino)
Pull request description:
Once `U256` was made private, we lost the ability to check whether a valid difficulty transition was made in the chain, since `Target` doesn't expose any operations. We only choose to expose `Shl<u32>` and `Shr<u32>` such that we can compute the min and max target thresholds allowed for a difficulty transition.
This is something we realized was missing after bumping to `rust-bitcoin v0.30.0` in `rust-lightning`, specifically for our `lightning-block-sync` crate. It may also be worth having a helper in `rust-bitcoin` that checks a header properly builds upon the previous, but that can be left for future work.
ACKs for top commit:
Kixunil:
ACK 8e6f953aa7
sanket1729:
ACK 8e6f953aa7 . Sorry, was confused by some details.
apoelstra:
ACK 8e6f953aa7
Tree-SHA512: 740dd64089426463dc6a19726d5a562276bd0966e0e31af8e1e67b28d18945644ac0e50be3cf0cc7fa604acc3d2c5b912a77a7caa69d8cff85f70fd57e5328c5
dff757d7db Comment predict_weight (yancy)
Pull request description:
I've been reading over the `predict_weight` function since it is one of the biggest challenges for coin-selection. IE choosing inputs and constructing an optimal selection strategy requires predicting the weight to get the best selection. It's great this work has been done but there are some things I don't understand well enough to comment.
1) why are we looking at the size of VarInt struct here
> let script_size = script_len + VarInt(script_len as u64).len()
2) [predict_weight_internal](36500b4451/bitcoin/src/blockdata/transaction.rs (L1245)) has a bunch of magic numbers. I'd like to be able to comment this as well but I don't fully understand that function.
Also, `Transaction.rs` is a big file and it seems like all of the prediction stuff could be moved to a separate module or maybe a separate crate?
ACKs for top commit:
tcharding:
ACK dff757d7db
Kixunil:
ACK dff757d7db
Tree-SHA512: 8ffa16d500075d691528ce1819b9352a148af431889bebbd7cddcf470bd4e3048ec53a5e778bc3659e33d8c25b68422a93dac1d46b9489ff56f41d88d7f05433
d57ec019d5 Use Amount type for TxOut value field (yancy)
Pull request description:
Propose using `Amount` type for the `TxOut` `value` field. I only implemented `Decodable ` and `Encodable` enough to compile but this needs to completed obviously if using `Amount` seems like a good idea.
ACKs for top commit:
tcharding:
ACK d57ec019d5
apoelstra:
ACK d57ec019d5
Tree-SHA512: df3fd55294d5f9392ca90bb920be8fbb9d7d285d97669412e07d5a099f70f81fd73e7e259679de9c8ce5c6c855e64f62213700f0fb7db415e0c706c509485377
ed6421c939 address: Add generic serde::Serialize for Address (Steven Roose)
814b9917da address: Add Sync, Send, Sized and UnPin marker traits on NetworkValidation (Steven Roose)
Pull request description:
With the new rewrite of Address, `serde::Serialize` is only implemented on `Address<bitcoin::address::NetworkChecked>` and `Address<bitcoin::address::NetworkUnchecked>`. But the compiler has no way of knowing that that are all the possible versions of `Address`, so the generic `Address<impl bitcoin::address::NetworkValidation>` doesn't implement `serde::Serialize`.
ACKs for top commit:
Kixunil:
ACK ed6421c939
tcharding:
ACK ed6421c939
Tree-SHA512: 65e43dff244c94fe08ccb2d985781a2687a1e2db186960a35d4ae89f3b31c5af66892630a3ebaac9cecdc83638487425afa17374869d278648b348869e0ba091
Once `U256` was made private, we lost the ability to check whether a
valid difficulty transition was made in the chain, since `Target`
no longer exposes any arithmetic operations.
6cab7beba3 Deprecate min/max_value methods (Tobin C. Harding)
5fbbd483ea Use MIN/MAX consts instead of min/max_value (Tobin C. Harding)
3885f4d430 Add MIN/MAX consts to amounts (Tobin C. Harding)
Pull request description:
The new MSRV (1.48.0) uses associated consts MAX/MIN instead of functions, we had functions to be compliant with the old MSRV.
~Remove all methods `min_value` and `max_value` including calls to these methods on stdlib types.~
PR is now split into three patches:
- patch 1: Add missing associated consts MIN/MAX as needed
- patch 2: Use consts instead of method calls
- patch 3: Deprecate methods `min_value` and `max_value`
ACKs for top commit:
sanket1729:
ACK 6cab7beba3
apoelstra:
ACK 6cab7beba3
Kixunil:
ACK 6cab7beba3
Tree-SHA512: 60949d1bb971e0dfbab7f573b4447f889b5fa1a5f1c9ac9325a2970fe17a19ccc93418dba57f07bed7e13864b130de48b6b3741d1d80266c6144237dd4565ff7
c4c64c0dc5 Test with minimal dependency versions (Martin Habovstiak)
d5655d503a Bump core2 dependency from 0.3.0 -> 0.3.2 (Tobin C. Harding)
Pull request description:
This is work originally done by Kixunil in #1272, I picked it up to help out. The only changes I made were rebasingg, updating the recent lock file, adding `--locked` to hashes contrib file, and adding a co-developed-by tag for accountability.
It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could cause resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate.
This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal dependency versions, while `recent` contains dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.)
The documentation is also updated accordingly.
Closes#1230
ACKs for top commit:
apoelstra:
ACK c4c64c0dc5
Kixunil:
ACK c4c64c0dc5
Tree-SHA512: 7d386e96ab747f6a6bafeea828ac65bd8bb11975eaa3408acecac369cd2f235f6e9d4c57202be18a3dc2eeb2a2df532d73e4d35cd1f3fbf092eb6414c55b1524
Our previous MSRV did not support MIN/MAX associated consts so we had
methods min/max_value. Now that our MSRV is Rust 1.48.0 we can use the
consts.
Deprecate min/max_value methods in favor of MIN/MAX associated conts.
We currently use the functions `min_value` and `max_value` because the
consts were not available in Rust 1.41.1, however we recently bumped the
MSRV so we can use the consts now.
It could happen that we unknowingly depend on a new version of a crate
without updating `Cargo.toml`. This could cause resolution issues for
downstream users. It's also unclear for outsiders to see which
dependencies we tested the crate with.
This change commits two lock files: `minimal` and `recent`. `minimal`
contains minimal depdendency versions, while `recent` contains
dependency versions at the time of making the change.
Further, this adds CI jobs to test with both lock files, CI job for
`internals` crate, removes old `serde` pinning and prints a warning if
`recent` is no longer up to date. (We may have to override it somehow if
any crate breaks MSRV.)
The documentation is also updated accordingly.
Co-developed-by: Tobin C. Harding <me@tobin.cc>
Closes#1230
1c3bbd4bf2 internals: Remove attribution from all files (Tobin C. Harding)
99673ab5c4 hashes: Introduce SPDX license identifiers (Tobin C. Harding)
984fe69448 bitcoin: Remove attribution from all files (Tobin C. Harding)
Pull request description:
Please note, whether or not we need a per-file license comment is out of scope for this PR. This PR leaves us with the most simple per-file solution possible and leaves the merit of per-file license comment to be discussed on another day.
Simplify the per-file license stuff by doing:
- Remove the attribution line from each file.
Currently we have a mishmash of attribution lines accompanying the SPDX identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
- Introduce SPDX license identifiers into `hashes` and remove attribution line (ie, make `hashes` uniform with `bitcoin`)
Required before merge please:
- [x] ack from apoelstra because as the library original author many of the changes in this PR remove his name
- [x] ack from Kixunil because he had some concerns in the issue descussion
Fix: #1816
ACKs for top commit:
Kixunil:
ACK 1c3bbd4bf2
sanket1729:
ACK 1c3bbd4bf2
apoelstra:
ACK 1c3bbd4bf2
Tree-SHA512: c5ac05c5eb23b3b6a760f707c344b22f5871a4dedee4990b1840f57e4cee1d38560ff4507c354bbf29bc8ff05a179d95d7e100fcf19bd93c5362344a352c7b5a
Currently we have a mishmash of attribution lines accompanying the SPDX
identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
Just remove all the attribution lines and be done with it. While we are
at it add an SPDX line to the few files missing it, whether this license
nonsense is even needed is left as an argument for another day.
dd4ad9444e Hardcode expected weight in txin_txout_weight_tests (Peter Todd)
Pull request description:
Rational: the expected weight is fixed so this both ensures we don't accidentally change it somehow, and makes it easier to re-use these test cases in other codebases (eg python-bitcoinlib).
ACKs for top commit:
apoelstra:
ACK dd4ad9444e
tcharding:
ACK dd4ad9444e
Kixunil:
ACK dd4ad9444e
Tree-SHA512: 4769a4bb8695f4f4c95e258bb5f06a232090b14c3d9159d6d5de2d09d7fc934a1b920b90cc09677a88fc0cf37ac21ed27794692dff2c73df4252c9551dc10fc2
2860aae1a5 fuzz: don't fuzz hashes against RustCrypto (Andrew Poelstra)
6467728202 fuzz: disable tests unless 'cfg(fuzzing)' is passed; update README for reproducing failures (Andrew Poelstra)
6e2ee5be66 fuzz: run 'cargo fmt' on all the fuzz targets (Andrew Poelstra)
9cfc0fcd81 fuzz: add contrib/test.sh so we at least 'cargo test' it in CI (Andrew Poelstra)
933ecb19e1 fuzz: fix warnings, clippy lints, 1.48.0 failures (Andrew Poelstra)
fd88e48696 fuzz: remove AFL support (Andrew Poelstra)
ab467cb091 fuzz: make hongfuzz fuzzing the default feature (Andrew Poelstra)
6f754df231 fuzz: add fuzzing README (Andrew Poelstra)
f093765efe fix fuzz.sh and cycle.sh to use generated lists of targets (Andrew Poelstra)
6534f22362 fuzz: auto-generate CI and Cargo.toml files (Andrew Poelstra)
8021034d86 rename travis-fuzz.sh to fuzz.sh; partially patch CI (Andrew Poelstra)
0be75f7edc move hashes/fuzz into main fuzz/ directory (Andrew Poelstra)
5a891dec2d move bitcoin fuzz targets into bitcoin/ subdirectory (Andrew Poelstra)
e3111c748b move bitcoin/fuzz into repo root; add to workspace (Andrew Poelstra)
Pull request description:
Several big changes here:
* Moves fuzzing to its own workspace with a `contrib/test.sh` etc so that CI will check that it compiles
* FIx all warnings, clippy lints, MSRV problems, etc.; mostly move to Rust 2018
* Merge `hashes/` fuzztests into workspace
* Rewrite all scripts; add file that auto-generates CI fuzz job and Cargo.toml so we don't have to manually keep these in sync
* Remove bitrotted and partial AFL support.
Supercedes #1422
I suspect the hashes fuzztests will actually fail since we haven't touched them in so long. Will address that if CI fails here.
ACKs for top commit:
sanket1729:
ACK 2860aae1a5
tcharding:
ACK 2860aae1a5
Tree-SHA512: b1aa3d6fac75fee51966f1d3f3245784e331bdea2a3fa7d6609bc4196c34f81acb7701faf8f269c3741568ea100438f24a2f06e75c8d01cb84c8b22d7886f1dd
Rational: the expected weight is fixed so this both ensures we don't
accidentally change it somehow, and makes it easier to re-use these test
cases in other codebases (eg python-bitcoinlib).
Seems we no longer need an explicit error handler, remove it.
I did not grok the reason (long thread link below) but just removed it
and checked that the embedded crates still ran correctly.
https://github.com/rust-lang/rust/issues/51540)
a54e1ceab1 Apply rustfmt (The rustfmt Tyranny)
38d11ce3da ci: Make release CI search for NEXT.RELEASE instead (Steven Roose)
dad3abd20f transaction: Rename is_coin_base to is_coinbase (Steven Roose)
Pull request description:
Alternative to https://github.com/rust-bitcoin/rust-bitcoin/pull/1795.
Keep the old method as deprecated and add doc alias. Also change internal usage of the method.
ACKs for top commit:
tcharding:
ACK a54e1ceab1
apoelstra:
ACK a54e1ceab1
Tree-SHA512: 52d9729bf83da164556d960f8867cb836ff57a0f619da3dd3620efffb28a974aac23b8085863ab0e072a4bdb2f13ac576efa43ad2eec9a271ad044227f4d00a4
6c61e1019e Fix pinning (schemars and MSRV) (Tobin C. Harding)
c8e38d6a5a hashes: Implement JsonSchema for sha256t::Hash<T> (Tobin C. Harding)
Pull request description:
This has grown due to now including pinning work also done in https://github.com/rust-bitcoin/rust-bitcoin/pull/1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And https://github.com/rust-bitcoin/rust-bitcoin/pull/1764 requires this PR.
- Patch 1 is unchanged
- Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to `schemars` - phew.
Fix: #1687
ACKs for top commit:
Kixunil:
ACK 6c61e1019e
apoelstra:
ACK 6c61e1019e
Tree-SHA512: eae4aa9700817bab6ad444e07709e8b1a4ffb1625e08be6ba399abde38bf6f8e5ea216a0836e2e26dfaddc76c392802cd016738ea6e753a1bca2584d9d2a9796
Done as is single patch to make sure all the docs and CI are in sync and
correct.
We currently pin the `schemars` dependency using `<=0.8.3` as well as a
the `dyn-clone` transient dependency in the manifest (`hashes` and the
extended test crate). This is incorrect because it makes usage of the
crate klunky (or possibly impossible) if downstream users wish to use a
later version of `schemars`.
Observe also that we do not have to pin `schemars`, we do however have to pin
the `serde` crate if either `serde` or `schemars` features are enabled.
Do so in CI and document in the readme file within hashes.
Currently we have a pin remaining from the old MSRV (`syn` due to use
of `matches!`).
Fix pinning by:
- Remove pin in manifest for `schemars`
- Fix pinning for MSRV in CI and docs (this includes documenting pinning
requirements for `schemars` feature because it is related to the other
pin of `serde`) in both `hashes` readme and main repo readme.
8f6317fbab Add predict_weight test for witness address types (yancy)
Pull request description:
Add a predict_weight test for address types with witness data
ACKs for top commit:
Kixunil:
ACK 8f6317fbab
apoelstra:
ACK 8f6317fbab
Tree-SHA512: 954908f068d6b0e8f7cc6bc6bffd110d490d7165cf2544ff0f936264761cb1f8e4da1e37fd5a6a34fd8b05f8d72817a3b340bbf968b29c9f538f10853347fdf0
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
29cb34eed7 Refactor Address struct and its methods (Harshil Jani)
Pull request description:
Closes#1755
In this PR the `as_unchecked` is added to the Address struct, which returns a reference to the same address but with the type Address<NetworkUnchecked>. Similarly, the `assume_checked_ref` is added to Address<NetworkUnchecked>, which returns a reference to the same address but with the type Address.
ACKs for top commit:
tcharding:
ACK 29cb34eed7
apoelstra:
ACK 29cb34eed7
Tree-SHA512: 75ba40883d9fb31026b0e94e8d3fdcf808ff38a4d10f61f99a1e14ddc48358da86bad44cd78563dc67aa5d39382a5493e73c03891317f361f590e39b6257cb84
91f45a214f Replace hardcoded values with compile-time hashing (Martin Habovstiak)
095b7958dd Make `sha256t_hash_newtype!` evocative of the output. (Martin Habovstiak)
Pull request description:
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1427
Depends on #1769
How do you like the syntax? Is weird `struct Foo = bar(..);` acceptable?
ACKs for top commit:
tcharding:
ACK 91f45a214f
apoelstra:
ACK 91f45a214f
Tree-SHA512: f6b555b20311a4c1cb097bc296c92459f6fbe16ba102d8c977eb2383dbcae3cc8ffce32e3cb771e7e22197fda26379971aa4feaadc5e2e70d37fa690ae8b8859
873501a85f Use slice patterns (Martin Habovstiak)
Pull request description:
Some code looks better with slice patterns. This changes `bip32` to use them.
ACKs for top commit:
apoelstra:
ACK 873501a85f
tcharding:
ACK 873501a85f
Tree-SHA512: 2e91b21d0f943c04f592112f241444ba3b1b1dad47ea6bea162f62cfbcaf98e383257cad144072b3807aaa0c122dabab638ac10b1cc1f5179727a5f05aac0659
This commit refactors the Address struct and its methods to improve
its functionality and usability.The AddressInner struct now holds
the payload and network, and the PhantomData<V> type is used to track
the network validation state.
Also as_unchecked and assume_checked_red methods are added to allow
conversion between checked and unchecked network validation state.
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
Previous changes enabled passing the string used as a tag into
`sha256t_hash_newtype!` macro rather than hard-coding midstate. This
commit takes advantage of it and replaces the hard-coded values with
compile-time executed (`const`) hashing.
The Rust API guidelines state that macros should be evocative of the
output, which is a sensible recommendation. We already had this for
`hash_newtype!` macro but didn't for sha256t version.
This changes the macro to have this syntax:
```rust
sha256t_hash_newtype! {
// Order of these structs is fixed.
/// Optional documentation details here. Summary is auto-generated.
/*pub*/ struct Tag = raw(MIDSTATE_BYTES, LEN);
/// Documentation here
#[hash_newtype(forward)] // optional, default is backward
/*pub*/ struct HashType(/* attributes allowed here */ _);
}
```
Closes#1427
We've upgraded MSRV but didn't update clippy config, so some things that
could be improved aren't caught by clippy. This updates the config and
fixes the new issues.
I also `rg '1\.41\.1'`ed for interesting changes and found one
additional improvement.
a189942c64 Use doc_auto_cfg (Tobin C. Harding)
Pull request description:
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of `#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
Props to pezcore for the lesson :)
ACKs for top commit:
apoelstra:
ACK a189942c64
Kixunil:
ACK a189942c64
Tree-SHA512: 1ced1e09f5d1733b362b83ca650d3f52c89eb57e78e8437f74c496d89776548f8c50feab6750352342e2abe680434681de2c126ce36a81dda21397b9695d4d4e
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
implementation of PartialEq<Address> for Address<NetworkUnchecked>
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
ed80df5ebc Add `ChainHash::from_genesis_block_hash` (Martin Habovstiak)
Pull request description:
This improves readability of converting `BlockHash` into `ChainHash`. It's useful in e.g. Electrum protocol which sends `BlockHash` (serialized backward).
Closes#1751
ACKs for top commit:
apoelstra:
ACK ed80df5ebc
tcharding:
ACK ed80df5ebc
Tree-SHA512: b03eb9ca0a1eb38196cc8e43751c2def0d00b463b774003c5319bf56599db598ddd4bb0e2ad0bd3803f21a925135992d421848e7b1ba5e7bab3a405f60fc973c
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
82b6332b91 create a set of recognized denomination forms (yancy)
Pull request description:
I took a stab at restricting the acceptable forms here. There was some consensus that "BtC" was confusing that was discussed in a previous pr https://github.com/rust-bitcoin/rust-bitcoin/pull/1715.
Also, personally I felt that the `PossiblyConfusingDenomination` enum variant was itself confusing. I think it's probably cleaner to just maintain a list of acceptable forms and treat everything else as unknown. For now I just created a const of possibly confusing forms.
ACKs for top commit:
Kixunil:
ACK 82b6332b91
apoelstra:
ACK 82b6332b91
Tree-SHA512: f3c212046e4d8a34ce2d50f34065b1778ae190ca80dd9ad73ef41eb5925705c88c0a764ec149d731eb1735d670c5542415953633a7f3f672d0c1e46b6e004d0b
This improves readability of converting `BlockHash` into `ChainHash`.
It's useful in e.g. Electrum protocol which sends `BlockHash`
(serialized backward).
Closes#1751
122188f7dd Use shorter import statements (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Use shorter import statements
As per discussion [0] use the shorter form for importing crates that we
re-export (`hashes` and `secp256k1`).
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1661
ACKs for top commit:
apoelstra:
ACK 122188f7dd
sanket1729:
utACK 122188f7dd
Tree-SHA512: 3f540464d38c72ba9d68f8ceda8600540bd0c3eef0ba67531c87fa1e0e4f757af7035cf80a1a5f17aa05604a17fdd9ef59bb6bece6b4145d540dac1e5362fc01
Add the `mutate` attribute to mutate `mul_u64`. Add non-doc comments
listing the two false positives. These are identical but we list them
twice so when devs grep for `mutagen false pos` the same number of lines
for each function is displayed as is displayed by the `mutagen` run.
This coding false positives thing is also introduced in PR #1655.
1dc04fe10f Remove rust_v_1_46 (Tobin C. Harding)
71fa9e81e7 Bump MSRV to 1.48.1 (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Bump MSRV to 1.48.1
As per discussion [0] bump our MSRV for all crates in `rust-bitcoin`
repo to 1.48.1 [1].
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1329
[1] https://blog.rust-lang.org/2020/11/19/Rust-1.48.html
ACKs for top commit:
apoelstra:
ACK 1dc04fe10f
sanket1729:
ACK 1dc04fe10f
Tree-SHA512: 533470c55f7aeede88382db8245033dac9317d75b38ced2ad8256d38319632a524335f893044e96300a1e60048f9aaca2d1634735eb324eb8ed9ad3c9ab2f872
dbd2ea07b5 Add kilo weight unit conversion (yancy)
Pull request description:
The FeeRate module defaults to sats per `kwu` so when doing fee calculations, it would be convenient to easily convert weight to the same units.
ACKs for top commit:
Kixunil:
ACK dbd2ea07b5
tcharding:
ACK dbd2ea07b5
apoelstra:
ACK dbd2ea07b5
Tree-SHA512: fe26b631cf474f1dca627a4d21e9052c80f8ada9a0eb635c46b7f8f42095671b9b161fb5012b723699008372faf7668507d5e92bde7d1808d389f85dba33529b
We just merged a patch to enable formatting in CI but commit: `05fdead2
Feature: Add difficulty_float method for block::Header.` must have
slipped in.
Run the formatter.
913575ac91 hashes: Run the formatter (Tobin C. Harding)
52c4579057 Enable formatting for hashes (Tobin C. Harding)
3f16b6bf9f util: Run the formatter (Tobin C. Harding)
d210d2ac83 Enable formatting for util (Tobin C. Harding)
5973dce9db blockdata: Run the formatter (Tobin C. Harding)
0dcbed3c7b Enable formatting for blockdata (Tobin C. Harding)
a52746d01c psbt: Run the formatter (Tobin C. Harding)
ef306db5e2 Enable formatting for psbt (Tobin C. Harding)
296f2ed82c Make test panic instead of using code comment (Tobin C. Harding)
3ec8a12428 crypto: Run the formatter (Tobin C. Harding)
c8a3c58786 Enable formatting for crypto (Tobin C. Harding)
314e6786b4 crypto: Add rustfmt::skip attributes (Tobin C. Harding)
450a84f6e8 consensus: Run the formatter (Tobin C. Harding)
89143205f9 Enable formatting for consensus (Tobin C. Harding)
ce773af20f tests: Remove useless use of super imports (Tobin C. Harding)
ef01f4d0f6 consensus: Introduce local variables (Tobin C. Harding)
Pull request description:
One final push crew, 16 patches, only a few are big.
All non-trivial formatting is done in separate patches so the changes can be verified mechanically.
With this applied the whole `rust-bitcoin` crate will be formatted.
Big thanks to everyone for putting up with the ongoing formatting PRs, no-one likes doing these but hopefully this an improvement to the project - especially in helping us get more contributors to the project.
ACKs for top commit:
tcharding:
> ACK [913575a](913575ac91). Went through the workflow locally.
sanket1729:
ACK 913575ac91. Went through the workflow locally.
apoelstra:
ACK 913575ac91
Tree-SHA512: b30eaa2893563155de05f8fa97be4a24a7dd8bf43bb426314c5104598477ca2173af279da796da8b18cc53a0ed525908b3d4edd0504836a443465efa0773632d
67618d679d Mark `Denomination` as `non_exhaustive` (Martin Habovstiak)
Pull request description:
It is possible that we will add new variants to `Denomination` in the future so making it `non_exhaustive` is better for forward compatibility.
ACKs for top commit:
tcharding:
ACK 67618d679d
apoelstra:
ACK 67618d679d
sanket1729:
ACK 67618d679d
Tree-SHA512: a28b7c6577f098b3d64c505e948e3b9fd0cc43a911a2b7c35610ada6a2f75514a1cb05ab8c99212340cf4e34426ac8b5f0ecc65d5afa22d67296e82d878b9308
2d23e11569 Remove extern crate hashbrown (Tobin C. Harding)
Pull request description:
(Merge candidate only after release of 0.30.0)
We no longer have a "hashbrown" feature, the feature gated `pub extern crate hashbrown` should have been removed when we removed the feature.
ACKs for top commit:
apoelstra:
ACK 2d23e11569
Kixunil:
ACK 2d23e11569
Tree-SHA512: 36d4c539f3f972d42bbc9fda9e96063a78d35afc6aebbf534b878dcef0440b72c2a990bcbdbb2ad3bf99cab7048cdbd4002899c2b314da21e4a7bacaf8c71f0f
Add `rustfmt::skip` attribute in a couple of places and then remove the
exclude for the `blockdata` module. Do not run the formatter, that will
be done as a separate patch to aid review.
Currently we have a code comment that is supposed to assist devs in
maintaining the `network::constants::Network` type by failing to build
if a new variant is added. This plays havoc with the formatter because
the comment is hanging at the bottom of a match block and the formatting
thinks its for the proceeding line of code.
Instead of using a code comment add a panic so the unit test fails if a
new variant is added to `network::constants::Network`.
In preparation for running the formatter introduce a couple of local
variables to reduce the line length and inhibit function call from being
split over multiple lines.
Refactor only, no logic changes.
bef7992ce5 Update readme to mention pin for 1.47 (Tobin C. Harding)
58033cf14e pin serde dep on 1.47 (Tobin C. Harding)
f5f4a33fa9 pin serde dep on 1.41 (Andrew Poelstra)
ee9b297e98 ci: update dupe check to whitelist syn (Andrew Poelstra)
6aa640ff8d update rust-secp to 0.27.0 (Andrew Poelstra)
Pull request description:
Also remove the spurious dev-dependency copy of rust-secp, which should've been updated to remove the "recovery" feature in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/545 and then been removed entirely in https://www.github.com/rust-bitcoin/rust-bitcoin/pull/1387
ACKs for top commit:
Kixunil:
ACK bef7992ce5
tcharding:
ACK bef7992ce5
Tree-SHA512: 7d1bf062dc6920bcafa85311b4b5ed348e31fb20aa21156e545bf7a4cc4c194e2b6dcfd0d3c6db3df337e4e2cd0f7b74f41ea421549521f07080feb88fbd9382
00b46d6d9d Indent functions (Martin Habovstiak)
d56d202aeb Support weight prediction in `const` context (Martin Habovstiak)
Pull request description:
**Notes for reviewers:**
This is something that I want to use in my code and hopefully reasonably easy to review, so if this can get into 0.30 that'd be really nice. No hard feelings if it doesn't.
I tried to put extra effort into making review easier by:
* intentionally "mis-formatting" the first commit so diff is smaller and easy to understand - see individual commits.
* copying patterns from non-const fn to const fn so it's obviously correct (includes same variable names)
* not bothering with the array trick in `VarInt::len` and simply accepting the limitation of Rust 1.46+ (I use 1.48 BTW).
**Description**
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
ACKs for top commit:
apoelstra:
ACK 00b46d6d9d
tcharding:
ACK 00b46d6d9d
Tree-SHA512: 5509886a68b4de5227db0e28d92a40be8de64592e0b189c519213db21bcfe98ca03d9a1936b1024729b97db69e8ec0b55fac870a7ce9bab0d0c9a47b2087990f
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
Note: this commit is intentionally unformatted to make diff easier to
understand. Formatting will be done in future commit.
It wasn't obvious that displaying address with alternate formatting
upper cases bech32 addresses.
This change adds information about this and also a note about the
compatibility of various wallets.
2158f88f1d Add a method to `pow::Target` for returning difficulty as an f64. (junderw)
Pull request description:
Closes#1703
This adds a conversion function to U256 to get an f64. We use the method shown in the following blog post.
https://blog.m-ou.se/floats/
Target::MAX was converted to a f64 and set as a const that is verified in a unit test.
The code is rather confusing, so I took a crack at explaining it in my comments as well. Please let me know if you want it cleaned up some more.
ACKs for top commit:
apoelstra:
ACK 2158f88f1d
tcharding:
ACK 2158f88f1d
Tree-SHA512: 7c0e82bd1756950c1c6dfb9da91fd71b276e2e7dc8a33e69112f87b87e358240f0f7c4894d24ab228e149d862938833a2e65e345ce2712a78dc86dec197da34f
This adds a conversion function to U256 to get an f64. We use the method shown in the following blog post. https://blog.m-ou.se/floats/
Target::MAX was converted to a f64 and set as a const that is verified in a unit test.
bfd401c96e bitcoin_hashes: add CHANgELOG (Andrew Poelstra)
d1b7b54e3a bump bitcoin-hashes version to 0.12 (Andrew Poelstra)
Pull request description:
It was a little tricky to bump the version number because of https://github.com/rust-bitcoin/rust-bitcoin/issues/1553. There are a couple other things I considered trying, which maybe we'll do for future releases, but I believe this works for now.
Maybe should wait for #1111.
ACKs for top commit:
tcharding:
ACK bfd401c96e
sanket1729:
utACK bfd401c96e. ChangeLog looks good to me, did not review whether all noteworthy changes were included.
Tree-SHA512: d2104fc93e364415ae955e8123e6087c1eaa4c955aeaf4647ead051a223563326f66c0e278d68f64335e22c9d0af9b296dc3b744cd9d82d206844461fe7bf9c9
e3f95ee22b Add tests for the FeeRate type (yancy)
Pull request description:
Adds some tests for the `FeeRate` type.
ACKs for top commit:
apoelstra:
ACK e3f95ee22b
tcharding:
ACK e3f95ee22b
Kixunil:
ACK e3f95ee22b
Tree-SHA512: 74d6597c747d5aa62a6510bf9fa8de971f89ad56f571aadd496f6487e80cc88bb2b5a1c6bcfed825d09d18ca2310b2bfd6fdbe330f2760369d167a653d26bef8
3eb648df01 Add constants to `InputWeightPrediction` (Martin Habovstiak)
Pull request description:
There are several common spends in Bitcoin that have known input weight predictions. It can be useful to have these as constants, so this change adds them. However, this only adds native segwit ones as the others are slowly fading away and might clutter the API.
If anyone wants other constants, please write them for me, their value is not that great to me so I'm not motivated to figure out the correct numbers. :)
This would be nice to add to 0.30 since it's small and easy but not critical.
ACKs for top commit:
apoelstra:
ACK 3eb648df01
tcharding:
ACK 3eb648df01
sanket1729:
ACK 3eb648df01
Tree-SHA512: 51d2cd2ecef7e6b79f9d4b52319e34b908fe8b5a337551dc2088994feeafc9c3ca87884c3db8369f8bd002947d6d14b373f08ef1419db282713206ed6f1b309a
There are several common spends in Bitcoin that have known input weight
predictions. It can be useful to have these as constants, so this change
adds them. However, this only adds native segwit ones as the others are
slowly fading away and might clutter the API.
Enable formatting in CI by doing:
- Add a section to the `test.sh` scripts to run the formatter (guarded by
the env variable `DO_FMT`) for all crates (bitcoin, hashes, internals).
- Add `DO_FMT` to the nightly `Tests` CI job.
Various formatting issues have crept into the codebase because we do not
run the formatter in CI.
In preparation for enabling formatting checks in CI run `cargo +nightly
fmt` to fix current formatting issues. No changes other than those
create by the formatter.
Because we have rust-secp in the loop, we need to update rust-secp, push
a new tag, and use that here, to ensure that the direct dependency on
bitcoin_hashes, and the rust-secp version, are compatible.
74022baa44 Rename ScriptLeaf to LeafNode (sanket1729)
289dc1e7f5 Remove serde for taprootspendinfo (sanket1729)
a397ab0c19 Remove serde for ScriptLeaf (sanket1729)
9affda3012 Introduce Hidden leaves in ScriptLeaves (sanket1729)
22bc39a143 Fix serde for TaprootMerkleBranch (sanket1729)
38ed9bdf49 MOVE ONLY: Move TapTree to taproot module (sanket1729)
Pull request description:
This PR changes/removes the serde implementation for the following types
- TaprootSpendInfo: Removed. This data structure contains derived information for taproot spending that cannot be validated easily. To elaborate, `TaprootSpendInfo` is constructed from a tree, but loses information about the tree structure and maintains handy information like `script_control_block_map`, cached tweaked key, Merkle root etc.
- TaprootBuillder: Removed. Hard to implement and not very useful.
- TapTree: Modified to check invariants.
- NodeInfo: Now implements serde with support for Hidden nodes
- ScriptLeaf: Removed serde. Users should not directly construct this. This is just an output iterator item of `TapTree::script_leaves()`
Data structure changes:
- Introduced `LeafNode`: Supports Hidden leaves. Has serde implemented
- Cleanly separate `TapTree` and `NodeInfo`: `TapTree` is a full BIP370 compatible tree with no hidden nodes. `NodeInfo` is a tree that can potentially contain hidden leaves.
- Added `NodeInfo::leaf_nodes`: Iterator that iterates over known and hidden leaves of `NodeInfo`.
- Updated `TapTree::script_leaves`: Iterator that is guaranteed to output known leaves.
ACKs for top commit:
tcharding:
ACK 74022baa44
apoelstra:
ACK 74022baa44
Tree-SHA512: 919ea5bf60dc0cd8431301c1b744b046d1d781b3bed302b83a600cfa644216b6c682752795d463646b2723ac8661879284f557862a67e4572fd965fcf3dc194d
b0b0cdb46c Improve the public API for Feerate and Weight (yancy)
Pull request description:
Small nit for https://github.com/rust-bitcoin/rust-bitcoin/pull/1627/ to re-export `Weight` and `FeeRate` to shorten the use path.
```
use bitcoin::Weight;
use bitcoin::FeeRate;
````
ACKs for top commit:
tcharding:
ACK b0b0cdb46c
Kixunil:
ACK b0b0cdb46c
apoelstra:
ACK b0b0cdb46c
Tree-SHA512: 81e508c980c4f37e3791d26616459608dd60e5a6255ef28b3a049c1d27281b2853b92474d42f10031254c8c46878adf666eb709826aa4ffde1b4b3542451e080
73e876ffd4 Include address in Error::NetworkValidation (Subhradeep Chakraborty)
Pull request description:
Fixes: #1677
## Change
In `bitcoin/src/address.rs`, a new field `address` is added to the enum variant `Error::NetworkValidation`. Also, the implementation of `Display` trait for `Error` is updated to print the `address` field.
However, to print the `address` through `Display`, either the reference is needed or `Address` and `Payload` both need to derive the `Copy` trait. Since I am little new to both the rust-bitcoin codebase and rust itself, I am confused about choosing between the two and have moved with the first one. Would appreciate any feedback on this.
ACKs for top commit:
Kixunil:
ACK 73e876ffd4
tcharding:
ACK 73e876ffd4
apoelstra:
ACK 73e876ffd4
Tree-SHA512: dd53b8648bccc8372c829e56817402fb02a9d51d1dffc854f24e68814bbefe7ea777f67aefb0d170762dbf6cdd50bd3ec55af325a1ffc21b1241d1df5531cd24
Implementing this for spendinfo is really complicated because it
contains some cached data without retaining the components that are used
to compute them.
Users should serde the 1) NodeInfo and 2) internal key and reconstruct
TaprootSpendInfo from it.
This was incorrect and not needed. Users should not be able to create
only tree leaves directly without going through the tree construction in
rust-bitcoin
Cleanly separate `TapTree` and `NodeInfo`. Fix serde not respecting
invariants for several data structures
Repurpose some tests from removed taproot builder for taptree
42b07586ac Improve the public API (Tobin C. Harding)
Pull request description:
We created the `crypto` crate as a container for cryptography modules with the idea that it may be split out into a separate crate. There is no reason for users of the lib to know about this module. Also, we have two `taproot` modules, one in `crypto` and one at the crate root, this makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from `crate::taproot`
Fix: #1668
ACKs for top commit:
apoelstra:
ACK 42b07586ac
Kixunil:
ACK 42b07586ac
Tree-SHA512: 5713b98b2a48d2776cdd6ca2c0e798d6e2df604d780e5182bd770fb2df807cf1f65bc24663ee6f7734fc1e0c80494c5a87dc60e44c83acefcffec7f059203a47
We created the `crypto` crate as a container for cryptography modules
with the idea that it may be split out into a separate crate. There is
no reason for users of the lib to know about this module. Also, we have
two `taproot` modules, one in `crypto` and one at the crate root, this
makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from
`crate::taproot`
7d1645aea0 Add constant for coinbase maturity (benthecarman)
Pull request description:
Not sure if this is the best place to put this but it is nice to have a constant for this instead of having other libraries make their own (ie https://github.com/lightningdevkit/rust-lightning/pull/1924#pullrequestreview-1222807626)
ACKs for top commit:
tcharding:
ACK 7d1645aea0
apoelstra:
ACK 7d1645aea0
Tree-SHA512: 5ac2a3359cadd303158c66ba45db8f4bf8cc80b6c19604262999ff361fd0bd98e2a4851c57da1962cb5c74f5789a85c8b3861f1742706a60ce1fbc57c3c200cc
56569b32ef Add utils to convert ChainHash to a Network (benthecarman)
Pull request description:
ACKs for top commit:
Kixunil:
ACK 56569b32ef
tcharding:
ACK 56569b32ef
Tree-SHA512: a489fcb1c1208db4271076d88288658988a63209e56e7433bde82d7d5719450433348fcc3cb6aae59ffa6ed8aff510d6b031c6899d5cf64c503a53b2d4c692b8
161273b209 Re-name hash inner/byte methods (Tobin C. Harding)
324b6f264b Use `into` for hash argument (Tobin C. Harding)
Pull request description:
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.
Fix: #1554
ACKs for top commit:
Kixunil:
ACK 161273b209
apoelstra:
ACK 161273b209
Tree-SHA512: b51b851a1855e6a26a7ef8ccb9f554723d4cc39b368812703587a50e81e7ab49714a81696af0be743b947f09e1fca227a5331b6735912c5b0d5cd0178905f006
a4b5fb4002 Fix docs for UnknownMagic to be accurate (benthecarman)
Pull request description:
I assume the old docs are a copy-paste error, strings are not involved when this error is encountered.
ACKs for top commit:
Kixunil:
ACK a4b5fb4002
apoelstra:
ACK a4b5fb4002
Tree-SHA512: b2b71f81be8a0d979b15471e7262e01284443e05626b26a19236fd25581700d9e37409576a4b73d5bb537c49ae83a4b7d40f0888dff078b07bd7550026cd778a
76c4c647cf Reexport `Magic` (Martin Habovstiak)
Pull request description:
Writing `network::Magic` is more natural and less annoying than `network::constants::Magic`, so this change reexports it.
Closes#1667
ACKs for top commit:
apoelstra:
ACK 76c4c647cf
tcharding:
ACK 76c4c647cf
Tree-SHA512: 1d85372ecd9723c0871faa4552ba5f93a3c95d0b213fa8aace398949f97157cdfde164bbcb2c35bae4c61ae0185e3fbdb854714cde0849c1afaf90e4e8ec223f
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
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
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
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
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
"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.
`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.
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.
dd316e4d14 pow: Remove Mul/Div by arbitrary integer types (Tobin C. Harding)
Pull request description:
When we added `Target` and `Work` types we implemented multiplication and division by anything `Into<u64>`, this is not typically done in the Rust stdlib and also is semantically incorrect for the types.
Remove `Mul` and `Div` impls from `Target` and `Work`. Also remove `Mul<T>` for `T: Into<u64>` from the private `U256` type.
Fix#1632
ACKs for top commit:
apoelstra:
ACK dd316e4d14
Kixunil:
ACK dd316e4d14
Tree-SHA512: ede53555844adab321ff344535b7b8bab3c5c73855823dfc3ad728b077ae199451b7e22a1d203ef73a076073b7f0cbf9637cefa5fe82fc78ab454d02fa0b62b9
When we added `Target` and `Work` types we implemented multiplication
and division by anything `Into<u64>`, this is not typically done in the
Rust stdlib and also is semantically incorrect for the types.
Remove `Mul` and `Div` impls from `Target` and `Work`. Also remove
`Mul<T>` for `T: Into<u64>` from the private `U256` type.
272cdbcf7c Flatten the types directory (Tobin C. Harding)
Pull request description:
We recently created a `types` subdirectory under `script` to keep all the `Script` and `ScriptBuf` impls together. Turns out this additional level of subdirectory is a bit annoying and we can achieve the same grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two submodules up a level, remove the `types` directory.
Fix: #1640
ACKs for top commit:
Kixunil:
ACK 272cdbcf7c
apoelstra:
ACK 272cdbcf7c
Tree-SHA512: 91fd78084829fa24f3b6420602d7d5094670647fff43e6e193d6de3126f1657132873ea133540d87db7d0d4dfc4cb9666489e39c861377085ce0254da81fd564
ae2aaaa436 Add `script_pubkey_lens` method (Martin Habovstiak)
cf068d16b0 Implement transaction weight prediction (Martin Habovstiak)
Pull request description:
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
Based on #1627
ACKs for top commit:
apoelstra:
ACK ae2aaaa436
tcharding:
ACK ae2aaaa436
Tree-SHA512: 55376601c2c2826bb0909cc25ff5b65816f0b1a2d57fb2cd8831f3db5382de0f4a364d518b312f0528bb5f44c30f3f74f8d254145eed2bfd65e2332b7c4d7c8b
6be89bf94f Add `minimal_non_dust` to `TxOut` (Martin Habovstiak)
Pull request description:
In some scenarios it's useful to create outputs with minimal relayable value. E.g. outputs designated for fee bumping using CPFP. A method for this is useful.
This implements a constructor of `TxOut` that computes the minimal non-dust value from the passed script.
Closes#1459
This one is quite easy, so if we could get it in 0.30, that'd be great.
ACKs for top commit:
apoelstra:
ACK 6be89bf94f
tcharding:
ACK 6be89bf94f
Tree-SHA512: f31ae5f649fbba95ccaabf465cb814df193e7ef89c6e0de7b316a2a484e172beada0da8851da96b195a69a4da1b0991741d4c119f9b0c94fff34150e4f033bd5
We recently created a `types` subdirectory under `script` to keep all
the `Script` and `ScriptBuf` impls together. Turns out this additional
level of subdirectory is a bit annoying and we can achieve the same
grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two
submodules up a level, remove the `types` directory.
In some cases people construct the transaction with a dummy fee output
value before calculating the weight. A method to create the iterator
over `script_pubkey` lengths is useful in such cases.
097e4e9c7f Fix license on bip158 module (Tobin C. Harding)
Pull request description:
When we introduced the SPDX license blurb in [0] we incorrectly gave attribution to Andrew when the original file author had the attribution as "the rust-bitcoin developers". The original author [1] was Tamas Blummer and he copied this code from code he wrote and explicitly re-licenses it. In order to make the re-licensing comment a little clearer and fix the mis-attribution use Tamas' name in the attribution.
[0] commit: `91ff2f628ce7db732d234a812e29fa8508f501a1 Introduce SPDX license identifiers`
[1] commit: `c93a70487f81a93c7d479ae046c75590d9fb7733 Add client side block filter (BIP158) (#281)`
ACKs for top commit:
apoelstra:
ACK 097e4e9c7f
Kixunil:
ACK 097e4e9c7f
Tree-SHA512: cb80d32c739ad562b2d657a34355bb28b1dd5c477b03018fbfbb14de40e03b806663aee89b578bcd8c681b067aa8d02611d4cde36e6fb9a8fa84ad4baf2e290e
6d99d3c061 Use ignore to stop rustdoc code from being built (Tobin C. Harding)
Pull request description:
Currently we have an attempted tag ```compile_fail that seems to be aiming at allowing code that does not build to exist in rustdoc. This is causing an error when running tests.
No clue how this made it through CI.
Use ```ignore to prevent rustdoc code from being built.
ACKs for top commit:
apoelstra:
ACK 6d99d3c061
Kixunil:
ACK 6d99d3c061
Tree-SHA512: 6c4b076000ba29377ac8cf942df66e849ff6421da6f9214664d487550cf45889e163b4de652079010bae327019163b63a1962ff8e6a04d918db63ffb0285ccd1
5f86b3091c Add From<Address> for ScriptBuf (Tobin C. Harding)
Pull request description:
Add an implementation of `From<Address> for ScriptBuf` that calls through to `address.script_pubkey` (which calls
`address.payload.script_pubkey()`).
Fix: #1457
ACKs for top commit:
apoelstra:
ACK 5f86b3091c
Kixunil:
ACK 5f86b3091c
Tree-SHA512: 8a45f292578765b345863946b276607d561b9bc75f6b9bb97f48b32d503143e234aedb658997db802c87289576361ec9ee6cb31fe3bbccfc06cc2fdabc7c41bb
In some scenarios it's useful to create outputs with minimal relayable
value. E.g. outputs designated for fee bumping using CPFP. A method for
this is useful.
This implements a constructor of `TxOut` that computes the minimal
non-dust value from the passed script.
Closes#1459
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
41f2dcf6ae Improve test coverage for docs build (Tobin C. Harding)
b4c14a4b7c hashes: Use automatic link (Tobin C. Harding)
96e8a080d1 ci: Remove redundant || exit (Tobin C. Harding)
Pull request description:
Currently the docs build commands in `hashes` and `bitcoin` differ, they should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched the feature guarding but since CI only runs `cargo rustdoc` with custom compiler conditional set we didn't catch it.
Done after seeing: https://github.com/rust-bitcoin/rust-bitcoin/pull/1504 and CI should fail on this PR until 1504 is in.
ACKs for top commit:
apoelstra:
ACK 41f2dcf6ae
Kixunil:
ACK 41f2dcf6ae
Tree-SHA512: 7cde68292cfc6f32b75d066e188e7c418ee251f9a5abc57fbd642ba33e9cd5bd8ef7c5ba7cffd206acae6ddec2f8c3db38c8c911a4319e979158666b8225953d
a7117bf8f1 Document source of logic fro read_scriptint (Tobin C. Harding)
2eb2420b40 Add comment on rountripping read/write scripint (Tobin C. Harding)
657dd51e8b Use OP_0 to better mimic bitcoin core code (Tobin C. Harding)
31d254a6a8 Fix push operators URL (Tobin C. Harding)
84cd4ca964 Deprecate script::read_uint (Tobin C. Harding)
Pull request description:
Patch one does the deprecation, the rest of the PR is made up of tiny improvements to the code around reading/writing 'scriptint's (conceptually `CScriptNum`s). I did all this while trying to decipher the discussion on #1547.
### Note Please
There are many more changes in the pipeline for all this read/write "script int" stuff. This PR was done ages ago and I believe it stall adds value.
I re-did the whole PR manually because of the recent `script` module changes. I hope no one else has to do that - if you do please feel free to holla and I'll "rebase" your PR for you.
ACKs for top commit:
Kixunil:
ACK a7117bf8f1
apoelstra:
ACK a7117bf8f1
Tree-SHA512: 5e8ee7fa8d1393a1a50e4241dd947b837cc0ddd15ff1239a49e4839489459fb95d184d6773f73633d55c436310bfab0c73f806d492ed4a4215f924c6c0993936
1e0e712bb0 Add push_* methods for lock times (Tobin C. Harding)
Pull request description:
Lock times are `u32` and can require encoding using 5 bytes.
Add methods `push_lock_time` and `push_sequence` for pushing absolute lock times and sequence numbers. We do not push relative locktimes because they are only 16 bits from the original sequence number.
ACKs for top commit:
Kixunil:
ACK 1e0e712bb0
apoelstra:
ACK 1e0e712bb0
Tree-SHA512: 4b511679270e7ef73937259ccf7d1b9b4b7512b2464f302310519a6e02d55c9cc24e3559302aeb671156e68130478258c1c565f474880e8be708b0ee234e67ff
8596e402f2 Fix docs.rs to use all features (Tobin C. Harding)
89086d094d hashes: Use angle braces to make hyperlinks (Tobin C. Harding)
9b5c2ad7af hashes: Clean up optional dependencies (Tobin C. Harding)
5b4f19c01f hashes: Improve std/alloc features (Tobin C. Harding)
132d2f90b6 bitcoin: Enable alloc feature in features list (Tobin C. Harding)
aa62ca224a hashes: Do not enable core2/alloc feature (Tobin C. Harding)
c15f8dee29 Improve manifest package section (Tobin C. Harding)
12f5e37ed9 Add excludes to manifests (Tobin C. Harding)
Pull request description:
Do a complete overhaul of the manifest of the top level crates (i.e., not `embedded`, `fuzzing` ect.).
Many of the problems being fixed here were introduced over the last year by my poor understanding of exactly what was going on with _every_ line of code in the manifests, after this PR I hope that is no longer a problem.
I'm closing #1571 because it is now done more fully at the end of this PR.
During review please be liberal with any questions so we can ensure everything is spot on now - as we add more crates there are going to be a proliferation of manifest files, best get it right now.
ACKs for top commit:
apoelstra:
ACK 8596e402f2
Kixunil:
ACK 8596e402f2
Tree-SHA512: 9e4ae21221cd5b185c04b6d1af73983d324e6d4e09abf94763f6eabcff420d9a3b2ec62a88cd20844fda518756862b0e64a85be54187166f3b1b2b206b82cb08
861fdd6ab1 Put the `MerkleBlock` struct at the top of the file (Tobin C. Harding)
f0d968197a Put error at the bottom of the file (Tobin C. Harding)
19e094788f Use self for Error variant imports (Tobin C. Harding)
83c2a552db Put helper function below where its called (Tobin C. Harding)
5076579fb9 Fix indentation in pmt_tests macro (Tobin C. Harding)
a7edbfb52e Move hex data to tests/data (Tobin C. Harding)
Pull request description:
PR 2 in the `merkle_tree::block` series, used to be on top of the now merged #1374.
Do a bunch of refactorings in preparation for more invasive changes. This is a separate PR because, other than the first patch which moves hex strings to `tests/data/` the other patches are refactoring only patches, no logic changes. However the last patch is big and will be annoying to review - sorry about that. If you really oppose this basically stylistic patch putting important things first, the opposite of C code, please say and I'll try to stop doing it.
ACKs for top commit:
Kixunil:
ACK 861fdd6ab1
apoelstra:
ACK 861fdd6ab1
Tree-SHA512: 3da0a600898f490b602ab05a396061587d86ffef55697877885a8c611eff96e7382a2d816fe9594c100d378dc56fe7fdc88009a0343bc602b7f4c180836adbd3
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`.
Lock times are u32 and can necessitate encoding using 5 bytes. As such
they are "special".
Add methods `push_lock_time` and `push_sequence` for pushing absolute
lock times and sequence numbers. We do not push relative locktimes
because they are only 16 bits from the original sequence number.
Our `script::read_scriptint` function is based on the constructor
code (incl. call to `set_vch`) code from Bitcoin Core. Add rustdoc
comment saying so, emit a link because there are already multiple links
to `script.h` in this file (one just right below the added comment).
We only support reads of upto 4 bytes where as Bitcoin Core allows
reading a `CScriptNum` with more bytes than that. Add a rustdoc
comment (incl. link to Bitcoin Core) mentioning that.
Our `Builder::push_int` method is the same as Bitcoin Core `CScript`
`push_int64` method. We currently use `OP_FALSE` (equivalent to `OP_0`)
but recently we added `OP_0`, lets use it to make our code better mimic
Core (also saves devs checking that `OP_FALSE` is the same as `OP_0`).
The `MerkleBlock` struct is the main type in this file, put it at the
top of the file. This leaves the next most important type,
`PartialMerkleTree` below that.
Refactor only, no logic changes.
70cf4515db Add `Weight` and `FeeRate` newtypes (Martin Habovstiak)
Pull request description:
Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion.
Part of #630
Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.)
ACKs for top commit:
apoelstra:
ACK 70cf4515db
tcharding:
ACK 70cf4515db
Tree-SHA512: ab9cc9f554a52ab0109ff23565b3e2cb2d3f609b557457b4afd8763e3e1b418aecbb3d22733e33304e858ecf900904a1af6e6fdc16dc21483b9ef84f56f103b2
a9108d3939 Refactor script module (Tobin C. Harding)
Pull request description:
The `script` module is large and unwieldy.
Refactor the `script` module, splitting it up into a tree of modules. Here are a few of the changes and their stated benefits
- Split the two script types out into separate files: Readers of the methods can then tell immediately from the file name which type they are reading.
- Put all the impls for the two script types together: Makes parsing the API easier because one can more quickly see which traits are implemented on what i.e., all the `AsRef` imlps are grouped together.
- Put the impls for the two script types in order, first `Script` then `ScriptBuf`: Makes it easier for us to see if we missed something.
- Put the `Builder` and `Instruction` (and associated) types in their own modules: Some devs find long files hard to navigate, so far there hasn't been too much push back against short files.
- Put tests in a separate file: This idea was recently discussed.
This is only moving code and fixing import statements etc. No other changes to the code.
## Note to reviewers
This PR is impossible to review from the diff because it moves so much code. Perhaps better to look at the resulting `src/blockdata/script/` directory and see if you like it.
#### Motivation
While adding script tagging I was having difficulty navigating the script module.
ACKs for top commit:
apoelstra:
ACK a9108d3939
Kixunil:
ACK a9108d3939
Tree-SHA512: 19123c8cfbdce6c42b322fa75a74073a0114b0ed21bd06ca5727981b3573b74cf05075723b774b92ae2b497e20644fca6e2fac14e30cc44f2802dde5aa567f66
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.
Part of #630
In `bitcoin` when we use the `core2` dependency we always need the
"alloc" feature. Enabling "alloc" when enabling "core2" in the "no-std"
feature is confusing because it makes it seem that we don't always need
it.
Set usage of the "alloc" feature of `core2` in the `features` list.
`core2` is for Read/Write, nothing to do with allocation and we do not
use the "alloc" feature of `core2` in `hashes`.
Fix core2 dependency/features by doing:
- Explicitly enable "bitcoin_hashes/core2" in `bitcoin`.
- Do not enable "core2/alloc" in `hashes`
Improve all manifest package sections by doing:
- Order the list of options uniformly
- Remove unnecessary homepage option (currently same as repo)
- Add categories section
We can check which files are included in the packaged release with
`cargo package --list `.
Add an `exclude` section to each manifest that excludes `tests/` and
`contrib/`. Not all crates have a `tests/` directory yet but they should
so add the exclude anyway to future proof the crates.
32d2d62e0f Rename from_slice methods to decode (Tobin C. Harding)
Pull request description:
The `TaprootMerkleBranch` and `ControlBlock` both have methods on them called `from_slice` but these methods do more that just basic copy from a slice. `decode` is a more descriptive name.
Deprecate the `from_slice` methods and implement `decode`, on other changes to the logic.
cc sanket1729
ACKs for top commit:
apoelstra:
ACK 32d2d62e0f
Kixunil:
ACK 32d2d62e0f
Tree-SHA512: e8c089545411a214ef9393f65d3990be46983000bd045182cc27dd70b62273bf48ac97adaf89d1e7fc807c72964a01eef176c7685684e8f87a01c219746d6d3d
55d17f2c18 Instruct devs to use nightly for embedded (Tobin C. Harding)
Pull request description:
The embedded test crate requires usage of the nightly toolchain, fix the docs to show this.
ACKs for top commit:
apoelstra:
ACK 55d17f2c18
Kixunil:
ACK 55d17f2c18
Tree-SHA512: 5d3d611ff4331d8475f77d260ebaa3da3ae71960b709eb4603056d7034885306b8bcc39be488f3391697d47a1074a695979fb50b6cb31af414f873da8e82bdbd
86f372774b Add '_ back into the BitStreamWriter (Tobin C. Harding)
Pull request description:
Recently we merged `commit 53d4fe66b57c255086def2b5e47afaddee776b75` to fix CI even though a better approach is to use `'_` because it assists reading the code (shows that the bit stream writer is not writing from a reference since its writing a `Copy` type `n`).
Add back in the `'_` (I forget what its called).
ACKs for top commit:
apoelstra:
ACK 86f372774b
Kixunil:
ACK 86f372774b
Tree-SHA512: 2a9989164562dbe7bf133e3aeb090fbff7831bfeefb0ac8431e75b17d57184c4d60ac206578c6ebbcff903a3832502a162027ed9f37e5ed87e42a6bf61efa594
The `script` module is large and unwieldy.
Refactor the `script` module, splitting it up into a tree of modules.
Here are a few of the changes and their stated benefits
- Split the two script types out into separate files: Readers of the
methods can then tell immediately from the file name which type they are
reading.
- Put all the impls for the two script types together: Makes parsing the
API easier because one can more quickly see which traits are implemented
on what i.e., all the `AsRef` imlps are grouped together.
- Put the impls for the two script types in order, first `Script` then
`ScriptBuf`: Makes it easier for us to see if we missed something.
- Put the `Builder` and `Instruction` (and associated) types in their
own modules: Some devs find long files hard to navigate, so far there
hasn't been too much push back against short files.
- Put tests in a separate file: This idea was recently discussed.
This is only moving code and fixing import statements etc. No other
changes to the code.
The `TaprootMerkleBranch` and `ControlBlock` both have methods on them
called `from_slice` but these methods do more that just basic copy from
a slice. `decode` is a more descriptive name.
Deprecate the `from_slice` methods and implement `decode`, on other
changes to the logic.
75b266a129 Improve `sighash` module documentation (Martin Habovstiak)
Pull request description:
"Sighash" is a technical term that newbies in Bitcoin may not know and could get lost when trying to find how to sign a transaction. This change attempts to make it more obvious that this module is needed for signing.
Closes#1463
ACKs for top commit:
tcharding:
ACK 75b266a129
apoelstra:
ACK 75b266a129
Tree-SHA512: 7157566c1639c63ce0fba2832e8e5e846e689d89e24077ed7769b721c5db4613cd7fd8d91464992eb78de74b42912ca877e7182a9c3c9c8848bf94d89767b8cc
b3188bbac3 Add `Transaction` accessors to `SighashCache` (Martin Habovstiak)
7c6854fe02 Use `Borrow` instead of `Deref` in `SighashCache` (Martin Habovstiak)
Pull request description:
This changes the bound from `Deref<Target = Transaction>` to `Borrow<Transaction>` (with respective `mut` changes) and adds accessors.
Closes#1423 (PSBT stuff will be separate issue).
ACKs for top commit:
tcharding:
ACK b3188bbac3
apoelstra:
ACK b3188bbac3
Tree-SHA512: 9db2c5890b26e9eefd483d697b42e84b1d7d3b8676fc39b4f39075c149e12697aa538828a757f9187578a958d72a592bb913f8f5788c93feb273db5370979d99
118a593c89 Implement from arrays for TaprootMerkleBranch (Tobin C. Harding)
Pull request description:
The `TaprootMerkleBranch` contains a vector of `TapNodeHash`s, as such it can trivially be constructed from an array of the same type.
Implement `From` for all array sizes 1 - 128 inclusive.
Fix: #1469
ACKs for top commit:
Kixunil:
ACK 118a593c89
apoelstra:
ACK 118a593c89
Tree-SHA512: dd497abd9143ea8b43485133beaccac9049fb915a95a3422d41c1f99961b59ec95df93efe759aa02f62ba1cf3e1afc4597671f1202ff0fa78eeee8b305d21305
The `TaprootMerkleBranch` contains a vector of `TapNodeHash`s, as such
it can trivially be constructed from an array of the same type.
Implement `From` for all array sizes 1 - 128 inclusive.
"Sighash" is a technical term that newbies in Bitcoin may not know and
could get lost when trying to find how to sign a transaction. This
change attempts to make it more obvious that this module is needed for
signing.
Closes#1463
277e8e96bd Add KeyPair import to rustdoc example (Tobin C. Harding)
Pull request description:
Recently, and bizarrely, a PR merged that broke `cargo test --doc`.
Add an import for `KeyPair` to the `schnorr` rustdoc example.
ACKs for top commit:
apoelstra:
ACK 277e8e96bd
Kixunil:
ACK 277e8e96bd
Tree-SHA512: ad214b668827b35848cc7b260cbd2104a916a82a5a6d242bdc498c62edc9a0e864f4bdb4abcade42924dbaf951223ae80feacbe68d8a4ccb4562d8ead50b23a9
bb612fdafa Set rustv_1_53 in build script (Tobin C. Harding)
Pull request description:
The rust version is supposed to be set by the build script so that users automagically get features matching the toolchain in use. Currently we have a feature in the manifest for `rustv_1_53` instead setting a compiler conditional configuration option in the build script. This causes `cargo +1.41.1 --all-features check` to fail.
## Note
I don't see `rustv_1_46` used anywhere, do we need that still?
ACKs for top commit:
apoelstra:
ACK bb612fdafa
Kixunil:
ACK bb612fdafa
Tree-SHA512: f74195d4ee5a5bc5f209e99d30789df3552cef10aee5ea8b61a5a701b753999c34d04be9fe0321ccee7a8ec14fa5a05e0b454b9dc5f8deddd7b5b8d4f3d7e744
It may be useful to access the transaction stored in `SighashCache`
during signing or afterwards, especially when the transaction is stored
without indirection (to enable long-lived storage).
This change adds the appropriate accessors.
The requirement for a type dereferencing to `Transaction` prevented
storing the cache in long-lived data without resorting to silly
wrappers. Since `Borrow` is implemented both for `T` and for smart
pointers it's a more flexible bound which this change implements.
While this is technically breaking, all usual non-generic code will
continue to work beause smart pointers generally have `Borrow`
implemented.
Currently we use a wildcard to export all the hash types in
`hash_types`. We are moving to a world were we only export
normal/standard types from the crate root.
Remove the reexport of the following hash types:
- `FilterHash`
- `FilterHeader`
- `TxMerkleNode`
- `WitnessCommitment`
- `WitnessMerkleNode`
- `XpubIdentifier`
- `Sighash`
Fix: #1541
f0e4e38844 Add newline in rustdoc (Tobin C. Harding)
Pull request description:
Docs created with the `sha256t_hash_newtype` macro are missing a newline between the doc heading and doc main section.
Note that the strings used span multiple lines and therefor the subsequent lines must be aligned with the start of the line (not indented).
Fix: #1540
ACKs for top commit:
Kixunil:
ACK f0e4e38844
apoelstra:
ACK f0e4e38844
Tree-SHA512: 240c68864da63688c400498903d5cc345bee224dcd3235df0127dcf391c66ee08c487d31fe59f890009c674574810b689d9a53628d07d8cdd46b79bc0ac3eb2b
Docs created with the `sha256t_hash_newtype` macro are missing a newline
between the doc heading and doc main section.
Note that the strings used span multiple lines and therefor the
subsequent lines must be aligned with the start of the line (not
indented).
Fix: #1540
The rust version is supposed to be set by the build script so that users
automagically get features matching the toolchain in use. Currently we
have a feature in the manifest for `rustv_1_53` instead setting a
compiler conditional configuration option in the build script. This
causes `cargo +1.41.1 --all-features check` to fail.
32ca6cc320 Remove hex_from_slice and display Sighash forwards (Tobin C. Harding)
a308e1e2ea Remove FromHex for all types except Vec and array (Tobin C. Harding)
3e70c01826 Manually format a bunch of vecs (Tobin C. Harding)
83e1c40c4d Remove script:: prefix from unambiguous types (Tobin C. Harding)
5ab5c264d2 Use fully qualified path in macro (Tobin C. Harding)
7e85452cd9 hashes: Implement std::error::Error (Tobin C. Harding)
5e3abc5e11 Fix feature gating on unit tests (Tobin C. Harding)
3344cf6be2 Favour $reverse instead of $reversed (Tobin C. Harding)
Pull request description:
This work started out, as the branch name suggests, as an effort to use the `hex_lit` crate. But once I got to this stage it seems that the `hex!` macro we have provides different, useful, functionality than the `hex_lit::hex!` macro (it allows usage with non-consts). So I'm unsure if we want to remove it now.
- Patches 1 - 6 are preparatory clean ups
- Patch 7 reduces usage of `FromHex`, please see git log for full description
- Patch 8 removes `hex_from_slice` and fixes a bug in how we display `Sighash`
ACKs for top commit:
apoelstra:
ACK 32ca6cc320
Kixunil:
ACK 32ca6cc320
Tree-SHA512: 11b45b39ec2fc0f837d7395b5fb86de7cc44641fd51cf7e93394a635e6a8fb1c7ac441a6070d5516dae60e084c04cc6e8b605a5167093f964679e445ef60c271
facaefc49c Add conversions for TweakedKeyPair -> TweakedPublicKey (Tobin C. Harding)
2407f241e4 Remove sep256k1 path from Parity (Tobin C. Harding)
Pull request description:
It is trivially possible to get `TweakedPublicKey` from a `TweakedKeyPair`, add conversion methods for doing so.
Patch 1 is preparatory cleanup. Please note `From` is not implemented because the conversion returns the `Parity` also.
Fix: #1452
ACKs for top commit:
apoelstra:
ACK facaefc49c
Kixunil:
ACK facaefc49c
Tree-SHA512: 597026c481fe2622a625cbeb381cac345af6f49f4a115418b69817345fc3c2140bbdbc5208eae1149d7d171f94c776365d302ffe1f9c01d944e738807db28a89
`Sighash` should be displayed forwards according to BIP143. Currently we
are displaying it backwards (as we do for double SHA256). This is
working because parse using `Vec::from_hex`.
We have the means to parse hex strings directly for hashes, we no longer
need `hex_from_slice`.
BIP143 test vectors display double SHA256 forwards but we display
backwards, this is acceptable because there is no fixed display in the
ecosystem for double SHA256 hashes. In order to overcome this we parse
test vector hex strings with into `Vec` when needed.
Remove `FromHex` from hash and script types
- Remove the `FromHex` implementation from hash types and `ScriptBuf`
- Remove the `FromStr` implementation from `ScriptBuf` because it does not
roundtrip with `Display`.
- Implement a method `from_hex` on `ScriptBuf`.
- Implement `FromStr` on hash types using a fixed size array.
This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
In preparation for modifying some unit test data structures, manually
format the code so it is uniform.
Move elements added to a vec with `vec!` onto a new line so they all
line up and one can better see what fields go where.
Refactor only, no logic changes.
ed6f6d11dd Implement fmt traits for ScriptBuf (Tobin C. Harding)
Pull request description:
We can improve ergonomics of the `script` module by implementing the `fmt` traits on `ScriptBuf`, trivial because we can call through to the `Script` implementations.
Fix: #1585
ACKs for top commit:
Kixunil:
ACK ed6f6d11dd
apoelstra:
ACK ed6f6d11dd
Tree-SHA512: 878a1522af4ed1e10d1d8d60d150e6571008c008b5e5c662c67462f9e09075b4f1fe4e399ed50e98cd7253b6815937c6732cd1ce02b74a5be017d5b8fcdbbd2f
We can improve ergonomics of the `script` module by implementing the
`fmt` traits on `ScriptBuf`, trivial because we can call through to the
`Script` implementations.
The `max_value` and `min_value` functions only exist to be
compatible/uniform with Rust 1.41.1 they will never change and they just
return a constant value. They can therefore be made const functions.
Recently we merged `commit 53d4fe66b57c255086def2b5e47afaddee776b75` to
fix CI even though a better approach is to use `'_` because it assists
reading the code (shows that the bit stream writer is not writing from a
reference since its writing a `Copy` type `n`).
Add back in the `'_` (I forget what its called).
877f9af364 Add new hex parse error variant (Tobin C. Harding)
Pull request description:
Recently we used an error type that holds only one expected hex string length when parsing but for `PublicKey`s we have two (66 and 130). Add a new error variant to express the error. Requires adding a variant to `bip32` for the same thing.
Fix: #1281
ACKs for top commit:
Kixunil:
ACK 877f9af364
apoelstra:
ACK 877f9af364
Tree-SHA512: c1ca493ee30418bd82bc326b35c18731260e4217c371f37301a73c64f9a6631163801acc217c6c2c7b14f632a2ad5043174266c1b4fdce127698e68ab8494f20
3c0598b399 Add standard constants to lock times (Tobin C. Harding)
Pull request description:
Some of the lock time structs (`Height`, `Time` ect.) are missing standard constants for min, max ect.
Add standard constants taking into consideration the various locktime corner cases.
Add `max_value` and `min_value` to be consistent with Rust 1.41.1 (incl. `Sequence`).
Fix: #1451
This PR is not complex in itself but **locktimes are notoriously complex, please wait for 3 acks before merging** - and ack'ing makes no guarantee that reviewer got all corner cases :)
There is no rush on this one, apoelstra, Kixunil, sanket1729 please just review when your brain is fresh.
ACKs for top commit:
apoelstra:
ACK 3c0598b399
Kixunil:
ACK 3c0598b399
Tree-SHA512: aa3d112db83b4785edb0a7a517cc335ded59f5967eb39b8979a6d68f9bba4644a27e5ca400fcabf368a1f8c0eecdef0b87b1011933ac7fd96b467b8501533203
Recently we used an error type that holds only one expected hex string
length when parsing but for `PublicKey`s we have two (66 and 130). Add a
new error variant to express the error. Requires adding a variant to
`bip32` for the same thing.
Fix: #1281
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.
Add an implementation of `From<Address> for ScriptBuf` that calls
through to `address.script_pubkey` (which calls
`address.payload.script_pubkey()`).
Fix: #1457
Some of the lock time structs (`Height`, `Time` ect.) are missing
standard constants for min, max ect.
Add standard constants taking into consideration the various locktime
corner cases.
Add `max_value` and `min_value` to be consistent with Rust 1.41.1 (incl.
`Sequence`).
Fix: #1451
a762a89b48 Add documentation to Sequence::is_final (Tobin C. Harding)
b1490a26ea Move enables_absolute_lock_time method (Tobin C. Harding)
Pull request description:
The term "final" is an archaic Bitcoin term however it is well used, it exists in Bitcoin Core code as well as in various bips. To help folks new to Bitcoin add documentation to the `is_final` method including historical notes.
Note, this does _not_ deprecate `is_final` - while writing the notes I found the term "final" in enough official places that I think its fair game to keep the term, some things people just have to learn, we can definitely help with that learning though.
Fix: #1198
ACKs for top commit:
Kixunil:
ACK a762a89b48
apoelstra:
ACK a762a89b48
Tree-SHA512: 895fbdce90223d90c0a68fb1e3d6b7aada4a3606d1294ea4df1f4194681a79d970b0434e7bb078f6d5cbf413b3550e72560d6d5cf811a5a959adf53f7f778ab2
8c0e5213d3 Delegate debug for ScriptBuf to Script (Tobin C. Harding)
Pull request description:
Currently the derived implementation of `Debug` for `ScriptBuf` prints the inner vector of u8s as integers, this is ugly and hard to read. The `Script` implementation of `Debug` prints the script opcodes and data as hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
ACKs for top commit:
Kixunil:
ACK 8c0e5213d3
apoelstra:
ACK 8c0e5213d3
Tree-SHA512: ca07d9fb191f4e0379cbd96b2944e6881094a8334d39b97209b6bf452a3c15d4aede53b9c88176b9b7667b7a539d47897940bc561dc9f8cd83ce1990a08047e1
1d3d5a9c5b Take Into<secp256k1::PublicKey> in PublicKey constructors (Tobin C. Harding)
b13a76407b keys: Clean up test imports (Tobin C. Harding)
Pull request description:
We can make the API more ergonomic by taking a generic argument that implements `Into<secp256k1::PublicKey>` in the `bitcoin::PublicKey` constructors.
The only thing than this is useful for is passing in `KeyPair` and the `From` implementation already exists. Add a unit test to verify.
Fix: #1453
## Note
As per the discussion in #1453 I checked secp and bitcoin for all keys that can be converted using `From` and it turns out its only `KeyPair` which already has `From` impls - good rust-bitcoin devs :)
ACKs for top commit:
Kixunil:
ACK 1d3d5a9c5b
apoelstra:
ACK 1d3d5a9c5b
Tree-SHA512: b5e5272561de15cdcfb15913aa5d42ddc96bf2fd5835068a5a9aa0274074ffa698ec9e81707f102b7d1b244f1abd0fdbd0eb4b6b505c84c3d5719dcb01d46efb
49e8b8da32 Use write_all for sighash encoding (Tobin C. Harding)
Pull request description:
From BIP143:
> If sighash type is SINGLE and the input index is smaller than the number of outputs, hashOutputs is the double SHA256 of the output amount with scriptPubKey of the same index as the input;
Currently we are using a `Sighash` which wraps double sha256 so while technically correct this means we are relying on `Sighash` to implement `Encodable`. We can remove this requirement by directly using the `sha256d::Hash` type to hash the outputs data.
Fix: #1549
ACKs for top commit:
Kixunil:
ACK 49e8b8da32
apoelstra:
ACK 49e8b8da32
Tree-SHA512: 8dd0037245a7cf180ba8a6eceeadad912d4adc14fc3f49df9008856de262624666d7d575195eea4868b2a5252dc565590e6be78471053b5e6367f3d2363310e8
e7bbfd3913 Improve Psbt error handling (DanGould)
Pull request description:
## Separate `encode::Error` and `psbt::Error` recursive dependency
This initial work attempts to fix#837's first 2 points
> - The current psbt::serialize::Deserialize has an error type of consensus::encode::Error. I think we should cleanly separate consensus encoding errors from application-level encoding errors like psbt.
> - There is a recursive dependence between encode::Error and psbt::Error which would need to be cleanly dissected and separated so that there is no dependence or only one-way dependence.
## Better `ParseError(String)` types
arturomf94 how compatible do your #1310 changes look to address #837's third point with this design?
> - There are a lot ParseError(String) messages that could use a better type to downflow the information.
I think your prior art would completely address this issue now.
## On handling `io::Error` with an associated error
`encode::Error` has an `Io` variant. now that `Psbt::deserialize` returns `psbt::Error` and produces an `io::Error`, we need an `Io` variant on `psbt::Error`. Except that doing so breaks `#[derive(Eq)]` and lots of tests for `psbt::Error`.
Kixunil, I'm trying to understand your feedback regarding a solution to this problem.
> I believe that the best error untangling would be to make decodable error associated.
> I meant having associated `Error` type at `Decodable` trait. Encoding should only fail if the writer fails so we should have `io::Error` there (at least until we have something like `genio`).
>
> > [it] is a problem to instantiate consensus::encode::Error in [the psbt] module for `io::Error`?
>
> It certainly does look strange. Maybe we should have this shared type:
>
> ```rust
> /// Error used when reading or decoding fails.
> pub enum ReadError<Io, Decode> {
> /// Reading failed
> Io(Io),
> /// Decoding failed
> Decode(Decode), // consensus and PSBT error here
> }
> ```
>
> However this one will be annoying to use with `?` :( We could have `ResultExt` to provide `decode()` and `io()` methods to make it easier.
>
> If that's not acceptable then I think deduplicated IO error is better.
Kixunil didn't we just get rid of Psbt as `Decodable`? Would this make more sense to have as an error associated with `Deserialize`? Or did we do the opposite of what we should have by making Psbt only `Serialize`/`Deserialize` because of #934, where only consensus objects are allowed to be `Decodable`? I wonder if we prioritized that strict categorization and are stuck with worth machinery because of it. My goal with #988 was to get to a point where we could address #837 and ultimately implement PSBTv2.
ACKs for top commit:
tcharding:
ACK e7bbfd3913
apoelstra:
ACK e7bbfd3913
Tree-SHA512: 32975594fde42727ea9030f46570a1403ae1a108570ab115519ebeddc28938f141e2134b04d6b29ce94817ed776c13815dea5647c463e4a13b47ba55f4e7858a
44d3ec487d Rename Payload::as_bytes to inner_prog_as_bytes (sanket1729)
a446df583c Make Payload non-exhaustive (sanket1729)
6ebc9de252 Introduce WitnessProgram struct and cleanup Address validity invariants (sanket1729)
41652caf05 Introduce is_spend_standard method (sanket1729)
Pull request description:
Fixes#1561.
Highlights:
- Segwitv0 programs with lengths apart from 20 or 32 are invalid `Address` struct. Such Addresses are useless and we should not parse/create them.
- Renamed `is_standard` to `is_spend_standard`.
ACKs for top commit:
apoelstra:
ACK 44d3ec487d
tcharding:
ACK 44d3ec487d
Tree-SHA512: 1ee36f7ea25c65619ddf7d643d025690096876843dbe6fbdf877ce23e88049d79b0bbd78cee6cf4b415bca028b3634bb70c9f52d1098bd90558e6ba7f8731332
Addresses with Segwitv0 not having len 20/32 are invalid and cannot be
constructed. Also cleans up a API bug in
ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.
ebfbe74243 Implement `Debug` for generic `Address<V: NetworkValidation>` (Jiri Jakes)
Pull request description:
Previously `Debug` was implemented for both `Address<NetworkChecked>` and `Address<NetworkUnchecked>`, but not for cases when the `NetworkValidation` parameter was generic. This change adds this ability. Based on Kixunil's tip.
With previous implementation, the `test_address_debug()` resulted in error:
The added `Debug` on `NetworkChecked` and `NetworkUnchecked` are required by compiler.
---
While dealing with derives and impls, I also attempted to turn all the derives on `Address` into manual impls (see Kixunil's suggestion in https://github.com/rust-bitcoin/rust-bitcoin/pull/1489#discussion_r1052448057). The motivation behind this was the possibility to remove derives on `NetworkChecked` and `NetworkUnchecked`, too. However, even with manual impls, all the traits on `NetworkChecked` and `NetworkUnchecked` were still required by compiler in this sort of situations (see also the rest of the same discussion linked above). I do not fully understand why, perhaps limitation of this way of sealing traits?
It can be demonstrated by removing `Debug` derivation on `NetworkUnchecked` and `NetworkChecked` in this PR and running `test_address_debug()`.
Therefore, if we want to allow users of the library to define types generic in `NetworkValidation` and at the same time derive impls, it seems to me that `NetworkChecked` and `NetworkUnchecked` will have to have the same set of impls as `Address` itself.
ACKs for top commit:
Kixunil:
ACK ebfbe74243
tcharding:
ACK ebfbe74243
apoelstra:
ACK ebfbe74243
Tree-SHA512: 87f3fa4539602f31bf4513a29543b04e943c3899d8ece36d0d905c3b5a2d76e29eb86242694b5c494faa5e54bb8f69f5048849916c6438ddd35030368f710353
We can make the API more ergonomic by taking a generic argument that
implements `Into<secp256k1::PublicKey>` in the `bitcoin::PublicKey`
constructors.
The only thing than this is useful for is passing in `KeyPair` and the
`From` implementation already exists. Add a unit test to verify.
Fix: #1453
Currently we have an attempted tag ```compile_fail that seems to be
aiming at allowing code that does not build to exist in rustdoc. This is
causing an error when running tests.
No clue how this made it through CI.
Use ```ignore to prevent rustdoc code from being built.
Currently the derived implementation of `Debug` for `ScriptBuf` prints
the inner vector of u8s as integers, this is ugly and hard to read. The
`Script` implementation of `Debug` prints the script opcodes and data as
hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 \
3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
When we introduced the SPDX license blurb in [0] we incorrectly gave
attribution to Andrew when the original file author had the attribution
as "the rust-bitcoin developers". The original author [1] was Tamas
Blummer and he copied this code from code he wrote and explicitly
re-licenses it. In order to make the re-licensing comment a little
clearer and fix the mis-attribution use Tamas' name in the attribution.
[0] commit: `91ff2f628ce7db732d234a812e29fa8508f501a1 Introduce SPDX license identifiers`
[1] commit: `c93a70487f81a93c7d479ae046c75590d9fb7733 Add client side block filter (BIP158) (#281)`
The term "final" is an archaic Bitcoin term however it is well used, it
exists in Bitcoin Core code as well as in various bips. To help folks
new to Bitcoin add documentation to the `is_final` method including
historical notes.
The `bip158` module uses a `HashSet` and in order to do so requires the
`hashbrown` dependency for "no-std" builds.
We can replace the usage of `HashSet` with a `BTreeSet` in `bip158` and
remove the `hashbrown` dependency entirely.
This patch makes no claims about performance cost or benefit of this
change. The patch also makes no claims about the validity of the current
`HashSet` usage.
The `hashbrown` dependency and `HashSet` usage can be trivially added
back in if someone comes up with perf data to back it up.
Previously `Debug` was implemented for both `Address<NetworkChecked>`
and `Address<NetworkUnchecked>`, but not for cases when the
`NetworkValidation` parameter was generic. This change adds this
ability.
In preparation for deprecating the `is_final` method; move the
`enables_absolute_lock_time` method to be directly above the `is_final`
method.
Refactor only, no logic changes.
`Sighash` does not need to implement `Encodable` because it is
claimed (I don't know exactly myself) that `Sighash` is never consensus
encode in Bitcoin.
We are currently relying on `Sighash` to implement `Encodable` when
encoding creating the segwit v0 sighash for a single input.
For reference, from BIP143:
If sighash type is SINGLE and the input index is smaller than the
number of outputs, hashOutputs is the double SHA256 of the output
amount with scriptPubKey of the same index as the input;
We can use `write_all` directly to write the hashed bytes and remove the
implementation of `Encodable` from the `Sighash` type.
While we are at it, use `write_all` to write the zero hash also to make
the code more uniform and understandable.
Fix: #1549
70fe07f1ce Export the DisplayHex trait from within prelude (Tobin C. Harding)
Pull request description:
We use `internals::hex::display::DisplayHex` in many places, we can improve ergonomics of the `internals` crate by re-exporting it from the `prelude` module.
ACKs for top commit:
Kixunil:
ACK 70fe07f1ce
apoelstra:
ACK 70fe07f1ce
Tree-SHA512: 96a89135cb0b829b7b5926a3b344f78e178b5b48e772a69da5133fab6d2e14e7b7bbaa56b7a417a5c1a64337546a1c7bac32307d3a1f27aa199ed61f590902bf
Currently the docs build commands in `hashes` and `bitcoin` differ, they
should be the same.
Add a command `cargo doc` to improve coverage e.g., recently we botched
the feature guarding but since CI only runs `cargo rustdoc` with custom
compiler conditional set we didn't catch it.
Run docs in CI using nightly and stable toolchains as required.
a7dd4b5ab0 Add a rustdoc test to Denomination (Tobin C. Harding)
Pull request description:
Add a rustdoc test to the `Denomination` type to show basic usage.
ACKs for top commit:
Kixunil:
ACK a7dd4b5ab0
apoelstra:
ACK a7dd4b5ab0
Tree-SHA512: 08f15c4641e70f043276b873e60fcf0e195fe50b6c5c18a245d2d609f4a4a4badc291aae1be532fc4890a91b2057cd308c86d0d3b85770b600a07499303a7bc4
Recently we introduced some mutation testing to the `pow` module but
testing is never done - add more `mutate` attributes and add unit tests
to ensure all mutants are killed.
Of note, the `from_compact` and `to_compact_lossy` functions are not
done, doing so results in a bunch of surviving mutants.
Parsing addresses from strings required a subsequent validation of
network of the parsed address. However, this validation was not
enforced by compiler, one had to remember to perform it.
This change adds a marker type to `Address` that will assist the
compiler in enforcing this validation.
We use `internals::hex::display::DisplayHex` in many places, we can
improve ergonomics of the `internals` crate by re-exporting it from the
`prelude` module.
1b0988833a Remove `ToHex` (Martin Habovstiak)
Pull request description:
The `ToHex` trait was replaced by either simple `Display`/`LowerHex` where appropriate or `DisplayHex` from `bitcoin_internals` which is faster.
This change replaces the usages and removes the trait.
ACKs for top commit:
tcharding:
ACK 1b0988833a
apoelstra:
ACK 1b0988833a
Tree-SHA512: a1b508e24ac247a0692c01b7cb1e7fa8f23fbfa3d6c3d5dfe669eec01f940a96c3f88508cb0b2e3529eebd9cb51e7d41435dbd5f4cbaf3bc14b9c7e7d790308b
a400757676 Add failing tests from serde-json (sanket1729)
b3246bf73f Fix LeafVersion serde (sanket1729)
Pull request description:
The default implementation maps to visit_u64. The current implementation
does not roundtrip with many deserializers, including serde_json. See
the failing test in the second commit
ACKs for top commit:
Kixunil:
tACK a400757676
tcharding:
ACK a400757676
Tree-SHA512: a6307b799b0bb4af398addc9ddbff0d811b632d1cc6ab4bdd77aaf3f151e48dd1cd10e3f90a71ef8fc3feb1cd988f7a719f92ec745b5fdf2ae0f0ad97ab2fa10
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
94b678e73f Rename `push_scriptint` and make it private (Martin Habovstiak)
Pull request description:
`push_scriptint` is a significant footgun with an unclear name. This renames it and unpublishes to avoid mistakes by downstream crates.
Closes#1517
ACKs for top commit:
apoelstra:
ACK 94b678e73f
Tree-SHA512: 9e1772c6fb326d8b0c78d702ad9926a79a91589feb8650aed7c5e75bfbdbf0164357b4d5b190877c40b8469e0e3be3d3453fe407741b5dae0c5758176f756417
c4363e5ba1 Deserialize Psbt fields, don't consensus_encode (DanGould)
c1dd6ad8a2 Serialize Psbt fields, don't consensus_encode them (DanGould)
1b7b08aa5d De/serialize Psbt without consensus traits (DanGould)
Pull request description:
fix https://github.com/rust-bitcoin/rust-bitcoin/issues/934
Instead of using consensus {de,en}code, serialize high-level structures (PartiallySignedTransaciton, Output, Input) borrow the signature from `Serialize`, `Deserialize` traits and implement them on Psbt:
```rs
impl Psbt {
/// Serialize a value as raw data.
fn serialize(&self) -> Vec<u8>;
/// Deserialize a value from raw data.
fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error>;
}
```
ACKs for top commit:
apoelstra:
ACK c4363e5ba1
tcharding:
ACK c4363e5ba1
sanket1729:
ACK c4363e5ba1. One small comment, but can be addressed in follow up if there is nothing else from other reviewers.
Kixunil:
ACK c4363e5ba1
Tree-SHA512: d8dd5ef1189b36fde08969b1ec36006a6fc55ae990f62ea744e6669e03a7f9e90e1d5907be7eac48ee1af23bc20a62aa7e60ff1ff78080d0e923bb9ccedcd432
The default implementation maps to visit_u64. The current implementation
does not roundtrip with many deserializers, including serde_json. See
failing test in the subsequent commit
089a1e452d Replace `Vec::from_hex` with `hex!` (Martin Habovstiak)
Pull request description:
This makes the code less noisy and is a preparation for changing it to `const`-based literal. Because of the preparation, places that used variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they won't be changeable to `const` and where `hex!` is unavailable (integration tests). These may be dealt with later.
See also #1189
Note that while the change appears big it's nearly entirely mechanical, so should be pretty easy to review. (But I don't feel it's `trivial`.)
ACKs for top commit:
apoelstra:
ACK 089a1e452d
tcharding:
ACK 089a1e452d
Tree-SHA512: c55357b8cffc86f8e107c0f8390490ede75948260018f63dc926455700cbcaf422f6ae72444f93943065e6f4ffa4335bee9160a64184ea4f8e91721f30a46ace
This makes the code less noisy and is a preparation for changing it to
`const`-based literal. Because of the preparation, places that used
variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they
won't be changeable to `const` and where `hex!` is unavailable
(integration tests). These may be dealt with later.
See also #1189
The additional `use` items were added to improve the style of
documentation. Because they were only used for doc they were `cfg`ed.
But because this is independent from being built by `docs.rs` the `cfg`
should've been `doc` not `docsrs`.
IOW `docsrs` means roughly `all(doc, nightly)` and the added items are
unrelated to `nightly`.
941083ec4e Remove rand-std dev-dependency from secp256k1 (Tobin C. Harding)
f71335f971 Add rand-std feature (Tobin C. Harding)
Pull request description:
This PR uncovered incorrect feature gating in `secp256k1`, fixed in https://github.com/rust-bitcoin/rust-secp256k1/pull/519
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the "rand" feature turn on "seck256k1/rand".
Fix: #1384
ACKs for top commit:
sanket1729:
ACK 941083ec4e
apoelstra:
ACK 941083ec4e
Tree-SHA512: e1d12196766c2b3082b488fe7d0c03a865ebbfc70ffa6f128c57074df14da71e56c31ea92982991d376ac62fa86c6639049ce17aac93613b523ddcc99677c269
64f7d2549e Fix wrong newtype APIs (Martin Habovstiak)
Pull request description:
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to `impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions and infallible conversions from arrays
Closes#1336
Notably this change is much less bad than even I expected and some places actually get cleaner than before.
ACKs for top commit:
tcharding:
ACK 64f7d2549e
sanket1729:
code review ACK 64f7d2549e
apoelstra:
ACK 64f7d2549e
Tree-SHA512: 0c5ab19aa1a01ccf34ad1c2f3f66bd003a3142a840cb7cf57e1449e721f6eed1523555c62f0e3391012708767ff86315ff7edce9afd85017a67c61cfe57dddbc
ca471557a5 locktime: Add mutation testing (Tobin C. Harding)
26c0da41b4 locktime: Add inline to public functions (Tobin C. Harding)
Pull request description:
Add mutation testing to the `locktime` module and add unit tests to cover all mutants and ensure they are killed.
ACKs for top commit:
sanket1729:
ACK ca471557a5. How stable are the mutation tests?
apoelstra:
ACK ca471557a5
Tree-SHA512: 46a59c90fc25b0c803e96f7c5b98bd39055f7835e45ba137b2a01ad4221a676c54bc228b9ef7663b7300bb4260a6c2c80a0820c4f1bf0987650e1e2bd699f62d
cf9733d678 Verify and fix mul_u64 (Tobin C. Harding)
Pull request description:
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in the current implementation due to overflow. Re-write the `mul_u64` method.
Fix: #1497
## Note
This PR now _only_ tests `mul_u64`, I will add testing div as a separate PR.
ACKs for top commit:
Kixunil:
ACK cf9733d678
apoelstra:
ACK cf9733d678
Tree-SHA512: c044fb385073bb068412195c079f26057ba560b2a7eca22693c360d324a7c0119cf10da2e70e544c69e05ca8269a81156418d307f4e11d8a4c1c1a7cafa32d0c
5a2a37d4be Allow dead_code/unused_imports when fuzzing (Tobin C. Harding)
Pull request description:
Littering the codebase with `#[cfg(not(fuzzing))]` is a bit messy just to quieten the linter during fuzzing. Instead just globally allow.
Done while debugging #1409
ACKs for top commit:
sanket1729:
ACK 5a2a37d4be
apoelstra:
ACK 5a2a37d4be
Tree-SHA512: fb84215a2b00ad6d3321b2781ba285af513ff8fd413c0997045a41c4f23028d2ef0fdf083839289d0c5108c990aa66bdae4430ad3ef32881eac5324b2e881b3b
e0bc50953a Make `Witness::tapscript()` return `Script` instead of raw bytes (Jiri Jakes)
Pull request description:
Since there is unsized `Script` now, this method can return it.
ACKs for top commit:
sanket1729:
utACK e0bc50953a
tcharding:
ACK e0bc50953a
Tree-SHA512: 32d4ca14f1b0fc1029f7376b1a43db90332b869a806609c82f660cb2690a4f0e1b91e1799fdac0d43c8a630aed0331f251d4159a662e86e5942c6fb698c42cd2
Add `#[inline]` to all public functions/methods excluding error types
and `Display` impls. Error paths do not need to be fast and presumably
`Display` is called on code paths that do IO so this also does not need
to be fast.
Add kani verification for `U256::mul_u64`, doing so uncovered a bug in
the current implementation due to overflow.
Re-write the `mul_u64` method.
Props to Elichai for the algorithm.
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
920599da94 Add test for previous commit (Martin Habovstiak)
a7f3458c27 Fix bug in `ScriptBuf::extend` for short iterators (Martin Habovstiak)
Pull request description:
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum length.
ACKs for top commit:
sanket1729:
ACK 920599da94 . Tested that the bug is correctly fixed and tested in the new test
tcharding:
ACK 920599da94
Tree-SHA512: a80f5f262a840d8e77efd42d63c511224380ee3efa6c31855233e81c90332ac15db228e8d552d039d729d7b642e03c3939c8b6a92d3279001377515acb83abea
8ce928b8e7 Add testing section to readme (Tobin C. Harding)
2e79a0bdc4 Introduce mutation testing (Tobin C. Harding)
Pull request description:
Introduce mutation testing by way of mutagen [0] (see #1484 for context).
- Conditionally add the dev-dependency `mutagen` (using `do_mutate` flag)
This flag is not very well named but `mutagen` and `mutate` are already taken?
- Mutate all methods of the `U256` struct that do not require additional unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need to check on `test` as well i.e., I don't understand why we cannot use `cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen` (doing so runs 29 mutants).
[0] https://github.com/llogiq/mutagen
ACKs for top commit:
Kixunil:
ACK 8ce928b8e7
apoelstra:
ACK 8ce928b8e7
Tree-SHA512: 024ba5d2dc983f7cd0444e09ba13280771157204999d2a44502e07a1d6436f571b75003c7cb543c943f17949b848d4070eda4e194bda774a3e41443ff79af0af
This fixes several API bugs:
* Use `TryFrom` instead of `From` for fallible conversions
* Move byte conversion methods from `impl_array_newtype` to
`impl_bytes_newtype`
* Add missing trait impls like `AsRef`, `Borrow`, their mutable versions
and infallible conversions from arrays
Closes#1336
`ScriptBuf::extend` contained an optimization for short scripts that was
supposed to preallocate the buffer and then fill it. By mistake it
attempted to fill it from already-exhausted iterator instead of the
temporary array it drained the items into. This obviously produced
garbage (empty) values.
This was not caught by tests because the optimization is only active for
scripts with known maximum length and the test used `Instructions` which
doesn't know the maximum lenght.
In order to get better test coverage we should not enable the secp26k1
feature "rand-std" in dev-dependencies but instead feature gate tests
that depend on this feature.
Currently we enable "secp256k1/rand-std" in the "rand" feature, this is
incorrect because it means "rand" implies "std" which it does not.
Add a "rand-std" feature that turns on "seck256k1/rand-std" and make the
"rand" feature turn on "seck256k1/rand".
Introduce mutation testing by way of mutagen [0]
- Conditionally add the dev-dependency `mutagen` (using `do_mutate`
flag)
This flag is not very well named but `mutagen` and `mutate` are already
taken?
- Mutate all methods of the `U256` struct that do not require additional
unit tests.
Uses `cfg(all(test, do_mutate), mutate)` - I cannot workout why we need
to check on `test` as well i.e., I don't understand why we cannot use
`cfg(do_mutate, mutate)`?
With this applied test can be run as usual with a stable toolchain. To
run mutagen we use `RUSTFLAGS='--cfg=do_mutate' cargo +nightly mutagen`.
[0] https://github.com/llogiq/mutagen
This is useful when one already has bytes allocated in a vec that can be
reused.
The change also documents that the mirror method `into_bytes()` doesn't
allocate.
bae264d0c2 Add `tapscript_leaf_hash()` to `Script` (Jiri Jakes)
Pull request description:
Adds convenience method to `Script` for computing leaf hash of tapscript. Closes#1482.
The little test case is taken from `bip341_tests.json`.
ACKs for top commit:
Kixunil:
ACK bae264d0c2
apoelstra:
ACK bae264d0c2
Tree-SHA512: fb7a3a552017208decd56ca7d27eab1987a3a92aae5b8620896b3a02986c2fc13043c200ccfbadf9cfdd2d74af38b0bc25936338f55b7d318c1296acc88bf22a
6acf9ac8b8 Patch hashes and update the code (Martin Habovstiak)
Pull request description:
This patches `bitcoin_hashes` to use the version in the repository and fixes the code after removal of `Deref`.
ACKs for top commit:
tcharding:
ACK 6acf9ac8b8
apoelstra:
ACK 6acf9ac8b8
Tree-SHA512: b779fa79309f1d648020146b58e96346b67e9f76e29551cbd50251ea6bb7bcfc4c5d42f49cc7ad5660dcd0a5f6306efe96dfcd9530e4b32c62edf4af7076d830
This patches `bitcoin_hashes` to use the version in the repository and
fixes the code after removal of `Deref`.
This also turns off `AS_DEPENDENCY` check with the intention to refactor
it later.
02c1cd6291 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra)
Pull request description:
Updates the CHANGELOG and also the doccomment on `Transaction`.
ACKs for top commit:
tcharding:
ACK 02c1cd6291
Kixunil:
ACK 02c1cd6291
sanket1729:
ACK 02c1cd6291
Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd
This renames `Script` to `ScriptBuf` and adds unsized `Script` modeled
after `PathBuf`/`Path`. The change cleans up the API a bit, especially
all functions that previously accepted `&Script` now accept truly
borrowed version. Some functions that perviously accepted `&[u8]` can
now accept `&Script` because constructing it is no loger costly.
1b15a13e5a run cargo clippy and fmt (Andrew Poelstra)
f2a5596899 examples: clean up taproot PSBT example locktime handling (Andrew Poelstra)
821842e1a1 drop Ord on absolute::LockTime; add Ord to Transaction (Andrew Poelstra)
5b7d801ee6 remove PackedLockTime type (Andrew Poelstra)
4dee116b8a delete PackedLockTime by aliasing it to LockTime (Andrew Poelstra)
fa81568fb6 locktime: add `FromHexStr` impl for `LockTime` (Andrew Poelstra)
74ff4946e4 locktime: unify serde impls (Andrew Poelstra)
Pull request description:
This is potentially a controversial PR, but hear me out. My argument is that right now `absolute::LockTime` and `PackedLockTime` are basically identical types; they can be converted between each other with `From` and they have exactly the same semantics except that `PackedLockTime` has `Ord` on it. This makes it very confusing to tell which one should be used, for example in PSBT2 where there are now extra locktime-related fields.
The motivation for having `PackedLockTime` independent of `LockTime` are:
* `PackedLockTime` is theoretically more efficient because you don't need to unpack the field, don't need to store a enum discriminate, etc. I don't buy this. If you are trying to save individual bytes in your transaction-parsing there are lots of places you'd look before messing around with locktimes, so we shouldn't privilege this specific thing.
* `PackedLockTIme` has an `Ord` impl, and removing that will have a cascading effect on transactions, blocks, etc., preventing them from being held in `BTreeMaps` etc. **My proposal**, implemented here, is to just manually impl `Ord` on `Transaction` and don't impl it on `LockTime`.
I recall some argument that we need to be able to sort miniscripts, and miniscripts might have locktimes in them, but I think this is wrong -- miniscripts always have explicitly either a `Height` or a `Time`, and there is no problem ordering these.
Closes#1455
ACKs for top commit:
sanket1729:
code review ACK 1b15a13e5a
tcharding:
ACK 1b15a13e5a
Tree-SHA512: d9776f14560c3822980e8fff8978bf8ca753035152f957a84af25d063c66e4e9df3d5cf98af38d6cb59cc52ba407282f722925b1ca670ae623ac91add3149d2f
b7a84d0c68 hashes: Do not implement Deref (Tobin C. Harding)
Pull request description:
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement `internals::hex::display::DisplayHex` for slices hashes get coerced into slices and `to_lower_hex_string` can be called on them, this is incorrect because `DisplayHex` does not account for hashes that display backwards so we end up with the wrong string.
This is an API breaking change, and I have not built any other crates in our stack to check if anything breaks.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
ACKs for top commit:
apoelstra:
ACK b7a84d0c68
sanket1729:
ACK b7a84d0c68
Tree-SHA512: 573169095dd9f9032e3f685e3b0af3b569f1cb5368e1b2f37940504fd887592f46488abcfbe84107baf9c5453c7db47bc342a6bc273ff98cb0570ffacb549c21
This still has the line
let lock_time = absolute::LockTime::from_height(psbt.unsigned_tx.lock_time.to_consensus_u32() + lock_time_delta).unwrap();
I'm unsure whether this "adding height to a locktime" concept is a
meaningful thing or just the sort of thing that shows up in example
code. Maybe we should have first-class support for it.
Note that the line, as written, depends on the fact that the original
locktime was a small blockheight. A proper function for this would
handle the exceptional case gracefully.
Currently we implement `Deref` for hashes. From the docs [0]
> Deref should only be implemented for smart pointers to avoid confusion
Furthermore because we implement `Deref` as well as implement
`internals::hex::display::DisplayHex` for slices hashes get coerced into
slices and `to_lower_hex_string` can be called on them, this is
incorrect because `DisplayHex` does not account for hashes that display
backwards so we end up with the wrong string.
[0] https://doc.rust-lang.org/std/ops/trait.Deref.html
This can be replicated by deleting the `type PackedLockTime = LockTime'
line, and then running
find . -type f | xargs sed -i 's/PackedLockTime/LockTime/g
at the root of the repo.
e00dfa9806 impl FromHexStr for structs with single u32 member (connormullett)
Pull request description:
Closes: #1112
- Adds new trait `FromStrHex` with 2 methods: `from_hex_str` and `from_hex_str_no_prefix`
- Impl new trait on each tuple struct with single u32 member. eg `Time(u32)`
As stated in the issue, grep through codebase with `\(u32\)` and `\(pub u32\)` to see all implementations and verify none were missed.
NonStandardSighashType is an error type and should never be constructed from a hex string. Therefore, it has been omitted from this change.
Tests are somewhat redundant, but cover 4 cases each. 2 happy paths, 1 for each function. 1 case for malformed/invalid hex input, and 1 for calling no_prefix without a prefix
ACKs for top commit:
Kixunil:
ACK e00dfa9806
apoelstra:
ACK e00dfa9806
Tree-SHA512: 221faef7fc1fa8fdb4cba79cfae317a0b63984937c345c6ca2287123a078f38911cdc07db7589a88b7bc6fbecf389e9bcff47952728410510ffcfc1857e0f91f
Adds new module `string` to be later converted to its own
crate. The module currently contains the FromHexStr trait and an error
type to be used for implementing hex parsing on types. This change
also adds implementations of FromHexStr for types with a single u32
member such as `Sequence(pub u32)`. All structs that match the
following regex have been given this implementation
`\(u32\)` and `\(pub u32\)`. All implementations have associated
unit tests matching all possible cases. NonStandardSighashType has
been ommitted from this change as it is an error and should not be
constructed using the methods added in this change.
Adds parse::hex_u32 for future use to be made generic to allow
different sizes of integers to be parsed from hex strings.
The error type FromHexError implements required traits such as
Display and std::error::Error
d7006ef80d Adds roundtrip tests for Network::from_core_arg (Sergi Delgado Segura)
bd1eb29f61 Adds Network::to_core_arg (Sergi Delgado Segura)
Pull request description:
Comming from https://github.com/rust-bitcoin/rust-bitcoincore-rpc/pull/247
`Network::from_str` only considers `rust-bitcoin` string as possible inputs to create a `Network` instance. This PR adds a new method to `Network`, `from_core_arg`, which is complementary to the existing `Network::to_core_arg`. This method allows the conversion between `bitcoind -network` string and `Network` variants.
This also links `Network::from_str` to `Network::from_core_arg` so the default case on the former calls the latter, and an error is only returned if none of the cases match.
ACKs for top commit:
Kixunil:
ACK d7006ef80d
apoelstra:
ACK d7006ef80d
Tree-SHA512: 97a66f858a7d4a3642bdef9016457833cfc1181e276f7ead7c6b87f6fcdcb7c5d1cfdb4b621225b806bc5949c3c5cc6a32b7df934157542d7c79aa00a9e20f41
c822fcf435 Remove helper variables (Martin Habovstiak)
70d1a0348e Use serde derive rather than manual parsing (Martin Habovstiak)
Pull request description:
Manual parsing Json is tedious and error-prone. It contained a bunch of
`unwrap`s and was hard to read.
This replaces manual Json parsing with serde_derive implementation.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/1231
ACKs for top commit:
apoelstra:
ACK c822fcf435
tcharding:
ACK c822fcf435
Tree-SHA512: 0e1df6a62dc8438d67979a00658f8a2820135beebdd01d7275149b06feacd0d18accb86cecbf8c85f9a02ddc724575eaffff079b45cfe0e7765c8559c5eb03f7
Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.
Improve the content of various rustdoc comments in the `blockdata`
module.
Suggested content came from reviewers, all mistakes are my own :)
Reduce the number of lines of code by using a longer column width, 100
as is more-or-less standard in this repo.
This patch only changes column width (line length), no other changes.
Manual parsing Json is tedious and error-prone. It contained a bunch of
`unwrap`s and was hard to read.
This replaces manual Json parsing with serde_derive implementation.
Closes#1231
1a2cf2681d Implement consensus encoding adapter for serde (Martin Habovstiak)
a6ecc58a5e Add `put_bytes_min` and `space_remaining` methods (Martin Habovstiak)
Pull request description:
In some protocols it is preferred to serialize consensus-encodable types
using consensus encoding. E.g. serialize `Transaction` as hex-encoded
string in Json in Bitcoin Core RPC protocol. This change provides
adapter to make this easier.
The adapter allows providing custom byte-to-string encoder for more
exotic cases and provides a hex implementation which should be useful in
majority of the cases.
Should help with #765
Based on #1252
Required by #1234
ACKs for top commit:
tcharding:
ACK 1a2cf2681d
apoelstra:
ACK 1a2cf2681d
Tree-SHA512: 96e10cf6ea0e7dfecfb58ee97453e0e7c8a2cfbb8af1e73a23c3afb67b985b394976361ac237528991fbb7344cc9f24644869199008245a91838309aff34bb97
df90c50242 Add log2 to Work (Jiri Jakes)
Pull request description:
Adds method `Work::log2()` providing value equivalent to Bitcoin Core's `log2_work` in its logs. Fixes#1326.
Questions:
- The original issue (#1326) also suggests to add log2 to Target but it does not seem to be meaningful, does it? Bitcoin Core, to which the issue refers, also displays only log2 of work.
- Although work should not be 0, the type allows it. In this case, log2 would return -inf. I think we could leave it like that but if there are suggestions to deal with it in a different way, please let me know.
ACKs for top commit:
Kixunil:
ACK df90c50242
tcharding:
ACK df90c50242
apoelstra:
ACK df90c50242
Tree-SHA512: 24e83c849ea3e16cad6f1636920883967cdac90b1b98ab19e86d1a5d0ac4585079740b54c1315afecabc1943d29a8b94316e3586bd2d10f1b61cc5cf7ad5b273
e9dffb1b7b Change `max_money` to a constant. (Martin Habovstiak)
Pull request description:
The value is statically known which is better expressed as a constant. Also allows usage in const context.
ACKs for top commit:
apoelstra:
ACK e9dffb1b7b
tcharding:
ACK e9dffb1b7b
ariard:
ACK e9dffb1
sanket1729:
ACK e9dffb1b7b
Tree-SHA512: b9b80d573531fe75dce22e185a1c84b2885160334418d1cfbd7279684fd4229c3c6c4041d3a3badb3652c5723e90ff52d3c761cbc3bff7b73978776694a67422
13d94cbc47 Remove no_run (Tobin C. Harding)
Pull request description:
`no_run` is not needed since we already mark this up as `bash` which rustc doesn't run when running examples.
While the keyword `bash` is not currently supported it may well be in the future and since only the `rust` keyword causes code to run any other string is effectively a wildcard, `no_run` is therefore meaningful only as a convention. Lets keep `bash` in case support is added later on.
cc sr-gi
ACKs for top commit:
sr-gi:
ACK 13d94cbc47
Kixunil:
ACK 13d94cbc47
sanket1729:
ACK 13d94cbc47
apoelstra:
ACK 13d94cbc47
Tree-SHA512: 90cbe8c5ea30577fc148d8e8b388008ea7f6c4975ea8b9e249e6058de901afca084d59e6a89eb9bda0a8e3112fb89336b622acd23046299c5e5ad7723ae26148
In some protocols it is preferred to serialize consensus-encodable types
using consensus encoding. E.g. serialize `Transaction` as hex-encoded
string in Json in Bitcoin Core RPC protocol. This change provides
adapter to make this easier.
The adapter allows providing custom byte-to-string encoder for more
exotic cases and provides a hex implementation which should be useful in
majority of the cases.
Should help with #765
My local scripts did not test serde feature on merge commit. While
merging 734, I accidently broke the serde feature on latest master. This
PR fixes it.
962abcc963 Add serde regression tests (Tobin Harding)
Pull request description:
Attempts to add regression tests for _all_ types defined in `rust-bitcoin` that implement `Serialize`/`Deserialize`.
- Add a `tests` directory and implement regression tests in there
- Use files for input hex and output bincode to reduce source file clutter
- Copy test block and `include_bytes!` usage from RCasatta's [PR](https://github.com/rust-bitcoin/rust-bitcoin/pull/750)
- Uses Kixunil's macro suggested below
- Adds a single regression test to `util/taproot.rs` for private types
## Note to reviewers
- Uses JSON for opcodes in a separate file (`tests/regression_opcodes.rs`), for all other tests uses bincode.
- Bypasses the order issue for maps by only serializing maps with a single element - is this correct?
Fixes#723
ACKs for top commit:
apoelstra:
ACK 962abcc963
sanket1729:
ACK 962abcc963. This has been open for a long time. Merging this in the interest of progress.
Tree-SHA512: e34e48e1c56fab5898bc74e7fb867435ed387d828dd3daf0c7d6df8f305e1da6883e91487115ac428618eb7d95bd16aa2cd209ca219684959bc95587ef0b4083
4c8570b512 base58: Run formatter (Tobin C. Harding)
2780e6cdaa Move base58 module to crate root (Tobin C. Harding)
Pull request description:
Move `base58` module to the carte root, direct `rustfmt` to not format the digits array. Run formatter as a separate patch.
ACKs for top commit:
apoelstra:
ACK 4c8570b512
sanket1729:
ACK 4c8570b512
Tree-SHA512: 3ab7afe089b9866a59f3f67c74c3e6657ca51d2d0ea1aaee0218a400b90826d36eaca7d491006a9448359ae3de5755d20b2d2df71e0fa3d6497fbaca270dcf42
`no_run` is not needed since we already mark this up as `bash` which
rustc doesn't run when running examples.
While the keyword `bash` is not currently supported it may well be in
the future and since only the `rust` keyword causes code to run any
other string is effectively a wildcard, `no_run` is therefore meaningful
only as a convention. Lets keep `bash` in case support is added later on.
In preparation for removing the `util` module move the `base58` module
to the crate root. This is likely not the final resting place for this
module but it is a step in the right direction.
Includes addition of rustfmt attribute to skip formatting the digits
array. No other changes to the `base58` module.
For some applications, such as block explorers, it's useful to be able
to obtain the public key used in case of P2PK. This is considered
general enough for addition into bitcoin library,
so this change adds it.
The public key is returned only when it's valid and the script is P2PK.
To avoid duplicating the logic checking whether a script is P2PK the
logic is moved to a new p2pk_pubkey_bytes() method which returns raw
bytes and is then called from both is_p2pk() and p2pk_public_key()
We are trying to flatten the `util` module. The `taproot` module can
live in the crate root. If/when we create a `crypto` module/crate we may
wish to pull some stuff out of this module but for now moving it gets us
closer to removing `util` without making the directory structure any
worse.
Includes adding rustfmt attributes to skip formatting of macros.
2df51dae15 Create crypto module (Tobin C. Harding)
Pull request description:
Done as part of [util flattening](https://github.com/rust-bitcoin/rust-bitcoin/issues/639).
Create a `crypto` module and move into it (out of `util`):
- ecdsa
- schnorr
- key
After review, this PR now includes some type re-names
- EcdsaSig -> ecdsa::Signature
- SchnorrSig -> schnorr::Signature
- EcdsaSigError -> ecdsa::Error
- SchnorrSigError -> schnorr::Error
- InvalidSchnorrSigSize -> InvalidSignatureSize (this is an error enum variant)
ACKs for top commit:
apoelstra:
ACK 2df51dae15
sanket1729:
ACK 2df51dae15
Tree-SHA512: 7cf63d51ed5fdc737cd59767d9bb96b1e3501634e3aee855493f6a51ad5c5397ce4b25c77f9929abd70d6ceb351fc6520e835da108f4c9a46df5b9c2b52ca6b3
f7a6d17143 Add rand feature flag to the example documentation (yancy)
Pull request description:
It's confusing trying to follow the documentation [here](https://docs.rs/bitcoin/latest/bitcoin/util/address/index.html) unless you know to enable the `rand` feature flag. This PR updates the docs so people know to enable the flag.
```
22:35 < control> hello, how can i generate a simple wallet address using rust bitcoin? cant find working example
22:45 < andytoshi> control: do you have a private key?
22:45 < andytoshi> what kind of address do you want to generate?
22:46 < andytoshi> my guess would be that you want to use bdk rather than rust-bitcoin directl
22:48 < control> P2PKH address and generate private key. just as simple as bit library in python does
22:52 < control> im trying to run this example
22:52 < control> https://docs.rs/bitcoin/latest/bitcoin/util/address/index.html
22:52 < control> but it says ^^^^ could not find `rand` in `secp256k1`
```
ACKs for top commit:
apoelstra:
ACK f7a6d17143
Kixunil:
ACK f7a6d17143
Tree-SHA512: 6d94f8ffa7797d1e7720a840d4f8bb0ac274507118597dff60631ad3d28dbe57e2341b87d6101e2a9f5fbcc2b1f0beb6d06f9dde48a480cb10a9a45c887f83a4
Kani can't really handle string processing, and appears to be unable
to check integer multiplication (for now), but we do several checks
for addition and subtraction, and conversion between signedness,
that Kani can easily prove.
Done as part of flattening util.
Currently in `util` module we have a bunch of modules that provide
cryptography related functionality.
Create a `crypto` module and move into it the following:
- ecdsa
- schnorr
- key
To improve uniformity and ergonomics, do the following re-names while we
are at it:
- EcdsaSig -> ecdsa::Signature
- SchnorrSig -> schnorr::Signature
- EcdsaSigError -> ecdsa::Error
- SchnorrSigError -> schnorr::Error
- InvalidSchnorrSigSize -> InvalidSignatureSize (this is an error enum variant)
519db4d951 add Network::to_core_arg() method (connormullett)
Pull request description:
closes: #1207
Adds converting `Network` to its `bitcoind` equivalent.
The arguments for -chain can be found in the documentation and is one of the following:
main, test, signet, regtest
ACKs for top commit:
apoelstra:
ACK 519db4d951
Kixunil:
ACK 519db4d951
Tree-SHA512: 5fd805d654f7c30f87ff877fe90e19490d0deb73b46ce87cc6b43d30595eb9d2de3f646f58a5d72180c3e8cc6a9b614bfe6753ecd6c21b8d193a8d862e3f887f
108a1f73ca Fail CI if docs build throws warnings (Tobin C. Harding)
b014f0fdcb Fix rustdocs build warnings (Tobin C. Harding)
Pull request description:
Currently we do not fail the CI script if the docs build throws warnings, since we are a group of super anal, easily triggered, code cleanliness obsessed devs this causes a mild rash to develop on the lower back [0]. We can easily fix this by checking for build warnings in CI.
[0] - Amusingly my rash has been playing up since Friday but I thought I'd fixed the warnings in an open PR someplace so I was ignoring it, seeing Kixunil's [issue](https://github.com/rust-bitcoin/rust-bitcoin/issues/1403) this morning prompted me to fix it :)
Fix#1403
ACKs for top commit:
Kixunil:
ACK 108a1f73ca
apoelstra:
ACK 108a1f73ca
Tree-SHA512: 0f86c318b2ec8bf7aa6a0d0f355f8fe8e3eb8ad5eb74d95f8dab882d6729c386c3e0ef4cc2378645e15460ff2b9b47d66e3603958f8b188f5e2b07272739d755
Convert all rustdocs build warnings to errors using `-D warnings` and
exit the script with 1 to signal the error.
While we are at it use `--all-features` instead of explicit feature list
when building docs. Without this the docs build fails with:
error: too many file operands
64495cc5fe Drop Network arg from max_money() (Antoine Riard)
Pull request description:
Amount of coins available stay in the same across Bitcoin network: signet, testnet, mainet. From my understanding this is a leftover from some potential multi-chain support.
For more context: https://github.com/lightningdevkit/rust-lightning/pull/1839#discussion_r1019753069
If there is already an existent PR, it can be closed, however didn't find one.
ACKs for top commit:
apoelstra:
ACK 64495cc5fe
tcharding:
ACK 64495cc5fe
Tree-SHA512: 929011ee73c5eda903fb0140438ed5e88c8f5b7378036a87a6a660a8b9138bf204bf59a0ba822c0cd98e37e97d2d0dbbf8c9893a834da9acdf817ba43a5ed5b6
e04a7a926d network: Run cargo fmt (Tobin C. Harding)
dc33c7999f Enable formatting of the network module (Tobin C. Harding)
81f69e846b Exclude from formatting stuff in the network module (Tobin C. Harding)
408d7737fb Run cargo fmt (Tobin C. Harding)
1ecf09359b Add local variable to reduce line length (Tobin C. Harding)
b2e74bc050 Exclude long function call (Tobin C. Harding)
ff5a80dbd3 Exclude formatting of function fmt_satoshi_in (Tobin C. Harding)
308a12b7cf Exclude array from formatting (Tobin C. Harding)
fb7ff46ccc Improve crate root re-exports (Tobin C. Harding)
Pull request description:
The `network` module is not currently included in formatting. Also, at this point in time not much is happening in the `network` module so formatting it should not cause too many merge conflicts with other in-progress PRs.
The first 3 patches are formatting preparation of the repo, the next 2 are formatting preparation of the `network` module. The last patch is the result of running `cargo +nightly fmt`. Can one reviewer please verify the last patch consists only of `rustfmt` changes by running the command on their branch. Thanks
cc luckysori :)
ACKs for top commit:
apoelstra:
ACK e04a7a926d
Tree-SHA512: 49b2873f0bfdd448df97073b462fa860c85b7f3c3094fdb0e16fd86aa467156ea8b2ceec61e2ee99848802cd171fd8abbc17e45c66672889a37c413fa4bea636
9674bf29fe hashes: Fix clippy warnings (Tobin C. Harding)
b9643bf3e9 Import bitcoin_hashes crate into hashes (Tobin C. Harding)
580feab3f9 internals: Add CHANGELOG file (Tobin C. Harding)
bae64e156e Move CHANGELOG to bitcoin crate (Tobin C. Harding)
9a2c856be6 Update gitignore file (Tobin C. Harding)
Pull request description:
#1337 was split out of this in an attempt to help review, I think we can merge this one though now it has some traction.
We would like to bring the `bitcoin_hashes` crate into the `rust-bitcoin` repository. https://github.com/rust-bitcoin/rust-bitcoin/issues/550#issuecomment-1248071843
Import `bitcoin_hashes` crate into `hashes`.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 2a78c250f78d391599040223870a4d1d6f6f5482
Please note the commit history of `bitcoin_hashes` is only preserved on the soon-to-be-archived `bitcoin_hashes` repository.
ACKs for top commit:
sanket1729:
ACK 9674bf29fe. Did my best to review the CI code, it looks good, and seems like we are testing everything when we add "hashes" to the root level `./contrib/tests.sh`. I would like other reviewers to confirm the same.
apoelstra:
ACK 9674bf29fe
Tree-SHA512: db3ce6adeb38430c5a9f372da16be9fb048c2e5cff646b701139fb4b5fa76e10261432284ede7fd139b75cd69bb124d55973ceb7eccaa996226a7be4cad9b68a
Amount of coins available stay in the same across Bitcoin network:
signet, testnet, mainet. From my understanding this is a leftover
from some potential multi-chain support.
We just removed the `bitcoin/src/network/` exclude from `rustfmt` config
file. Run the command `cargo +nightly fmt`.
No other changes than those introduced by `rustfmt`.
When we moved to edition 2018 the use of `extern` became unnecessary and
we moved to using `pub use` for re-exports. It was observed however that
`pub extern crate` is more readable.
Improve crate root re-exports by doing:
- Use `pub extern crate foo` to re-export foo.
- Fix docs attribute for optional dependency `bitcoinconsensus`.
- Re-order to how rustfmt would put them.
30888f74c5 Move psbt module to crate root module (Tobin C. Harding)
8a75ff450f Move read_to_end out of util module (Tobin C. Harding)
445b07c94c Move util::Error to error module (Tobin C. Harding)
Pull request description:
In an effort to flatten `util` move things out that can/should be put in submodules of the crate root module. For each, configure `rustfmt` to ignore the module. This pushes the `rustfmt` review nightmare down the road.
ACKs for top commit:
apoelstra:
ACK 30888f74c5
Kixunil:
ACK 30888f74c5
Tree-SHA512: 0d93d60bec822d1dc82d4d67c25854364b0863488e4b35c9a0828a843fc3792286c18abde40a8e9d6ec535cfc7f0f0d6495d35961ce43af3f2605c92aaa0815d
f55c4099d5 Format the merkle_tree module (Tobin C. Harding)
Pull request description:
Run `cargo +nightly fmt` and commit the changes to the `merkle_tree` module. No manual changes, only those introduced by `rustfmt`.
ACKs for top commit:
tcharding:
ACK f55c4099d5
apoelstra:
ACK f55c4099d5
Tree-SHA512: 2f47fc426988b4755408fea5f8c4c47ebe23b53850737a1640fa9477cfd83f0ada046aec734986a92a9effbaaee0f34764ef87d82deff1a594f7b73bf2e93e93
We now have an `error` module but the `util::Error`, which is a general
error, is not in it.
Make `Error` more ergonomic to use by doing:
- Move the `util::Error` to `crate::error::Error`
- Re-export it from the crate root since it is our most general error
- Re-export and deprecated it from `util`
613107298d Move merkleblock into merkle_tree (Tobin C. Harding)
c89d9c48ac Move merkle_tree.rs to merkle_tree/mod.rs (Tobin C. Harding)
Pull request description:
Re-done after review comments below. This is now PR 1 in the `merkle_tree::block` series :)
Move the `merkleblock` module into the `merkle_tree` module in a submodule called `block`. In order to do the minimum amount of changes in this patch DO NOT rename types to improved naming and reduce stutter.
Note:
- block module is private
- the three types are re-exported from `merkle_block`
- the `MerkleBlock` re-export from the crate root is left in place.
This patch purposefully does the minimum amount of changes because there a whole bunch of improvements to the old "merkleblock" module that are coming next in a separate PR.
ACKs for top commit:
Kixunil:
ACK 613107298d
apoelstra:
ACK 613107298d
Tree-SHA512: 7299f605a0408372301642ac6826f7532de187b43a6d934715fc0806379b04cfd1550610428b720cb89095659c25e0f4fc8d6c842a93eafc19c091bbfcd5f35e
Move the `merkleblock` module into the `merkle_tree` module in a
submodule called `block`. In order to do the minimum amount of changes
in this patch DO NOT rename types to improved naming and reduce stutter.
Note:
- block module is private
- the three types are re-exported from `merkle_block`
This patch purposefully does the minimum amount of changes because there
a whole bunch of improvements to the old "merkleblock" module that are
coming next.
In preparation for moving `MerkleBlock` into the `merkle_tree` module;
create a new directory for the module and move `merkle_tree.rs` to
`merkle_tree/mod.rs`.
d78a996bf6 Add `Witness::from_slice()` and depreciate `Witness:from_vec()` (Noah Lanson)
d5bdf5d225 Add non-generic `Witness::push_slice()` method (Noah Lanson)
Pull request description:
Cleanup PR to improve the `Witness` API by:
- Adding `Witness::from_slice()` and depreciating `Witness::from_vec()` methods (#1371).
- Making `Witness::push()` not generic and take in `&[u8]` instead of `AsRef<[u8]>` (#1372).
Note: `Witness::from_vec()` has been marked for depreciation from `0.30.0`. Let me know if this should be different.
ACKs for top commit:
tcharding:
ACK d78a996bf6
apoelstra:
ACK d78a996bf6
Tree-SHA512: 3a0b11b1ea77966a773cf7c9e9853822192897eac495fc0a23068bad3b0c46714fc839b20ceeb6e076aa10ea8ff0c023dfc418feff2f892cf11e8c057e5b0c7d
4e9ff972ad Improve checksum documentation (Tobin C. Harding)
0f01cb9f51 Use rustdoc summary (Tobin C. Harding)
6151d4c841 base58: Rename public functions (Tobin C. Harding)
a94af5c052 base58: Re-order code (Tobin C. Harding)
d362e6286a base58: Improve rustdocs (Tobin C. Harding)
a43234e7ab base58: Make SmallVec methods private (Tobin C. Harding)
27f2cba623 base58: Use alternate form to print hex (Tobin C. Harding)
f659a7aca3 base58: Remove key related errors (Tobin C. Harding)
Pull request description:
Do some clean up work to the `base58` module in preparation for splitting it out into its own crate.
- Patches 1-6: Basic clean up.
- Patch 7: Re-names the public API functions.
- Patch 8: Fixes rustdoc comment as suggested during review.
- Patch 9: Improves documentation on checksum, also as suggested during review.
ACKs for top commit:
apoelstra:
ACK 4e9ff972ad
sanket1729:
ACK 4e9ff972ad. Left some naming nits.
Tree-SHA512: 0fb1e5a964bd197fcb3ef5e9ecd6f8c6b35439af46528e8dbe654d9d10f7c8ed3ca1461593caf6efd0be1cd3a1c24fed1a176931114846a394b396bed6a2411d
49d7b0bfe1 Remove deprecated re-exports (Tobin C. Harding)
Pull request description:
Recently we added a bunch of deprecated re-exports while moving things out of the util module. Turns out while the code reads like it works, `deprecated` actually only works for functions, not types or modules etc.
Remove the non-functional deprecated lines and elect to _not_ re-export things we moved. Release 0.30 is going to break a lot of code but there is no real nice way to resolve that. We will need good release notes and a public apology probably :)
Fix import statements that still rely on `util::bip32` - these should have been fixed when we moved the `bip32` module.
ACKs for top commit:
Kixunil:
ACK 49d7b0bfe1
apoelstra:
ACK 49d7b0bfe1
Tree-SHA512: 2b6a6d2d001f6124585f692315c48654b4fd0f5047b9fcef92b25829a27c8a02b3d187c8d363e9304b3998b652ead1ad368a7bf68ea23c984d1973074df2af21
00c7b6e06f Witness: Fix nits from PR 1323 (junderw)
Pull request description:
Ref: #1323
This is just to quickly fix some of the smaller nits. Larger changes (deprecations, adding / refactoring of methods) should be in a separate PR.
ACKs for top commit:
Kixunil:
ACK 00c7b6e06f
tcharding:
ACK 00c7b6e06f
sanket1729:
ACK 00c7b6e06f
Tree-SHA512: 5f661187a7003060669d15d873e323c017c905a00b62eb56ca3afc2fc27084b245ad62dfcf6d2fd14eac361430be954e7636f6b9ff668aefaad0424789a2f826
Recently we added a bunch of deprecated re-exports while moving things
out of the util module. Turns out while the code reads like it works,
`deprecated` actually only works for functions, not types or modules
etc.
Remove the non-functional deprecated lines and elect to _not_ re-export
things we moved. Release 0.30 is going to break a lot of code but there
is no real nice way to resolve that. We will need good release notes and
a public apology probably :)
Fix import statements that still rely on `util::bip32` - these should
have been fixed when we moved the `bip32` module.
We would like to bring the `bitcoin_hashes` crate into the
`rust-bitcoin` repository.
Import `bitcoin_hashes` into `rust-bitocin/hashes`, doing so looses all
the commit history from the original crate but if we archive the
original repository then the history will be preserved. We maintain the
same version number obviously and in the changelog we note the change of
repository.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 54c16249e06cc6b7870c7fc07d90f489d82647c7
Includes making `embedded` and `fuzzing` per-crate i.e., move them into
`bitcoin` as hashes includes these also.
NOTE: Does _not_ enable fuzzing for `hashes` in CI.
Notes on CI:
Attempts to merge in the github actions from the hashes crate however reduces
coverage by not running hashes tests for beta toolchain. Some additional
work could be done to improve the CI to increase efficiency without
reducing coverage. Leaving for another day.
3c0d5aed73 Add get_tapscript to Witness (junderw)
4226d60205 Add Index<usize> and nth(index) to Witness (junderw)
Pull request description:
Ref: https://github.com/rust-bitcoin/rust-bitcoin/pull/672#issuecomment-980636502
[Add Index<usize> and nth(index) to Witness](4226d60205)
[4226d60](4226d60205)
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
---
[Add get_tapscript to Witness](a7501d9599)
[a7501d9](a7501d9599)
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
---
Edit: This is the previous PR body:
> In a taproot script payment with annex, quick access to the 3rd to last element (which is the actual script in this case) is convenient.
>
> This feels like kicking the can down the road again, but I think it's a nice to have method.
>
> RCasatta dr-orlovsky were discussing this issue. I would like to ask if they have any thoughts on the addition of this.
ACKs for top commit:
tcharding:
ACK 3c0d5aed73
apoelstra:
ACK 3c0d5aed73
Kixunil:
ACK 3c0d5aed73
Tree-SHA512: 0038eed6ad56786b8dd6d98db0d1846753b8b25de0bc1089cdc75d5850d0ccc66dde9a10be7fe09589ad7db118fd50ee9f7993695968df5c389457ccfcdaa761
Recently we added a workspace but left the CHANGELOG file at the
repository root, this is incorrect because the CHANGELOG is a per crate
thing since it is updated along with crate release.
248f9a3b4b Use capital letters for Bitcoin Core (Tobin C. Harding)
832169eb8d Add to/from_consensus methods to Version type (Tobin C. Harding)
24984f095f Make block::Version inner value private (Tobin C. Harding)
7e146ede96 Make types in block module more terse (Tobin C. Harding)
Pull request description:
After initial attempt and review this PR has been re-written.
- Patch 1: Make types in `block` more terse, this is preparatory clean up based on suggestion below.
- Patch 2: Make inner value of `Version` private to hide the i32/u32 discrepancy
This is a follow up to #1240
ACKs for top commit:
Kixunil:
ACK 248f9a3b4b
apoelstra:
ACK 248f9a3b4b
Tree-SHA512: ee031035288a2bcc246a9837a6028c254c51daf78a5cc2441b467ab7f183f1700a63911a2e78b84a20674ce0a83851a7c3bb7e46644a56fdd255685b9a0bf7f2
2157e69857 Document the `all` module (Tobin C. Harding)
Pull request description:
Improve documentation on the `all` module by doing:
- Document guarantee that `all` will only ever contain opcode constants
- Fix stale/incorrect code comment
Done as follow up to #1295
ACKs for top commit:
apoelstra:
ACK 2157e69857
Kixunil:
ACK 2157e69857
Tree-SHA512: 4df091bbdce7b9ba73caabd74b80f9e8c0a30fa2f9a20ed9b75542e71a204e5cd82698a74bebbd6f0beab55ecd807154d1b7d27a787cc9dede7abbd20a0a4ad5
The `Version` type uses a signed 32 bit integer inner type but we bit
twiddle as if it was a `u32`. We recently made the inner type private to
hide the data type because of this oddness.
Add methods `from_consensus` and `to_consensus` to facilitate any
possible thing users may want to do with a consensus version value.
The Bitcoin block version is a signed integer for historical reasons,
but we bit twiddle it like an unsigned integer and during consensus
encode/decode we cast the signed value to an unsigned value.
In order to hide this confusion, make the inner value private and add a
couple of constants for v1 and v2 block versions.
Currently the types in the block module have longer names than
necessary, "header" and "version" identifiers contain the word "block",
this is unnecessary because we can write `block::Header` instead of
`BlockHeader` when context is required. This allows us to use the naked
type `Header` inside the `block` module with no loss of clarity.
We are stuck with `BlockHash` because the type is defined along with all
the other hash types in `hash_types`, leave it as is for now but
re-export it from the `block` module to assist in putting types that are
used together in scope in the same place, making import statements more
ergonomic.
The `all` module enables usage of a wildcard import statement without
muddying the scope with any other types defined in `opcodes`, in other
words if one wants to use the `All` type `opcodes::All` is the most
clear way to use it, however usage of naked `OP_FOO` types is perfectly
clear.
Add documentation stating that we guarantee to never put anything else
in the `all` module so folks are confident using a wildcard import will
not bring any rubbish into scope.
Expected usage in downstream applications that need types in `opcodes`
as well as the opcodes:
```
use bitcoin::opcodes::all::*;
use bitcoin::opcodes;
```
Also, we do no implement `Ord` or `PartialOrd`, document this including
HTML tags hiding an example bug from Bitcoin Core that shows why not.
9a1623c1dc Re-export hashbrown when enabled (Tobin C. Harding)
Pull request description:
`hashbrown` used to be exported until commit 23ee0930c7 which removed the `pub extern crate` declaration.
Found thanks to afilini (#1342)!
ACKs for top commit:
apoelstra:
ACK 9a1623c1dc
Kixunil:
ACK 9a1623c1dc
Tree-SHA512: 0363781dc06211eec59246ce54582220674d5ab2bd6e62ad15eeb97b0df6435cdf23df8306131c176b6003dde31d1e376f52981f2e69e9d2529876d3ada727e0
b6f9e47dba Fix `no_std` when `bitcoinconsensus` is enabled (Martin Habovstiak)
Pull request description:
`default-features = false` was missing previously but blindly adding it would lead to subtle risk of breaking when a crate not needing `std` depends on `bitcoinconsensus` and simultaneously another crate not needing `bitcoinconsensus` depends on `std` and another crate depends on them both.
This change fixes it by introducing `bitcoinconsensus-std` feature flag and provides a fallback if the flag is off. Unfortunately the fallback has to use a bit of reasonable `unsafe` due to limitations of upcasting.
The only safe alternatives are not do it and provide worse experience for crates that are affected by the problem above or break the API, which couldn't be backported and would be more annoying to use.
Closes#1343
This is considered PoC PR as I realized the possibility of the hack (and necessity of `unsafe`) at the last moment. Things like tests and modifying CONTRIBUTING to change the stance on `unsafe` will be added if `unsafe` is ACKed.
ACKs for top commit:
tcharding:
tACK b6f9e47dba
apoelstra:
ACK b6f9e47dba
Tree-SHA512: 3a2845f4701c94ff6214749fa490aecf3fd96089df31b15f9d3e0afe3c74329ff2b9054d51244358a79f928aa9d4cf4001fc3ec40a9b0e189323544c4480c709
b84e1d46c0 Move amount module out of util (Tobin C. Harding)
Pull request description:
Done as part of flattening the `util` module. Simply move the `amount` module out of the `util` module and to the crate root. Justified by the fact that the `Amount` type is more-or-less a "primitive" bitcoin type.
ACKs for top commit:
apoelstra:
ACK b84e1d46c0
Kixunil:
ACK b84e1d46c0
sanket1729:
ACK b84e1d46c0
Tree-SHA512: 9ec707f49b7ab29f573be22b366d2ea9c1a8e4b27e80350d521b9c6607fca4142f079648cb739ba8590edd97c21a00029c3647c4c8cebe47cc2dfee1b10b8b39
29df410ea3 Document state after call to calculate_root_inline (Tobin C. Harding)
2dbc7fdf21 Rename merkle_root functions (Tobin C. Harding)
22dd904735 Rename util::hash module (Tobin C. Harding)
Pull request description:
Done as part of flattening `util`.
The `util::hash` module only provides two functions, both to calculate the merkle root of a list of hashes.
1. Rename `util::hash` -> `crate::merkle_root`
2. Change function names to `calculate[_inline]` so usage becomes `merkle_root::calculate`
Done as two separate patches so we can bikeshed the names, can squash if needed.
ACKs for top commit:
Kixunil:
ACK 29df410ea3
apoelstra:
ACK 29df410ea3
Tree-SHA512: 17ace90c7700b5d7adf8b95731c9a348b5c92863806cc88bc40730547f457e44160efb19985e025970b59fea86d68f0bf4be0af17717a65ae44f11c8d10ec4c6
1050fe9cae Remove unnecessary borrow (Tobin C. Harding)
3966709336 Use is_none() (Tobin C. Harding)
d192052519 Remove unnecessary dereference (Tobin C. Harding)
624cda07b3 Remove unnecessary casts (Tobin C. Harding)
Pull request description:
Clippy has been updated and new warnings are being triggered in our codebase. This PR does all warnings using nightly since they all looked like reasonable things to fix.
Needed for CI to pass in other open PRs.
ACKs for top commit:
Kixunil:
ACK 1050fe9cae
sanket1729:
ACK 1050fe9cae.
Tree-SHA512: 7dcfb6a72a0aae51b49b417bb94cbe1becb1095d1bf0011921b1834a10f792cfcdeee37993ab9b103bd2dfcc9cd3c26cd7f1bb80b06b0d1aa4aaa454bfb0b3f0
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
This transaction broke past versions of `rust-bitcoin` and LND so this
adds a test to avoid reintroducing the problem in the future.
See also https://github.com/romanz/electrs/issues/783
In order that we can safely change/maintain de/serialization code we
need to have regression tests with hard coded serializations for each
type that implements serde.
It is enough to test a single serde data format, use JSON for `opcodes`
and bincode for other types.
Do regression testing in a newly added `tests` module.
The `base58` module is for encoding and decoding, it makes sense for the
public functions to be called `encode` and `decode`. We also have some
functions that operate on data with a checksum, for these it makes sense
to tack `check` onto the _end_ of the function name.
With this applied the public API is:
- decode
- decode_check
- encode
- encode_check
- encode_check_to_fmt
Tobin C. Harding
Martin Habovstiak
Steven Roose
Andrew Poelstra
startup-dreamer
yancy
TATHAGATA ROY
Riccardo Casatta
kshitjj
Wilmer Paulino
Peter Todd
The rustfmt Tyranny
Sergi Delgado Segura
Harshil Jani
junderw
Salvatore Ingala
Subhradeep Chakraborty
sanket1729
benthecarman
hashmap
Lorenzo Maturano
DanGould
Arturo Marquez
Jiri Jakes
bnabi
Casey Rodarmor
Byron Hambly
Daniela Brozzoni
connormullett
Antoine Riard
Noah Lanson
