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
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
95b7a95fc2 test: correct psbt used for regression test (Subhradeep Chakraborty)
Pull request description:
The `unknown` key used in the `Psbt` (in `serde_regression_psbt` test) is not really unknown. Also the `preimages` in the `Input` don't seem to be valid. Though they don't affect the tests because of the `serde::serialize`, the `Psbt` itself is invalid and hence this PR.
ACKs for top commit:
tcharding:
ACK 95b7a95fc2
apoelstra:
ACK 95b7a95fc2
Tree-SHA512: 275320bb273163364640351e55a3f16c34c1eef06e9dde79b72bf40187d313f037edb5e4544f4dce32fb8a7cc552504aa2e4d22a646c89a3dec022f917539219
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
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.
Tobin C. Harding
Andrew Poelstra
junderw
Subhradeep Chakraborty
Clark Moody
Riccardo Casatta
yancy