abc014a079 Move rustdoc above attribute (Tobin C. Harding)
e9230019eb Implement std::error::Error::source for MerkleBlockError (Tobin C. Harding)
613f1cf2d5 Implement std::error::Error::source for bip152 (Tobin C. Harding)
a37ab82503 Add non_exhaustive to _all_ errors (Tobin C. Harding)
Pull request description:
Do error cleanups, and add a script to help find missing `non_exhaustive` on error types
- Patch 1: Audit the codebase and put `non_exhaustive` on all error types.
- Patch 2: Implement `std::error::Error::source` for `bip152::Error`
- Patch 3: Implement `Display` and `std::error::Error::source` for `MerkleBlockError`
- Patch 4: Move rustdocs to above attributes on one error type
- ~Patch 5: Add a python script to `contrib` that checks the codebase for missing non exhaustive on the line above the results of regex `pub enum .*Error`~
I removed the Python script patch, I can't be bothered working on this for now but the clean ups and `non_exhaustive` additions are useful IMO.
### labels
- I added 'API break', I think the `non_exhaustive` addition requires major bump but not sure?
- release notes mention is just for the `non_exhaustive` patch.
ACKs for top commit:
Kixunil:
ACK abc014a079
apoelstra:
ACK abc014a079
Tree-SHA512: 16ea15014eae97de7ac9cca1e9b76304aa3702a98cde577c2d71343022f840d3b33a39d2ab6d3fba0f0f1ebaa1631a0595eea1d794ba9727fe6ccfcf08e2feae
22fd107e16 Run rustfmt (Tobin C. Harding)
e1ae9d86bb Move the address module out of util (Tobin C. Harding)
6cc8b22f82 Run cargo fmt (Tobin C. Harding)
Pull request description:
The identifier 'util' does not convey any real information. We have a whole bunch of modules inside the `util` module.
As part of work to reduce the amount of arbitrary things in the `util` module move the `address` module to the crate root level.
- Patch 1: fixes rustfmt issues currently on master, this patch is in heaps of my open PRs at the moment
- Patch 2: Does the module move
- Patch 3: Runs the formatter, done separately so reviewers can run `rustfmt +nightly fmt` and check the diff
ACKs for top commit:
sanket1729:
ACK 22fd107e16. Reviewed range diff from 9e7998897639ddcf115296b72fbb74ed5d4592c8 which I previously ACKed.
Kixunil:
ACK 22fd107e16
apoelstra:
ACK 22fd107e16
Tree-SHA512: b1e3384b79b61e2a27bb4afa0a1a37f9da359bd4ab0a2338a45aa29e1a64394a97b552beafa55a8540ed7dcbd99fe6e1aea573dd37648e705b96176a96e0e27f
Currently we have a bunch of sighash types defined in the `transaction`
module and we have a newer `sighash` module that defines a bunch of
related types. This is kruft from the development process as we added
the new types. If we put things that are related together it makes it
easier for devs to find their way around the library and grok the code.
Move all the sighash types from the `blockdata::transaction` module to
the `sighash` module.
Refactor only, no logic changes.
Clean up some imports, only do the ones that we are about to touch.
- Remove/add whitespace to group by crate
- move modules/types to their module instead of using lib wide
re-export (this helps minimise future merge conflicts)
Re-order imports, refactor only. No logic changes.
We already export some of the sighash related types, there are others
that are uniquely named that can also be exported. Doing so makes use of
the library more ergonomic because devs do not have to know where types
are defined.
We just moved the `address` module out of `util` which is currently
ignored by `rustfmt`.
Run `rustfmt`, no other changes other than those made by the tool.
The identifier 'util' does not convey any information. We have a whole
bunch of modules inside the `util` module.
As part of work to reduce the amount of arbitrary things in the `util`
module move the `address` module to the crate root level.
We have formatting issues on master because we do not yet run the
formatter in CI (PR is open).
We are about to move the `address` module to `/src` which will require
running the formatter.
Run `cargo fmt` to fix formatting issues, no other changes.
7001e93ad5 Fix clippy warnings (Tobin C. Harding)
Pull request description:
We have a bunch of clippy warnings on master, at least one of them is from a patch dated from March, before we ran clippy on CI.
Fix clippy warnings
- warning: accessing first element with `self.0.get(0)`
- warning: deref on an immutable reference
- warning: you are deriving `PartialEq` and can implement `Eq`
All one patch because clippy has to run cleanly for each patch on CI now.
Please note the `Eq` change, adding this derive is an API change.
ACKs for top commit:
apoelstra:
ACK 7001e93ad5
sanket1729:
ACK 7001e93ad5
Tree-SHA512: b59bf92a8222d236538fe38951a92d0daaf86fc19e4ec4752c0eaf3e5b59025953560d089c72141d90f72ea137ec70425b4fa438ebfae65160fbd8799b95af7d
We have a bunch of clippy warnings on master, at least one of them is
from a patch dated from March, before we ran clippy on CI.
Fix clippy warnings
- warning: accessing first element with `self.0.get(0)`
- warning: deref on an immutable reference
- warning: you are deriving `PartialEq` and can implement `Eq`
All one patch because clippy has to run cleanly for each patch on CI
now.
Please note the `Eq` change, adding this derive is an API change.
dfb8ef97dc Remove deprecated MerkleBlock methods (Tobin C. Harding)
b171f88bc3 Remove deprecated SighashComponents struct (Tobin C. Harding)
c2bbf7c343 Use SighashCache instead of SigHashCache (Tobin C. Harding)
1c04a99ec3 Use sighash::SighashCache for bip143 unit tests (Tobin C. Harding)
f65e034742 Refactor test module imports (Tobin C. Harding)
Pull request description:
I'm not sure what the exact policy we decided on for deprecating is but we have code deprecated since v0.24.0 and v.026.2, both of these seem reasonable safe to remove.
Removal of `SighashCompontents` uncovered the fact that we never moved the BIP 143 test vector code over to the new `SighashCache`.
- Patch 1: Trivial preparatory cleanup
- Patch 2: Re-write bip143 test vectors using `sighash::SighashCache`
- Patch 3: Trivial, use `SighashCache` instead of `SigHashCache`
- Patch 4: Remove `SighashComponents` struct deprecated in 0.24.0
- Patch 5: Remove `MerkleBlock` methods deprecated in 0.26.2
ACKs for top commit:
apoelstra:
ACK dfb8ef97dc
sanket1729:
ACK dfb8ef97dc
Tree-SHA512: 493525859a131307fbe3ea17f8edb70045dc56526d5579a4fc0d6e7c3fded58c890304721502d1cfceed0a61f4b3d1827a60760e42af0114f3b3ba29b136238c
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`.
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
Andrew Poelstra
Tobin C. Harding
sanket1729
Martin Habovstiak