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`.
We are no using `non_exhaustive` on any error types unless we are
_really_ sure there will be no additional variants required.
There are only three error enums that do not have it in the codebase as
of this commit, add it to all three.
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`.
The `bip143::SigHashCache` is deprecated in favour of
`sighash::SighashCache` however we still use it in `bip143` unit tests.
Use the new `sighash::SighashCache` in `bip143` unit tests.
We have a bunch of unit tests that verify the BIP 143 test vectors.
Three of these use the deprecated `SighashComponents` struct. We should
implement these tests using the new `SighashCache`. In order to do so we
have to move the tests to the `sighash` module so they can get access to
the private `segwit_cache()` function, and verify the cache internal
state against the test vectors.
Re-write the BIP 143 test vector code using `SighashCache` and remove
the versions using `SighashComponents`.
Done in preparation for removing the deprecated `SighashComponents`.
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
Recently we introduced SPDX license ids but there was some confusion
about the use, form, and purpose of lines before (library name, author).
It was pointed out to me by Andrew that if/when folks cut'n'paste our
code they often will keep the whole blurb. Design it with that in
mind. FTR, Andrew also indicated that he did not mind his name being
removed in favour of "The rust-bitcoin developers".
At first I thought the term "The rust-bitcoin developers" was a bit
wishy-washy but yesterday I realised that I am proud to be part of that
crew, striving to deliver code to the highest possible standard.
Introduce a canonical format for the licences blurb.
- Use the name of the library
- Use "The rust-bitcoin developers"
- Use the SPDX ID
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
Martin Habovstiak
Tobin C. Harding
Andrew Poelstra
sanket1729