c062e4ff80 Deprecate legacy sighash methods (Tobin C. Harding)
1fdf5f9b82 Move legacy sighash unit test data to test_data (Tobin C. Harding)
6fa0329930 Re-order import statements (Tobin C. Harding)
Pull request description:
When we introduced the `SighashCache` we put encoding and sighash code
for segwit (v0 and taproo) in the `sighash` module. We also added
methods for legacy inputs that wrapped calls to encode/sighash methods
in the `transaction` module. This is confusing for a couple of reasons
- When devs read `Transaction` there are two methods that apparently
enable encodeing tx data and calculating the sighash but there is no
indication that these methods are only for legacy inputs.
- Once devs work out that segwit inputs have to be handled by methods on
the `SighashCache` it is not obvious why the methods on `Transaction`
exist at all.
Move the legacy encode/sighash code over to the `sighash` module and
deprecate the old methods. (Includes moving unit tests.)
Patch 1 and 2 are preparation.
(I did this work because I got lost in `Transaction::signature_hash` when messing with `TxOut::Amount` and trying to understand the signing algorithm vs bip143.
sanket1729 is there some other reason that I'm missing why we left the legacy code in `transaction`?
ACKs for top commit:
sanket1729:
ACK c062e4ff80
apoelstra:
ACK c062e4ff80
Tree-SHA512: b94de928e64e652ec75e34af94d5d161f5c52a862b5b10904e980b8ae669b3b434f8639d5ddadad84a7bbdf36ac21bdd287398448e844b5bb531c3664e91fb02
167ee8e72c Improve docs on bip158 module (Tobin C. Harding)
5cfe9169f5 Refactor tests module imports (Tobin C. Harding)
2447d96b44 Use Gcs instead of GCS (Tobin C. Harding)
e9846ad579 Improve docs on filter_header (Tobin C. Harding)
e0fddce9e9 Refactor new_script_filter (Tobin C. Harding)
f6105a16a7 Improve module docs (Tobin C. Harding)
25d1472924 Move new to top of impl block (Tobin C. Harding)
08e55bc4f1 Remove unneeded newlines (Tobin C. Harding)
28853fd3cc Use generics instead of dynamic dispatch (Tobin C. Harding)
d79c6b8358 Remove unnecessary use of Cursor (Tobin C. Harding)
Pull request description:
In attempt to resolve#1147 clean up the `bip158` module.
I was unable to resolve the `"confusing method names read and write that look like those from std:i:o::{trait}"`
I find the `bip158` data types and abstractions kind a funky but was unable to come up with any improvements.
Open question: Are all the public data types really meant to be public? Perhaps we should have an issue to write an example module that uses the bip158 module?
ACKs for top commit:
sanket1729:
ACK 167ee8e72c
apoelstra:
ACK 167ee8e72c
Tree-SHA512: 7caa661432f02d90cf32c13b54a635647b871bb1564d1df67957b6422465880fcfca8f74d51d4b0255dc34306a56cd866366febabc9a27ecdc00a2d1e6a21d5a
When we introduced the `SighashCache` we put encoding and sighash code
for segwit (v0 and taproo) in the `sighash` module. We also added
methods for legacy inputs that wrapped calls to encode/sighash methods
in the `transaction` module. This is confusing for a couple of reasons
- When devs read `Transaction` there are two methods that apparently
enable encodeing tx data and calculating the sighash but there is no
indication that these methods are only for legacy inputs.
- Ocne devs work out that segwit inputs have to be handled by methods on
the `SighashCache` it is not obvious why the methods on `Transaction`
exist at all.
Move the legacy encode/sighash code over to the `sighash` module and
deprecate the old methods. (Includes moving unit tests.)
Refactor the `new_script_filter` by doing:
- Put it directly below the `new` constructor
- Improve docs
- Remove unneeded scope
- Correctly indent `where` keyword
Currently in the `bip158` module we do a bunch of dynamic dispatch using
the reader/writer objects. We can improve runtime performance, at the
cost of compile time, by using generics instead of dynamic dispatch.
The function `match_all` takes an iterator, it does not need to use the
`Seek` trait, we can just pass in the content as a slice, no need to
wrap it in a `Cursor`.
7a28a56ac4 Pin `serde` to 1.0.142 (Martin Habovstiak)
Pull request description:
1.0.143 bumps MSRV higher than what we support.
Closes#1175
Unfortunately I have to go now, will try to get back ASAP.
ACKs for top commit:
apoelstra:
ACK 7a28a56ac4
tcharding:
ACK 7a28a56ac4
Tree-SHA512: 09546984d3d68c24068cb357e4a57db223cea6b48e32471fab551998d8afe46eeaac2e6a49ec3d8bf2cc9767b564f12ccd6022ef8167d8e5eefdc6c898f3077a
b6c5fc3622 Remove MAX_SEQUENCE const (Tobin C. Harding)
Pull request description:
This is follow up work to the recent addition of the `Sequence` type. We
do not need to keep a public integer const for `MAX_SEQUENCE` because we
offer the `Sequenc::MAX` associated type.
Use the all-bits-set u64 directly in the associated type `Sequence::MAX`.
cc nlanson
ACKs for top commit:
Kixunil:
ACK b6c5fc3622
apoelstra:
ACK b6c5fc3622
Tree-SHA512: c0acacc25d7cb2f8774e8c6dee8acb3faca0bf6cbd657054ea7262f26b4dfe8baaf2066662966dc507d26716c0468c97d09c590deec48ecd9d11a415ab2bf9ff
This is follow up work to the recent addition of the `Sequence` type. We
do not need to keep a public integer const for `MAX_SEQUENCE` because we
offer the `Sequenc::MAX` associated type.
Use the all-bits-set u64 directly in the associated type `Sequence::MAX`.
7f554774a3 Update bitcoinconsensus dependency (Tobin C. Harding)
Pull request description:
We recently released a couple of new versions of
`rust-bitcoinconsensus`, the first was mainly to move to git subtree,
included in this release was a bump of the patch version of bitcoin
core. The next release updated bitcoin core major version to 0.20.2
Update our bitcoinconsensus dependency to `0.20.2-0.5.0`.
ACKs for top commit:
apoelstra:
ACK 7f554774a3
sanket1729:
utACK 7f554774a3
Tree-SHA512: e30857846e6dcf237e6c87ea5cce81f091104772ce3293c8b7193f253aab3176627466ca41fc3764c949edb4f6c4ae5dc00968d71a4492e19a8a1a1e9dba7013
We recently released a couple of new versions of
`rust-bitcoinconsensus`, the first was mainly to move to git subtree,
included in this release was a bump of the patch version of bitcoin
core. The next release updated bitcoin core major version to 0.20.2
Update our bitcoinconsensus dependency to `0.20.2-0.5.0`.
5cef2f1a8f Remove stutter for error variants (Tobin C. Harding)
08c4a2204a Allow wrong_self_convention for non-Copy type (Tobin C. Harding)
0786d92a5c Take self by value (Tobin C. Harding)
Pull request description:
[Turns out](https://github.com/rust-lang/rust-clippy/issues/9248) by default clippy does not lint certain parts of the public API.
Specifically, by setting `avoid-breaking-exported-api = false` we can get clippy to lint the public API for various things including
`wrong_self_convention`.
This means the saga of to/into/as continues ... we have used the wrong from for many of our `to_*` methods, they should consme `self`. Patch 1 fixes them. Patch 2 adds an `allow`. Patch 3 fixes a few error enum variant identifiers (removes `Error` suffix).
ACKs for top commit:
Kixunil:
ACK 5cef2f1a8f
sanket1729:
code review ACK 5cef2f1a8f
apoelstra:
ACK 5cef2f1a8f
Tree-SHA512: f1cdb94764a132a7c58f3e97a208f3f8d618e4f2620753d0394590b19d3dab0df7669e187f6e0a3d0aa358a7d8aee5495f78008f244b6eedb33715ff1df151b1
Error variants should not end with the same identifier as the enum,
i.e., they should not stutter.
Found by clippy after setting:
avoid-breaking-exported-api = false
Clippy gives a warning about `wrong_self_convention` because we consume
self in a method called `is_*`. We have to consume self because the
object has a generic `E` (error type) that does not implement `Copy`.
Turns out by default clippy does not lint certain parts of the public
API. Specifically, by setting
avoid-breaking-exported-api = false
we can get clippy to lint the public API for various things including
`wrong_self_convention`.
Clippy emits:
warning: methods with the following characteristics: (`to_*` and `self`
type is `Copy`) usually take `self` by value
As suggested, take `self` by value for methods named `to_*`.
0c9c14128b Remove deprecated `StreamReader` (Martin Habovstiak)
Pull request description:
`StreamReader` was deprecated for a while, yet we needlessly maintain it
and it eats our testing time. This change removes it completely.
Closes#1123
ACKs for top commit:
tcharding:
ACK 0c9c14128b (with the function rename).
sanket1729:
utACK 0c9c14128b.
apoelstra:
ACK 0c9c14128b
Tree-SHA512: 3f10c48081f92c02bc5a9f3187df1d9279451a79eb2776b10b3e4c4dc5370a9816b3eb5f04b6e1ede61dfe13c1844d606ba00b00a2bfc6a56aefda900cd9ccc2
870ad59a5e Rename is_finalized to is_finalizable (sanket1729)
aaadd25ddc Add breaking test that allowed incomplete builders to be created (sanket1729)
0b88051318 Update TaprootBuilder::finalize (sanket1729)
5813ec7ac0 Return EmptyTree instead of OverCompleteTree when there are no scripts to add (sanket1729)
Pull request description:
Found while reviewing https://github.com/rust-bitcoin/rust-miniscript/pull/450/ . There is also a BUG fix in the second commit that would have let users spendinfo from incomplete trees.
The bug was introduced in #936 which I am responsible for ACKing
ACKs for top commit:
apoelstra:
ACK 870ad59a5e
Kixunil:
ACK 870ad59a5e
tcharding:
ACK 870ad59a5e
Tree-SHA512: 61442bd95df6912865cbecdc207f330b241e7fbb88b5e915243b2b48a756bea9eb29cb28d8c8249647a0a2ac514df4737bddab695f63075bd55284be5be228ff
This commit does two things:
1) BUG FIX: We should have checked that the length of the branch is 1
before computing the spend_info on it. This was introduced in #936, but
slipped past my review :(
2) Update the return type to return the consumed `self` value. This
function can only error when there the tree building is not complete.
Returning TaprootBuilderError causes issues in downstream projects that
need to deal with error cases which cannot happen in this function
405be52f5c Impl string conversion traits for `CommandString` (Martin Habovstiak)
Pull request description:
After MSRV bump `try_from` usually refers to `TryFrom` method but
`CommandString` used inherent one making it confusing and non-idiomatic.
This implements `FromStr` and `TryFrom<{stringly type}>` for
`CommandString` and deprecates the inherent method in favor of those.
To keep the code using `&'static str` efficient it provides
`try_from_static` inherent method that only converts from
`&'static str`.
Closes#1135
ACKs for top commit:
sanket1729:
utACK 405be52f5c
tcharding:
ACK 405be52f5c
Tree-SHA512: 754fc960a4bc5c096cccf47b85a620e33fcf863f3c57ea113eae91cd34006168113dd1efc47231e79e6e237e2fc412890cc9e8a72d4cfc633bfebbecdc4610e6
1003ca08e1 Remove needless allocation from BIP-158 encoding (Martin Habovstiak)
Pull request description:
The BIP-158 finalize code was serializing value to `Vec` only to
serialize it to writer right away. Thus the intermediary `Vec` was not
needed.
Even more, the code used `write` which, while correct in case of `Vec`,
could trigger code analysis tools or reviewers.
ACKs for top commit:
tcharding:
ACK 1003ca08e1
sanket1729:
ACK 1003ca08e1
apoelstra:
ACK 1003ca08e1
Tree-SHA512: 01e198726f45ef1b0e4bbe80154674d9db12350281e682531259d1d6467881bc7503cfed30d3837813437c3081a35ba7893c3ae4490d07daf20f1b75dbc62d52
The BIP-158 finalize code was serializing value to `Vec` only to
serialize it to writer right away. Thus the intermediary `Vec` was not
needed.
Even more, the code used `write` which, while correct in case of `Vec`,
could trigger code analysis tools or reviewers.
After MSRV bump `try_from` usually refers to `TryFrom` method but
`CommandString` used inherent one making it confusing and non-idiomatic.
This implements `FromStr` and `TryFrom<{stringly type}>` for
`CommandString` and deprecates the inherent method in favor of those.
To keep the code using `&'static str` efficient it provides
`try_from_static` inherent method that only converts from
`&'static str`.
Closes#1135
071a1c02b7 Implement string parsing for `Sequence` (Martin Habovstiak)
c39bc3909d Extend `ParseIntError` to carry more context (Martin Habovstiak)
Pull request description:
When debugging parsing errors it's very useful to know some context:
what the input was and what integer type was parsed. `ParseIntError`
from `core` doesn't contain this information.
In this commit a custom `ParseIntError` type is crated that carries the
one from `core` as well as additional information. Its `Display`
implementation displays this additional information as a well-formed
English sentence to aid users with understanding the problem. A helper
function parses any integer type from common string types returning the
new `ParseIntError` type on error.
To clean up the error code a bit some new macros are added and used.
New modules are added to organize the types, functions and macros.
Closes#1113
Depends on #994
ACKs for top commit:
apoelstra:
ACK 071a1c02b7
tcharding:
ACK 071a1c02b7
Tree-SHA512: 31cb84b9e4d5fe3bdeb1cd48b85da2cbe9b9d17d93d029c2f95e0eed5b8842d7a553afafcf8b4a87c075aa53cf0274776e893bed6dca37e7dbc2e1ee1d602b8e
c67f74bb81 Upgrade bech32 to version 0.9.0 (Tobin C. Harding)
Pull request description:
Recently `rust-bech32` v0.9.0 was released, this release included
updating the MSRV to 1.41.1.
ACKs for top commit:
apoelstra:
ACK c67f74bb81
Kixunil:
ACK c67f74bb81
Tree-SHA512: 59aac2f40e0bc8c63a7549a988ff7cdc114ca7768421c7926cc6badbaa1d4f9de0c5cf41d987e4f5124a5c9042650e722cf5fbd892e91d0232ac175122eb042d
`Sequence` didn't have `FromStr` nor `TryFrom<{stringly type}>`
implemented by accident. This moves a macro for implementing them from
`locktime` module to the `parse` module, renames it for clarity and uses
it to implement parsing for `Sequence`.
When debugging parsing errors it's very useful to know some context:
what the input was and what integer type was parsed. `ParseIntError`
from `core` doesn't contain this information.
In this commit a custom `ParseIntError` type is crated that carries the
one from `core` as well as additional information. Its `Display`
implementation displays this additional information as a well-formed
English sentence to aid users with understanding the problem. A helper
function parses any integer type from common string types returning the
new `ParseIntError` type on error.
To clean up the error code a bit some new macros are added and used.
New modules are added to organize the types, functions and macros.
Closes#1113
0ed78e543b Add lock time types (Tobin C. Harding)
1390ee12ec Add a max scriptnum constant (Tobin C. Harding)
Pull request description:
Implement a `LockTime` type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY.
For example usage in `rust-miniscript` please see https://github.com/rust-bitcoin/rust-miniscript/pull/408.
### Notes:
I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad.
This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them _both_ timelocks.
If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :)
Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too.
ACKs for top commit:
Kixunil:
ACK 0ed78e543b
apoelstra:
ACK 0ed78e543b
Tree-SHA512: 486fcce859b38fa1e8e6b11cd2f494462d6d7d1d9030d096ce6b260f6c9d0342b8952afe35152bdf3afe323a234a8165ac3d6c946304afcc13406d7a0489d75a
Add a `LockTime` type to hold the nLockTime `u32` value. Use it in
`Transaction` for `lock_time` instead of a `u32`. Make it public so this
new type can be used by rust-miniscript and other downstream projects.
Add a `PackedLockTime` type that wraps a raw `u32` and derives `Ord`,
this type is for wrapping a consensus lock time value for nesting in
types that would like to derive `Ord`.
Integers within Script can have a maximum value of 2^31 (i.e., they are
signed) but we (miniscript) often uses unsigned ints, to facilitate
checking the unsigned type is the correct size to fit in a signed int
add a const `MAX_SCRIPTNUM_VALUE`.
fe840f0b42 Fix stale toolchain docs (Tobin C. Harding)
73b506e149 Remove stale MSRV docs (Tobin C. Harding)
Pull request description:
We have stale docs referring to the old MSRV. We do not need MSRV docs
in `CONTRIBUTING` because we have them in the README already.
Fix stale docs by doing:
- Remove the MSRV docs from readme in favour of `CONTRIBUTING.md`.
- Add a sentence to the redame pointing readers towards `CONTRIBUTING.md`.
- Point readers towards `RUSTUP_TOOLCHAIN`
ACKs for top commit:
apoelstra:
ACK fe840f0b42
Kixunil:
ACK fe840f0b42
Tree-SHA512: 6d3006db7a460e5d7146b73ff4b99dc322d396e65da39f5ea47c4904aa387c8a36e4a8ae707653dbab90096e692bf69918e6c19c4d3d989c85b22dba4dcba904
74f3a5aeda Run clippy from the test script (Tobin C. Harding)
aa8109a791 Use struct field short form (Tobin C. Harding)
d1a05401f4 Remove redundant calls to clone (Tobin C. Harding)
196492554d Use assert instead of assert_eq (Tobin C. Harding)
3173ef9dbb Remove unnecessary explicit reference (Tobin C. Harding)
Pull request description:
Currently we run clippy in CI using a github action. The invocation has a couple of shortcomings
1. it does not lint the tests (this requires `--all-targets`)
2. it does not lint the examples
I could not find a way to lint the examples without explicitly linting each example by name.
**This PR does the following:**
- Fix clippy issues (patch 1-4)
- Move the clippy control to `test.sh`
- Add an env var `DO_LINT` to control it
- Remove the separate CI job
- Run the linter during the `Test` job using the stable toolchain.
- Run clippy with ` --all-features` AND `--all-targets` (only recently made possible).
- Run each example explicitly
Thanks to dunxen for noticing all the errors in my psbt example on review and prompting me to work out why clippy was not running :)
ACKs for top commit:
apoelstra:
ACK 74f3a5aeda
Kixunil:
ACK 74f3a5aeda
Tree-SHA512: 56dc6262144f4caa5efa6fdc46aeecf7bddc050ef134a639b31a34d4c5e01abcdeda18a00f4ded443866bbdfc982b4e5b67b0089639e0c253e207f0b54777f57
We have docs that use an env var `BITCOIN_MSRV` that no longer exists.
Point readers towards `RUSTUP_TOOLCHAIN`.
(Note that we still use `TOOLCHAIN` in the script but it is being phased
out, `RUSTUP_TOOLCHAIN` works today.)
We have stale docs referring to the old MSRV. We do not need MSRV docs
in `CONTRIBUTING` because we have them in the README already.
Fix stale docs by doing:
- Remove the MSRV docs from readme in favour of `CONTRIBUTING.md`.
- Add a sentence to the redame pointing readers towards `CONTRIBUTING.md`.
Andrew Poelstra
Tobin C. Harding
Martin Habovstiak
sanket1729