2ecab31f94 Remove stale comment and map_err (yancy)
b166442fb0 Replace hex_psbt macro with test helper function (yancy)
9e4a784b8b Move psbt macro to the psbt test module (yancy)
Pull request description:
Remove `#[cfg(test)]` and the macro `psbt_with_values` from macros.rs and place it in the tests module for psbt.
ACKs for top commit:
apoelstra:
ACK 2ecab31f94
tcharding:
ACK 2ecab31f94
Tree-SHA512: 06a55056e864befac8b33968bf4e469c3c7bc20e651ad5bb3b80aa76749169af1266e1d4101d3e9e9bbffe7c860e8b9fcd675a78ca7ae67dc09892c75fba0dd0
875545517d Add clippy exceptions for needless_question_mark lint (Steven Roose)
Pull request description:
This lint forces you to write semantically different code that is in most cases inferior, just to save you 5 characters.
The reason why the code is inferior is because it doesn't do error conversion so it would break when either of the two function signatures changes while in the original code using the `?` operator, nothing would break if the inner error can be converted into the outer error.
ACKs for top commit:
apoelstra:
ACK 875545517d
tcharding:
ACK 875545517d
Tree-SHA512: 8429e0fb7d759a3d19231e7bcaed61b0988172d931e758a9522d7c994854fd403408bb93b06778a5c09746cd38b6a96d3d2e0a862fb4516f2dbfffffe8735ce0
hex_psbt was added as a macro so that a panic would reveal the line
number of the failing test by expanding the macro at the test location.
However, a stack trace can be used to reveal the test that caused the
failure using RUST_BACKTRACE=1. Furthermore, the track_caller macro is
added to the helper methods which will reveal the line number of the
calling function (the offending test). More detailed information for
debugging has been added to hex_psbt() so that the offending string
will be included in the panic message.
The macro psbt_with_values is used by the psbt test module. Since
there is no pre-processing required, there is no reason to use
metaprogramming here, so this commit moves the logic from a
macro to common function in the test module.
38005f6aa7 Use Target for pow_limit (Tobin C. Harding)
Pull request description:
The `Params::pow_limit` field is currently a `Work` type, this is incorrect. The proof of work limit is the highest _target_ not the lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they differ to Core.
Use the new consts in the various network specific `Params` types.
Fix: #2106
ACKs for top commit:
junderw:
ACK 38005f6aa7
apoelstra:
ACK 38005f6aa7
Tree-SHA512: 5e71f69cdd555fd95a52fc1479e27b5e11226772f6432758c1364107a068bd1271486da6db1ece694da0287ce10cfbc18d28d6e3dbb0b9d387ff968eea43ab18
The `Params::pow_limit` field is currently a `Work` type, this is
incorrect. The proof of work limit is the highest _target_ not the
lowest work (even though these have a relationship).
Note that we use the highest _attainable_ target, this differs from
Bitcoin Core and the reasoning is already documented in the code.
Add new consts and document where they came from as well as how they
differ to Core.
Use the new consts in the various network specific `Params` types.
We have a new API function available with recent version of `secp256k1`
to create a `Message` directly from a sighash byte array.
Use `Message::from_digest(sighash.to_byte_array())` to construct
messages ready to sign.
Upgrade the `secp256k1` dependency to the newly released `v0.28.0`.
FTR this includes two simple changes:
- Use `Message::from_digest_slice` instead of `Message::from_slice`.
- Use `secp256k1::Keypair` instead of `secp256k1::KeyPair`.
In preparation for updating the secp dependency to v0.28.0, which
includes a change of `KeyPair` to `Keypair`, change our identifier usage
to indicate that "keypair" is a single word.
Deprecate the old forms.
Update the `bech32` dependency to use the newly release beta version.
The main fix here is silent, a bug fix in `bech32` that was being hit by
our fuzzing suite.
6b5d06f23e ci: fix the byteorder to 1.4.3 for edition 2018 (Vincenzo Palazzo)
98513ef151 clippy: more worning fixes (Vincenzo Palazzo)
05d3dc5d72 Remove redundant guard (Tobin C. Harding)
4537634e7e ci: bump rustc to 1.60 for fuzz test (Vincenzo Palazzo)
Pull request description:
Ci looks like broken, so this should fix
it
ACKs for top commit:
apoelstra:
ACK 6b5d06f23e
Tree-SHA512: bfa0eaf8cbc02a671237d99221db8c21264ce9df91301818c95c41dcc5ad4935e91254b0b3fa8f36738a9d71b6541fb8784ac8280d67057960a3d20e385a9f17
8eff4d0385 Remove private hex test macro (Tobin C. Harding)
Pull request description:
We have this macro in `hex-conservative` now, remove the version here.
This patch does not change the public API and only touches test code.
ACKs for top commit:
apoelstra:
ACK 8eff4d0385
clarkmoody:
ACK 8eff4d0385
Tree-SHA512: 93a08fff778930071cd1a28c19202e4a94ca8881b2e873538de2e942b71c2cd6184ed6364c572538a8a699295a71761c6f836accaf251a15683138b71f148fab
10374af75c Make error types uniform (Tobin C. Harding)
43d3306822 Use explicit error::Error impl instead of the default (Tobin C. Harding)
2512dbafc2 Remove impl_std_error macro (Tobin C. Harding)
6933ca4fc2 Add suffix to HiddenNodes error type (Tobin C. Harding)
2b40ea24fb Add suffix to IncompleteBuilder error type (Tobin C. Harding)
f41416a0ea Add suffix to UnknownMagic error type (Tobin C. Harding)
5658dac024 Add suffix to UnknownChainHash error type (Tobin C. Harding)
2fb71dd943 Move p2p error types to bottom of file (Tobin C. Harding)
39314ad52f Move error code to match conventional layout (Tobin C. Harding)
Pull request description:
PR aims to achieve two things:
- Make error code brain dead easy to read
- Get error code closer to being ready for v1.0
The first 8 patches are pretty basic, and are broken up into really small changes. The last patch is much bigger, it has a long git log to explain it but reviewing should not take too much brain power.
This PR does not introduce anything new, it just applies what we have been doing recently with errors. Before v1.0.0 others will likely want to re go over all the error types. As such I believe this PR can be merged under the one ack carve-out.
### TODOs (future PRs)
We have a few errors that still need splitting up:
- Split up `merkle_tree::block::MerkleBlockError`
- Split up `psbt::error::Error`
- Split up `IncompleteBuilderError`
Also, all error From's should probably have `#[inline]`, I noticed late in the process and did not have the heart to visit every error again.
ACKs for top commit:
apoelstra:
ACK 10374af75c
clarkmoody:
ACK 10374af75c
Tree-SHA512: 4f4f3533f42dc11af8e7978f3272752bb56d12a68199752ed4af0c02a46a87892b55c695b7007bc3d0bdf389493068d068e2be1780e8c3008815efec3a02eedf
On our way to v1.0.0 we are defining a standard for our error types,
this includes:
- Uses the following derives (unless not possible, usually because of `io::Error`)
`#[derive(Debug, Clone, PartialEq, Eq)]`
- Has `non_exhaustive` unless we really know we can commit to not adding
anything.
Furthermore, we are trying to make the codebase easy to read. Error code
is write-once-read-many (well it should be) so if we make all the error
code super uniform the users can flick to an error and quickly see what
it includes. In an effort to achieve this I have made up a style and
over recent times have change much of the error code to that new style,
this PR audits _all_ error types in the code base and enforces the
style, specifically:
- Is layed out: definition, [impl block], Display impl, error::Error impl, From impls
- `error::Error` impl matches on enum even if it returns `None` for all variants
- Display/Error impls import enum variants locally
- match uses *self and `ref e`
- error::Error variants that return `Some` come first, `None` after
Re: non_exhaustive
To make dev and review easier I have added `non_exhaustive` to _every_
error type. We can then remove it error by error as we see fit. This is
because it takes a bit of thinking to do and review where as this patch
should not take much brain power to review.
In a further effort to make the code brain-dead easy to read; use an
explicit implementation of `std::error::Error` that returns `None`
instead of relying on the default trait implementation.
We would like the codebase to be optimized for readability not ease of
development, as such code that is write-once-read-many should not use
macros.
Currently we use the `impl_std_error` macro to implement
`std::error::Error` for struct error types. This makes the code harder
to read at a glance because one has to think what the macro does.
Remove the `impl_std_error` macro and write the code explicitly.
752adff9d1 Add method calc_height (Tobin C. Harding)
46f5588646 Add unit test for calc_tree_width (Tobin C. Harding)
Pull request description:
Add a private `PartialMerkleTree::calc_tree_width` function and a unit test to test it.
ACKs for top commit:
apoelstra:
ACK 752adff9d1
clarkmoody:
ACK 752adff9d1
Tree-SHA512: 9c4ad9f6ff47d8faad1c7c1e977427f1528af2712ceffd05357d0c9117b5fdb7b2783afc00a75cb19b853bfbd7b3895baa3a3563bdc496593cc9b06ce80dbbf8
dac627cc09 Feature: Psbt fee checks (junderw)
Pull request description:
Closes#2061
These new methods on Psbt will add checks for high fees by default. The threshold for "high fees" is currently set to 25000 sat/vbyte, which is about 20x higher than the highest next block fees seen on the "Mempool" website.
The primary goal of this change is to prevent users of the library from accidentally sending absurd amounts of fees.
(ie. Recently in September 2023 there was a transaction that sent an absurd amount of fees and made news in the Bitcoin world. Luckily the mining pool gave it back, but some might not be so lucky.)
There are variants of the method that allow for users to set their own "absurd" threshold using a `FeeRate` value. And there is a method that performs no checks, and the method name is alarming enough to draw attention in a review, so at least developers will be aware of the concept.
ACKs for top commit:
apoelstra:
ACK dac627cc09
tcharding:
ACK dac627cc09
Tree-SHA512: ae0beafdb50339ba3efc44a48ba19c0aeeb0a2671eb43867c1e02b807677ce99fb6b4c47b74a9ed2999f827b3edc00a8871fa4730dd12a4cb265be99437c13db
c34e3cc7cc Re-write size/weight API (Tobin C. Harding)
73f7fbf520 Add code comments to transaction serialization (Tobin C. Harding)
29f20c1d0b Add segwit serialization constants (Tobin C. Harding)
Pull request description:
Audit and re-write the weight/size API for `Block` and `Transaction`. First two patches are trivial, patch 3 contains justification and explanation for this work, copied here:
```
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
```
Please note, this PR introduces panics if a sciptPubkey overflows the calculation `weight = spk.size() * 4`.
Fix#2049
ACKs for top commit:
apoelstra:
ACK c34e3cc7cc
sanket1729:
ACK c34e3cc7cc.
Tree-SHA512: 4944f652e6e362a282a5731140a9438a82d243a4c646b4627d9046a9f9cf13c476881750d432cfbc6b5fe5de1f0c4c9c44ed4569dac4bc11b55a5db28793803c
5901d35095 Add push_p2wpkh function on Witness (Tobin C. Harding)
8cd409d561 Deprecate push_bitcoin_signature (Tobin C. Harding)
Pull request description:
In order to create the witness to spend p2wpkh output one must create a `Witness` that includes the signature and the pubkey, we should have a function for this.
## Notes
The PR originally added a `push_p2wphk` method, this is now instead a constrcutor `Witness:p2wpkh` (after review discussion below).
- Patch 1 changes `push_bitcoin_signature` to take an `ecdsa::Sigtnture` instead of an `ecdsa::SerializedSignature`
- Patch 2 takes a `secp256k1::PublicKey` removing the need for an error path (discussed below).
ACKs for top commit:
sanket1729:
ACK 5901d35095
apoelstra:
ACK 5901d35095
Tree-SHA512: 646014d97daafbf0909106d8990debaf481ac6f3578f0ddf232d739c3e2d55ae1d0275abe5a4a1db1c5c192c8c5f0b5546fc65aac37b91a3729db881c5ad3dec
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
The `Witness::push_bitcoin_signature` method is old and a bit stale.
Bitcoin has taproot signatures now so the name is stale, also we have
the `crate::ecdsa::Signature` type that holds the secp sig and the hash
type so we can use that instead of having two separate parameters.
Add a new, up to date, `Witness::push_ecdsa_signature` function and
deprecate the `push_bitcoin_signature` one.
There is no logical default for the transaction version number, there is
only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version
they want not rely on us making the choice.
(I originally added this impl to support testing, this was in hindsight
the wrong thing to do, props to Sanket for noticing.)
One of our stated aims is to make it possible to learn bitcoin by using
our library. To help with this aim add to private consts for the segwit
transaction marker and flag serialization fields.
158ba26a8a Feature: Count sigops for Transaction (junderw)
Pull request description:
I copied over the sigop counting logic from Bitcoin Core, but I made a few adjustments.
1. I removed 2 consensus flags that checked for P2SH and SegWit activation. This code assumes both are activated. If we were to include that, what would be a good way to go about it? (ie. If I run this method on a transaction from the 1000th block and it just so happened to have a P2SH-like input, Bitcoin Core wouldn't accidentally count those sigops because the consensus flag will stop them from running the P2SH logic. Same goes for SegWit)
3. Since there's no guarantee that we have an index from which we can get the prevout scripts, I made it into a generic closure that looks up the prevout script for us. If the caller doesn't provide it, We can only count sigops directly in the scriptSig and scriptPubkey (no P2SH or SegWit).
## TODO
- [x] Write tests for transaction sigop counting
~~Edit: The test changes are just to get the 1.48 tests passing. I'll remove them and replace them with whatever solution that is agreed upon in another PR etc.~~
Edit 2: This is the code I used as a guide:
8105bce5b3/src/consensus/tx_verify.cpp (L147-L166)
Edit 3: I found a subtle bug in the implementation of `count_sigops` (https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687)
ACKs for top commit:
apoelstra:
ACK 158ba26a8a
tcharding:
ACK 158ba26a8a
Tree-SHA512: 2b8a0c50b9390bfb914da1ba687e8599b957c75c511f764a2f3ed3414580150ce3aa2ac7aed97a4f7587d3fbeece269444c65c7449b88f1bdb02e573e6f6febd
e4c7e01a6f Use the new bech32 iterator API (Tobin C. Harding)
Pull request description:
Depend on the newly released version of `bech32`, BOOM!
ACKs for top commit:
apoelstra:
ACK e4c7e01a6f
clarkmoody:
ACK e4c7e01a6f
Tree-SHA512: 91675a830cf67f8dcabd42e7dc1b70d80b669330be5244bb8102e0ec5d1a206d5ead07f73b328a158b761c328bc78d573185af8d31f14183ccc17318d752c02b
b2a7d7023c Rename XpubIdentifier to XKeyIdentifier (Tobin C. Harding)
ffd2466ad1 Move XpubIdentifier to the bip32 module (Tobin C. Harding)
Pull request description:
- Patch 1: Move the hash to the `bip32` module where it is used, as we have done with other hashes recently (and re-export it at crate root).
- Patch 2: Rename the hash to `XKeyIdentifier` as discussed in #2014Fix: #2014
ACKs for top commit:
apoelstra:
ACK b2a7d7023c
Tree-SHA512: 5efa9fc857c71e506263bf6adee3b4294f22838d5b119177c9108c69191d545338c11a4796bc95e956a67f3418010725f3d12c06d2c4c3bb5cf038d59976ae0f
71a5fe2b54 Customize Debug implementation of absolute::LockTime (Subhradeep Chakraborty)
Pull request description:
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/2011.
This PR aims to make the "pretty print" of `absolute::LockTime` prettier by printing `X blocks` and `X seconds` for `Blocks` and `Seconds` respectively instead of the default Enum printing.
ACKs for top commit:
apoelstra:
ACK 71a5fe2b54
tcharding:
ACK 71a5fe2b54
Tree-SHA512: 79baad5ee0ba1aa892cb38588dae6a24977b8e42356208d6cc9adb007d1f8371e61dc82e35bb4dac9961216d68c56907f0db376c5c6fbb5a406728b8f7c3f5ad
bc398204bf Remove redundant segwit version from function names (Tobin C. Harding)
Pull request description:
A P2TR output does not need to be clarified with version 1, it is implicit. As with p2wpkh/p2wsh and version 0.
Remove redundant version identifiers from function names, deprecating the originals.
ACKs for top commit:
apoelstra:
ACK bc398204bf
Tree-SHA512: 49806c564badca25ce02161445b2b41497b565f2002aa1edfc0cf0c57b38683480deec0d9b682e18dc7e59c22128e0b641abcccc2cbedd0b5603cbcbf2fd26df
f17bb0d18f Remove unnecessary reference (Tobin C. Harding)
Pull request description:
`T` is a generic that implements`AsRef<PushBytes>`, it should not be a reference. This is inline with other usages of `AsRef<PushBytes>` for example in `Builder::push_slice`.
Found while working on #2003
ACKs for top commit:
apoelstra:
ACK f17bb0d18f
Tree-SHA512: 6f6ae0ba5d5010db53d9c2af107df84bc058277b2b7cc35800f4e6ed93d351838b7f101284b7d80345bee639615d27d76a2e5c4c784782c5b3e5090444defe29
BIP-68 activated a fair while ago (circa 2019) and since then only
transaction versions 1 and 2 have been considered standard.
Currently in our `Transaction` struct we use an `i32`, this means users
can construct a non-standard transaction if they do not first look up
what the value should be. We can help folk out here by abstracting over
the version number.
Since the version number only governs standardness elect to make the
inner `i32` public (ie., not an invariant). The aim of the type is to
make life easy not restrict what versions are used.
Add transaction::Version data type that simply provides two consts `ONE`
and `TWO`.
Add a `Default` impl on `Version` that returns `Version::TWO`.
In tests that used version 0, instead use `Version::default` because the
test obviously does not care.
52f2332383 Remove docs from witness version conversion functions (Tobin C. Harding)
47d6d785cb Remove bip 173/350 test vectors (Tobin C. Harding)
e0eaeaad99 Split ParseError out of Error (Tobin C. Harding)
0f536e86dc Add new UnknownAddressTypeError for parsing address type (Tobin C. Harding)
e2014cba1b Import error variants within dislay impl (Tobin C. Harding)
9d7791fcd6 Remove unnecessary self:: from error import (Tobin C. Harding)
b2e485ed51 Split the address error code out into a separate module (Tobin C. Harding)
f34ca0c52b Move address.rs to address/mod.rs (Tobin C. Harding)
Pull request description:
In preparation for depending on the recently released version of `rust-bech32` do a bunch of preparatory fixes.
1. Improve `address` module error handling as we are doing else where at the moment
2. Remove bip 173 and 350 test vector tests, these are fully covered in bech32
3. Trim down the docs on `WitnessVersion`
This PR is the first 8 patches of https://github.com/rust-bitcoin/rust-bitcoin/pull/1951
ACKs for top commit:
sanket1729:
ACK 52f2332383
apoelstra:
ACK 52f2332383
Tree-SHA512: 67a4ea4020b4e5c9c8396e4195e06dbd1d11335788f9e52f60abbc0b399e37e5dacc9bb7fa4e88221670322fa3c3407ade059d5c709f96e2df97240f4524e08c
ccdcffe69c Re-export Opcode (Tobin C. Harding)
Pull request description:
We recently rename `opcodes::All` to `Opcode` but did not re-export it from the crate root. Since it now has a nice clear name we can do so.
Found while hacking up the `rust-bitcoincore-rpc` dependency upgrade for upcoming release `bitcoin v0.31.0`.
ACKs for top commit:
apoelstra:
ACK ccdcffe69c
Tree-SHA512: 8acac90d01f14245f93edc10f3483d8aa9865aca59b6ef42ab744b875558fca8ad74894ad95a83637f0cec7a2a353d74d4d6f7ee5f1d1276cc0fc3dcb6983362
acbf23aaa5 Add `is_multisig` helper to Script type (Clark Moody)
Pull request description:
A new `is_multisig` helper method to classify bare multisig output scripts.
The form of a valid multisig script is:
- Pushnum `M`
- <N> pubkeys
- Pushnum `N`
- `OP_CHECKMULTISIG`
`N` must equal the number of pushed pubkeys, and `M` must be less than or equal to `N`.
I've tested this against the RPC output of Core at the block level, checking that the total number of multisig outputs matches.
```
Block 350338, 89 multisig
Block 350340, 29 multisig
Block 350341, 4 multisig
Block 350343, 579 multisig
Block 350344, 48 multisig
Block 350346, 11 multisig
Block 350347, 404 multisig
Block 350350, 127 multisig
Block 350351, 1 multisig
Block 350353, 40 multisig
Block 350356, 13 multisig
Block 350357, 2 multisig
Block 350358, 1 multisig
```
ACKs for top commit:
tcharding:
ACK acbf23aaa5
apoelstra:
ACK acbf23aaa5
Tree-SHA512: b8feeaa8725ac63a658897dac3b303fc8b3d56674d796b14569548124928329993bea45482928d9ce85231f1b5837922af8c0a77b2601a92f88b5e2a9394e97f
026a55809e Fix: Script::count_sigops parsing should not return a Result (junderw)
Pull request description:
When implementing some tests for the Transaction PR, I noticed that there were coinbase transactions that would pass Bitcoin Core parsing and fail my code.
It turns out that the Script parsing for sigops calls `break` to exit the loop and returns the current n value whenever there is an EarlyEndOfScript error.
See this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687 for some links to the relevant source.
ACKs for top commit:
apoelstra:
ACK 026a55809e
tcharding:
ACK 026a55809e
Tree-SHA512: 57c1b88add5e1c9ef9245fcec0e471db55c2f9b1b0b0f8ebd471f1bede0ca5eeb8492d8c75dea1fd43f1343037df44969c9b9fde26a7de9ac68a26dca899e47f
These docs do not add that much value, we do not typically bother
documenting `From` and `TryFrom` implementations because they are super
well known and its obvious from the function signature what is going on.
The `address::Error` is module level general, we can make the code
easier to maintain and easier to stabalize by splitting the parse error
out of the general error.
Create a `ParseError` that is returned by `FromStr for Address`. Remove
the now unused variants from the general `address::Error`.
In an effort to reduce the number of lines of code import the error
variants locally within the `Display` impl on `Error`.
Refactor only, no logic changes.
Split the error code out of `address/mod.rs` and into
`address/error.rs`. Code move only, no changes other than to
imports/exports etc. to make it build.
a0a3d4728a Fix deprecation notice (Tobin C. Harding)
Pull request description:
Recently we deprecated the `segwit_signature_hash` function but during development the deprecation notice got stale.
Fix deprecation notice to use the actual function names.
ACKs for top commit:
RCasatta:
ACK a0a3d4728a
apoelstra:
ACK a0a3d4728a
Tree-SHA512: d84941b605c5bc6ceab75cd60eb820c1d2c16fcd1431dc3927dc22d79886d3de26fd796fab92d97e7f8d567eab0b5a1987303107720524e7b648b1168541a2ed
7309c7749a Split witness version errors up (Tobin C. Harding)
40db2f5ed6 witness_version: Remove rustdocs from TryFrom imlps (Tobin C. Harding)
3397ff9910 witness_version: Use Self in error From impl (Tobin C. Harding)
Pull request description:
Done as part of the push to have small specific errors instead of large general ones.
Split the `witness_version::Error` up into small specific errors.
The first two patches are preparatory clean up.
ACKs for top commit:
stevenroose:
ACK 7309c7749a
apoelstra:
ACK 7309c7749a
Tree-SHA512: 9e8b4bc5db3435c88aa6de9d92f668146b20b292c3609e2d4415ff0c32a0e3923bbe765333e0d7c255c326d65680015fc9cdf3e4a994727f4d0273dc396314df
4f43965ade Make Encodable/Decodable usage uniform (Tobin C. Harding)
Pull request description:
One encodes to a writer and decodes from a reader, most of the time in the consensus `Encodable`/`Decodable` traits we use generic `R`/`W` and variable `r`/`w` but there are other places that use other characters.
While touching these lines note also that there are a bunch of unneeded `mut`s, I'm not sure why since usually between the compiler and the linter `mut` is handled correctly.
Make implementations of `Encodable` and `Decodable` uniform by:
- Use R/W and r/w for trait and variable name
- Remove unneeded mut
(This is split out of #1891 to assist review.)
ACKs for top commit:
apoelstra:
ACK 4f43965ade
stevenroose:
ACK 4f43965ade
Tree-SHA512: 256d080b32e60a7cabf6db4945a18d7ff5b296cb848712238e33f9b1ff3cabbe7d76723fed61a04e4b2a6c9423f12c32ec10e3ebb633761122add50e6a6cc7c9
a68c42e113 Remove test from Transaction test names (yancy)
f796d6fef9 Use Weight type for scaled_size (yancy)
e746341f33 Add tests for scaled_size (yancy)
97b7a2dee9 Use Weight type for block base_size (yancy)
9536a9947c Add base_size test (yancy)
Pull request description:
Use Weight type for `base_size` in Transaction. Also a small re-factor to remove `test_` and `_tests` from the testname for transaction tests.
ACKs for top commit:
apoelstra:
ACK a68c42e113
tcharding:
ACK a68c42e113
Tree-SHA512: f4ab54143cbd9b1439912390f1e0857069a32b715477a4bc08692c5e32860a7090c95a92f78b118b17c1295c45a3bbdd209ba1d68c3a934341269235040e6911
de95bf52cb Use checked_sub (Tobin C. Harding)
Pull request description:
Recently we "if" guarded subtraction manually using `> 0`, we can better convey the meaning by using `checked_sub` and pattern match on the option.
Refactor only, no logic changes.
ACKs for top commit:
RCasatta:
utACK de95bf52cb
apoelstra:
ACK de95bf52cb
Tree-SHA512: 2514cc2d8af89158e5e5e5a866f3fadb4927ba07dfb4e077fd16a98acf638588bee5ce03e2dc73fbda0b5064c30d8773d3be583c03c2a5336b8738c212a9776f
As we have recently been doing, move the declaration of the hash type to
where it is used.
Move the `XpubIdentifier` hash declaration to the `bip32` module.
This is an API breaking change.
A P2TR output does not need to be clarified with version 1, it is
implicit. As with p2wpkh/p2wsh and version 0.
Remove redundant version identifiers from function names, deprecating
the originals.
`T` is a generic that implements`AsRef<PushBytes>`, it should not be a
reference. This is inline with other usages of `AsRef<PushBytes>` for
example in `Builder::push_slice`.
One encodes to a writer and decodes from a reader, most of the time in
the consensus `Encodable`/`Decodable` traits we use generic `R`/`W` and
variable `r`/`w` but there are other places that use other characters.
While touching these lines note also that there are a bunch of unneeded
`mut`s, I'm not sure why since usually between the compiler and the
linter `mut` is handled correctly.
Make implementations of `Encodable` and `Decodable` uniform by:
- Use R/W and r/w for trait and variable name
- Remove unneeded mut
Done as part of the push to have small specific errors instead of large
general ones.
Split the `witness_version::Error` up into small specific errors.
Recently we "if" guarded subtraction manually using `> 0`, we can better
convey the meaning by using `checked_sub` and pattern match on the
option.
Refactor only, no logic changes.
Recently we deprecated the `segwit_signature_hash` function but during
development the deprecation notice got stale.
Fix deprecation notice to use the actual function names.
84614d9997 Unit test debug print of witness with empty instruction (Tobin C. Harding)
e96be5ee6e Fix Witness debug display bug (Tobin C. Harding)
Pull request description:
When we introduce a custom `Debug` implementation for the `Witness` we introduced a bug that causes code to panic if the witness contains an empty instruction.
The bug can be verified by putting patch 2 first or by running `cargo run --example sighash` on master.
ACKs for top commit:
apoelstra:
ACK 84614d9997
RCasatta:
ACK 84614d9997
Tree-SHA512: d51891206ab15f74dda07eb29ff3f6c69dc3f983a5a5abb55685688548481a19f7c1d33aa1183a89c553ff2bc86cf41057c2bae33d75e8a7f3b801056775bf9e
55e94b5dea Remove test from test names for Weight type (yancy)
142dde64c3 Use Weight type for stripped_size (yancy)
cb76f3ec43 Add scale_by_witness_factor to Weight type (yancy)
38c9e9947e Add witness scale factor to the Weight type (yancy)
77552987ab Add from_wu_usize to Weight type (yancy)
1a88c887f5 Rename strippedsize to stripped_size (yancy)
3369257c75 Fix grammar (yancy)
Pull request description:
Return Weight type for the strippedize function.
ACKs for top commit:
apoelstra:
ACK 55e94b5dea
tcharding:
ACK 55e94b5dea
Tree-SHA512: ad3e4bc29380f22e20a6302c1b24c201c772be759c655c62ba4717840a01fcaa36f0f8442c9a3ba71c6400d6af47a9a815e6d90877b5f14c6883fb950b9669fd
29a4f9b114 Wrap the bitcoinconsensus error (Tobin C. Harding)
Pull request description:
Currently the `bitcoinconsensus` error is part of the public API. This hinders maintainability because changes to the verison of `bitcoinconsensus` force a re-release in `rust-bitcoin`. This is an unnecessary maintenance burden, we can wrap the error instead.
ACKs for top commit:
apoelstra:
ACK 29a4f9b114
sanket1729:
utACK 29a4f9b114
Tree-SHA512: 36bc1b0ad5f5675d79eea2409844a839d862997c256e301c53c5f1af547edc9a0b83e586bd70e1b8853722cd7ef279e7515e09fbe942660f8049090d1be39d3a
f18f684ad2 Add version bytes consts (Tobin C. Harding)
Pull request description:
BIP-32 defines 4 4-byte consts used as version bytes; currently we are hardcoding the version bytes in multiple places.
Add BIP-32 version bytes consts and use them throughout the module.
ACKs for top commit:
apoelstra:
ACK f18f684ad2
RCasatta:
utACK f18f684ad2
Tree-SHA512: 50bf2d26f0f8e3528642ffcc621c03b82f536994deb808a6c84225676b4b8849db8e0d16e46f3819e0810296a422b31cf90d0595739910afdb92fb768ef7696e
Tobin C. Harding
Andrew Poelstra
Steven Roose
yancy
Vincenzo Palazzo
junderw
Clark Moody
Subhradeep Chakraborty
Riccardo Casatta