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
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
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.
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
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
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`.
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.
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
Throughout the codebase we cast values to `u64` when constructing a
`VarInt`. We can make the code marginally cleaner by adding `From<T>`
impls for all unsigned integer types less than or equal to 64 bits.
Also allows us to (possibly unnecessarily) comment the cast in a single
place.
Currently if the witness has zero elements or any of the individual
witnesses is empty we panic. Panic is caused by subtracting 1 from a
zero length.
Check the length is non-zero before subtracting 1, print `[]` if empty.
2e3006a729 Add max standard tx weight constant to transaction (yancy)
Pull request description:
Add a constant for the max transaction weight. Similar to [max block weight](1b009b809b/bitcoin/src/blockdata/weight.rs (L35)). This value is pulled from core [here](44b05bf3fe/src/policy/policy.h (L27))
ACKs for top commit:
apoelstra:
ACK 2e3006a729
sanket1729:
ACK 2e3006a729
Tree-SHA512: 1583695f43387538f948be85ded7ff9a4bf9778169acb958debcbe1572a6dc8bfcd26ddfb8dbe0c030c98ab1f8a66d239a5bc663bf65ec3376a46d5f71e90894
9eff2f2f5e fee_rate: Add public absolute weight convenience functions (Tobin C. Harding)
f00e93bdcd Fix typos in rustdoc (Tobin C. Harding)
f3412325ea weight: Make docs uniform and terse (Tobin C. Harding)
Pull request description:
- Patch 1 is docs cleanup
- Patch 2 adds two functions to `FeeRate`
From the commit log of patch 2:
Calculating the absolute fee from a fee rate can currently be achieved
by creating a `Weight` object and using
`FeeRate::checked_mul_by_weight`. This is kind of hard to discover, we
can add two public convenience functions that make discovery of the
functionality easier.
Add two functions for calculating the absolute fee by multiplying by
weight units (`Weight`) and virtual bytes (by first converting to weight
units).
This seems like an obvious thing so I'm inclined to think that Kixunil left it out for a reason. (Mentioning you here Kix so even if this merges you'll see it in notifications later on.)
ACKs for top commit:
sanket1729:
utACK 9eff2f2f5e
apoelstra:
ACK 9eff2f2f5e
Tree-SHA512: 5b3997b721b7d7225bcf0e8a4a3efb6f207393541dbbcef533135af06a4d9c95210d450bb2322fd65a727e4560b29f0b20fb96c154d019fd4e745506213abc1c
27b3c1e0e6 Improve the ScriptHash and WScriptHash types (Tobin C. Harding)
2197f1377f Improve PubkeyHash and WPubkeyHash (Tobin C. Harding)
Pull request description:
Total re-write since review. Now this PR moves the hash type definitions out of `hash_types`. Please see https://github.com/rust-bitcoin/rust-bitcoin/issues/1909#issuecomment-1603634440 for more.
No longer adds unit tests.
Fix: #1909
ACKs for top commit:
apoelstra:
ACK 27b3c1e0e6
Tree-SHA512: 216b9bed05d1a4a4fc493262664ceb5d60f9c30685b63d6f6675d21a7bf811053320a002165487b29599c52f345057d9c92babb0fc1ccd4628671ec468c804f9
50ada8298f Move EncodeSigningDataResult to sighash module (Tobin C. Harding)
1b7dc51ccb Remove deprecated code (Tobin C. Harding)
Pull request description:
We only keep deprecated code around for one release so we can now remove code deprecated in v0.30.0
Done in preparation as we gear up for v0.31.0 release.
ACKs for top commit:
apoelstra:
ACK 50ada8298f
sanket1729:
ACK 50ada8298f
Tree-SHA512: 40769258605563e2e12a6118306655fc9a012ae1f86509fca757ca411f0cef74480b7bb7b0db147f30a7d362b8494a077d5ec04f719351661ceb5a0697a5369d
3c0bb63423 Do trivial rustdoc improvements (Tobin C. Harding)
3225aa9556 Use defensive documentation (Tobin C. Harding)
80d5d6665a crypto: key: Move error code to the bottom of the file (Tobin C. Harding)
fe3b1e1140 Move From for Error impl (Tobin C. Harding)
5f8e0ad67e Fix docs on error type (Tobin C. Harding)
f23155aa16 Do not capitalize error messages (Tobin C. Harding)
ae07786c27 Add InvalidSighashTypeError (Tobin C. Harding)
baba0fde57 Put NonStandardSighashTypeError inside ecdsa::Error variant (Tobin C. Harding)
6c9d9d9c36 Improve error display imlps (Tobin C. Harding)
22c7aa8808 Rename non standard sighash error type (Tobin C. Harding)
Pull request description:
EDIT: The commit hashes below are stale but the text is valid still.
In an effort to "perfect" our error handling, overhaul the error handling in the `crypto` module.
The aim is to do a small chunk so we can bikeshed on it then I can apply the learnings to the rest of the codebase.
Its all pretty trivial except:
- commit `4c180277 Put NonStandardSighashTypeError inside ecdsa::Error variant`
- comimt `5a196535 Add InvalidSighashTypeError`
- commit `05772ade Use defensive documentation`
Particularly the last one might be incorrect/controversial.
Also, please take the time to check the overall state of error code in the `crypto` module on this branch in case there is anything else we want to do.
Thanks
ACKs for top commit:
apoelstra:
ACK 3c0bb63423
Tree-SHA512: 7e5f8590aec5826098d4d8d33351a41b10c42b6379ff86e5b889e73271b71921fc3ca9525baa5da53e07fa2e961e710393694e04658a8243799950b4604caf43
Improve the script hash types by doing:
- Define the types in the `crypto::script` module
- Put the From impls directly below the type definitions
Keep the current crate level re-export so this does not impact the
public API _if_ people are using the re-export but is still a breaking
change.
Improve the pubkey hash types by doing:
- Define the types in the `crypto::key` module
- Add From<&PublicKey> impl for `PubkeyHash`
Keep the current crate level re-export so this does not impact the
public API _if_ people are using the re-export but is still a breaking
change.
Calculating the absolute fee from a fee rate can currently be achieved
by creating a `Weight` object and using
`FeeRate::checked_mul_by_weight`. This is kind of hard to discover, we
can add two public convenience functions that make discovery of the
functionality easier.
Add two functions for calculating the absolute fee by multiplying by
weight units (`Weight`) and virtual bytes (by first converting to weight
units).
The `network` module deals with data types and logic related to
internetworking bitcoind nodes, this is commonly referred to as the p2p
layer.
Rename the `network` module to `p2p` and fix all the paths.
07041d584d Apply rustfmt (The rustfmt Tyranny)
dada6d65b7 script: Move some inspector methods from ScriptBuf to Script (Steven Roose)
Pull request description:
Noticed that these methods belong in Script.
ACKs for top commit:
tcharding:
ACK 07041d584d
sanket1729:
ACK 07041d584d.
apoelstra:
ACK 07041d584d
Tree-SHA512: cdcbdf22f0457123205621ec2834164c4598be1e5b221cf859d60e88110b19f8c1e484e86f60653af237e9c2acbcdbe5d2b4c98ccf239924386639c4ba6222f7
As part of an ongoing effort to make our error types stable and useful
add a stand set of derives to all error types in the library.
`#[derive(Debug, Clone, PartialEq, Eq)]`
Add `Copy` if possible and the error type does not include
`#[non_exhaustive]`.
If an error type includes `io::Error` it only gets `#[derive(Debug)]`.
bb8bd16302 internals: Remove hex module (Tobin C. Harding)
2268b44911 Depend on hex-conservative (Tobin C. Harding)
db50509cd3 Add usage docs to the "core2" feature (Tobin C. Harding)
Pull request description:
Use the newly released `hex-conservative` crate, by doing the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Remove all the old hex code from `internals`
- Remove the now unused `internals::prelude`
- Fix all the import statements (makes up the bulk of the lines changes in this patch)
ACKs for top commit:
apoelstra:
ACK bb8bd16302
sanket1729:
utACK bb8bd16302
Tree-SHA512: ec83b3941cae6f32272471779f28461bb04959a3f6a126a68bbf2c748d83ff9518ff8932d9e937a6f389c10028bf3eb58c6b6d71ea066924dd7a34faaec7a087
This type was defined in the `transaction` module because it was
originally used in a function that had been deprecated in favour of
moving the logic to the `sighash` module.
We just removed the deprecated code so we can now move this type to the
`sighash` module where it is used.
We only keep deprecated code around for one release so we can now remove
code deprecated in v0.30.0
Done in preparation as we gear up for v0.31.0 release.
We have just released the `hex-conservative` crate, we can now use it.
Do the following:
- Depend on `hex-conservative` in `bitcoin` and `hashes`
- Re-export `hex-conservative` as `hex` from both crate roots.
- Remove all the old hex code from `hashes`
- Fix all the import statements (makes up the bulk of the lines changed
in this patch)
7b402e930c schemars: Add pinning docs (Tobin C. Harding)
0848ab7e25 Fix clippy warnings for embedded build (Tobin C. Harding)
5b1443a91c hashes/embedded: Add script dir and README (Tobin C. Harding)
94732aecbf Add patch section to test crates (Tobin C. Harding)
512d982275 Remove path field from internals dependency (Tobin C. Harding)
Pull request description:
Do a bunch of infrastructure fixes that either are needed for adding additional crate deps (hex) or updating deps (internals, hashes), or just make the tests more maintainable.
ACKs for top commit:
apoelstra:
ACK 7b402e930c
sanket1729:
ACK 7b402e930c
Tree-SHA512: 9349bb20225363914acc774cca672a23e6562fb02aea644777c558074d5eeb65289d68a93b5be59a93e9b32167e2494f6599caedc8a0d9cfbee2f94d406edbfc
e30c492faf witness: clean up Debug implementation (Andrew Poelstra)
Pull request description:
The previous code seems to have been rebased/iterated on too many times, and had room for significant simplification. By inlining the indentation logic we can eliminate 40 LOC and also clean up the output by removing trailing spaces.
Fixes#1937
It is not good form to add unit tests for debug output but you can test this locally with the patch
```
diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs
index d0b7408c..a2c38af0 100644
--- a/bitcoin/src/blockdata/witness.rs
+++ b/bitcoin/src/blockdata/witness.rs
@@ -619,6 +619,9 @@ mod test {
"304402207c800d698f4b0298c5aac830b822f011bb02df41eb114ade9a6702f364d5e39c0220366900d2a60cab903e77ef7dd415d46509b1f78ac78906e3296f495aa1b1b54101")
];
assert_eq!(witness.to_vec(), expected_witness);
+
+ println!("{:?}", witness);
+ panic!();
}
#[test]
```
And by sticking `{:#?}` in there to see the alternate output.
ACKs for top commit:
tcharding:
tACK e30c492faf
RCasatta:
ACK e30c492faf
Tree-SHA512: 0ec07885f5c75f3f34965852cf5b42b63290295d1f56e9fef7d5b3610b8ac8d318cbf8f184da5b8a9ed5b352bb2c0402797b41714cb9d5488e93c2e290340c2a
9787ba6c96 Rename Script::empty to Script::new (Tobin C. Harding)
Pull request description:
The `empty` constructor is mis-named for the following reasons:
- Non-uniform with `ScriptBuf::new`
- Non-standard with respect to stdlib which uses `Path::new` and `PathBuf::new` (on which we based the `Scritp`/`ScriptBuf`)
Rename the function to `new`, put it at the top of the impl block while we are at it.
ACKs for top commit:
apoelstra:
ACK 9787ba6c96
RCasatta:
ACK 9787ba6c96
Tree-SHA512: 2dee0f61fa9097a48369a0df802ebf238b00ad3e9ed520fbf31affa1cb2a1820cbb910b525be63513e4586acb2aa0b593cecddcad0b6cd894cdac0ba7fcf0871
Pull all the code that depends on `bitcoinconsensus` out into a separate
module `consensus::validation`.
Leave transaction testing of bitcoinconsensus code in the transaction
module.
There is not need to return the general `script::Error` from the
transaction verify functions. We can better describe the error path by
returning a custom error.
There is no need no nest the `bitcoinconsensus::Error` type within the
`script::Error`, it is the only error type returned by the verify
functions so just return it directly.
The previous code seems to have been rebased/iterated on too many times,
and had room for significant simplification. By inlining the indentation
logic we can eliminate 40 LOC and also clean up the output by removing
trailing spaces.
Currently the test `hex` macro is only available when the `test`
compiler configuration option is set but we are using it in benches
code, this works for use because `cargo bench` sets `test` for the
current crate, however it breaks downstream crates.
Fix: #1830
d45dbef3e7 Manually implement Debug on Witness (Tobin C. Harding)
Pull request description:
The current derived debug implementation on `Witness` prints the content field as an array of integers. We can do better than this by manually implementing `Debug`.
With this applied `Witness` is printed as follows: (first line is `{:?}` and the next is `{:#?}`):
Using `{:?}`:
```
Witness: { indices: 3, indices_start: 8, witnesses: [[0x00], [0x02, 0x03], [0x04, 0x05]] }
```
Using `{:#?}`:
```
Witness: {
indices: 3,
indices_start: 8,
witnesses: [
[0x00],
[0x02, 0x03],
[0x04, 0x05],
],
}
```
ACKs for top commit:
sanket1729:
tested ACK d45dbef3e7. This would be helpful for debugging downstream.
apoelstra:
ACK d45dbef3e7
Tree-SHA512: eacf4fa8e3f38c4e9ddc45de78afb8eab5b5b196b77a6612f61860e0e4e7ba96de2e7f434b92816e0b00532e73c05378cafc046ec9c34108e9d9216fb36c524a
Add rustdocs to `WitnessProgram` commenting on why we carry the witness
version number around with the witness program. This is mainly a dev
comment but it helps document the invariants so make it a rustdoc
comment.
From BIP 141:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that
> consists of a 1-byte push opcode (for 0 to 16) followed by a data push
> between 2 and 40 bytes gets a new special meaning. The value of the
> first push is called the "version byte". The following byte vector
> pushed is called the "witness program".
`WitnessVersion` and `WitnessProgram` are scriptPubkey concerns and
scriptPubkey is basically synonymous with address so in one way it makes
sense that these types are in `address` however we are in the process of
overhauling the `Address` (and `AddressInner`) types so lets move the
witness stuff to `script` and put it in individual sub-modules.
This move helps simplify the address error type also.
Note please, there are a bunch of formatting changes in here in the
error type that I cannot explain and could not remove.
The current derived debug implementation on `Witness` prints the content
field as an array of integers. We can do better than this by manually
implementing `Debug`.
With this applied `Witness` is printed as follows: (first line is `{:?}`
and the next is `{:#?}`):
Using `{:?}`:
```
Witness: { indices: 3, indices_start: 8, witnesses: [[0x00], [0x02, 0x03], [0x04, 0x05]] }
```
Using `{:#?}`:
```
Witness: {
indices: 3,
indices_start: 8,
witnesses: [
[0x00],
[0x02, 0x03],
[0x04, 0x05],
],
}
```
The `empty` constructor is mis-named for the following reasons:
- Non-uniform with `ScriptBuf::new`
- Non-standard with respect to stdlib which uses `Path::new` and
`PathBuf::new` (on which we based the `Scritp`/`ScriptBuf`)
Rename the function to `new`, put it at the top of the impl block while
we are at it.
6a18997e3c Removed only available in 1.46.0 line (TATHAGATA ROY)
Pull request description:
Fix: #1850
Removed "*Important: only available in Rust 1.46+*" on the file transaction.rs from lines 1288 and 1407 respectively.
ACKs for top commit:
Kixunil:
ACK 6a18997e3c
apoelstra:
ACK 6a18997e3c
tcharding:
ACK 6a18997e3c
sanket1729:
ACK 6a18997e3c
Tree-SHA512: 1395384ffe301b628687cc6d154e191b6a4415acd33eb4209065c5bf94115c3210ea1d28f7d7186e41665b39b5bebae849c3fa5394786ce24bdcd57b765cdbd3
0f74eb6876 Remove the unused crate::Error (Tobin C. Harding)
74154c2294 Add block::ValidationError (Tobin C. Harding)
3a9b5526b3 Move BlockHash From impls (Tobin C. Harding)
Pull request description:
Remove the `crate::Error` and replace its usage with `block::ValidationError`.
ACKs for top commit:
apoelstra:
ACK 0f74eb6876
Kixunil:
ACK 0f74eb6876
Tree-SHA512: 80b2c98d3d8f7c3f060c8ea2d94e1ebe118c07d0dcf91f6d13aed00df2cb0b15bf5e295ec0976d88d81e029cf7d3e8e4a1fe70120db57e49bbd8dd229291836b
Add a `ValidationError` to the `block` module and remove the two
variants out of `crate::Error`.
This error is only used by the `validate_pow` function, a specific error
better serves our purposes.
013dffa65d tests: Use script hash methods (Tobin C. Harding)
Pull request description:
The `ScriptBuf` type can be serialized using it's `to_bytes` function. Do not use the `psbt::Serialize` trait to do so in test code.
No logic changes, since the impl of `psbt::Serialize` for `ScriptBuf` just calls `to_bytes`.
ACKs for top commit:
Kixunil:
ACK 013dffa65d
apoelstra:
ACK 013dffa65d
Tree-SHA512: 08959e065f1528f2ca69c12d5e34bceea3ddd17eefcee45094f071b3fa7357dbf289f6fe54d18fea02eecd1d0a7cd04598bbf014a5f10676387dbe27bb490395
We have methods to convert a script to a `WScritpHash` and `ScriptHash`,
no need to do this manually, let alone use the `psbt::Serialize` trait
to do so.
These constants had an error that they had `script_size` set to 0 which
was incorrect because it's not length of the script but serialized size.
Rather than just bumping the value this uses the `from_slice` method
which is less error-prone.
This also deletes a useless test of the constants.
Closes#1834
dff757d7db Comment predict_weight (yancy)
Pull request description:
I've been reading over the `predict_weight` function since it is one of the biggest challenges for coin-selection. IE choosing inputs and constructing an optimal selection strategy requires predicting the weight to get the best selection. It's great this work has been done but there are some things I don't understand well enough to comment.
1) why are we looking at the size of VarInt struct here
> let script_size = script_len + VarInt(script_len as u64).len()
2) [predict_weight_internal](36500b4451/bitcoin/src/blockdata/transaction.rs (L1245)) has a bunch of magic numbers. I'd like to be able to comment this as well but I don't fully understand that function.
Also, `Transaction.rs` is a big file and it seems like all of the prediction stuff could be moved to a separate module or maybe a separate crate?
ACKs for top commit:
tcharding:
ACK dff757d7db
Kixunil:
ACK dff757d7db
Tree-SHA512: 8ffa16d500075d691528ce1819b9352a148af431889bebbd7cddcf470bd4e3048ec53a5e778bc3659e33d8c25b68422a93dac1d46b9489ff56f41d88d7f05433
Our previous MSRV did not support MIN/MAX associated consts so we had
methods min/max_value. Now that our MSRV is Rust 1.48.0 we can use the
consts.
Deprecate min/max_value methods in favor of MIN/MAX associated conts.
We currently use the functions `min_value` and `max_value` because the
consts were not available in Rust 1.41.1, however we recently bumped the
MSRV so we can use the consts now.
1c3bbd4bf2 internals: Remove attribution from all files (Tobin C. Harding)
99673ab5c4 hashes: Introduce SPDX license identifiers (Tobin C. Harding)
984fe69448 bitcoin: Remove attribution from all files (Tobin C. Harding)
Pull request description:
Please note, whether or not we need a per-file license comment is out of scope for this PR. This PR leaves us with the most simple per-file solution possible and leaves the merit of per-file license comment to be discussed on another day.
Simplify the per-file license stuff by doing:
- Remove the attribution line from each file.
Currently we have a mishmash of attribution lines accompanying the SPDX identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
- Introduce SPDX license identifiers into `hashes` and remove attribution line (ie, make `hashes` uniform with `bitcoin`)
Required before merge please:
- [x] ack from apoelstra because as the library original author many of the changes in this PR remove his name
- [x] ack from Kixunil because he had some concerns in the issue descussion
Fix: #1816
ACKs for top commit:
Kixunil:
ACK 1c3bbd4bf2
sanket1729:
ACK 1c3bbd4bf2
apoelstra:
ACK 1c3bbd4bf2
Tree-SHA512: c5ac05c5eb23b3b6a760f707c344b22f5871a4dedee4990b1840f57e4cee1d38560ff4507c354bbf29bc8ff05a179d95d7e100fcf19bd93c5362344a352c7b5a
Currently we have a mishmash of attribution lines accompanying the SPDX
identifier. These lines are basically meaningless because:
- The date is often wrong
- The original author attributed is not the only contributor to a file
- The term "rust bitcoin developers" is basically just noise
Just remove all the attribution lines and be done with it. While we are
at it add an SPDX line to the few files missing it, whether this license
nonsense is even needed is left as an argument for another day.
dd4ad9444e Hardcode expected weight in txin_txout_weight_tests (Peter Todd)
Pull request description:
Rational: the expected weight is fixed so this both ensures we don't accidentally change it somehow, and makes it easier to re-use these test cases in other codebases (eg python-bitcoinlib).
ACKs for top commit:
apoelstra:
ACK dd4ad9444e
tcharding:
ACK dd4ad9444e
Kixunil:
ACK dd4ad9444e
Tree-SHA512: 4769a4bb8695f4f4c95e258bb5f06a232090b14c3d9159d6d5de2d09d7fc934a1b920b90cc09677a88fc0cf37ac21ed27794692dff2c73df4252c9551dc10fc2
Rational: the expected weight is fixed so this both ensures we don't
accidentally change it somehow, and makes it easier to re-use these test
cases in other codebases (eg python-bitcoinlib).
a54e1ceab1 Apply rustfmt (The rustfmt Tyranny)
38d11ce3da ci: Make release CI search for NEXT.RELEASE instead (Steven Roose)
dad3abd20f transaction: Rename is_coin_base to is_coinbase (Steven Roose)
Pull request description:
Alternative to https://github.com/rust-bitcoin/rust-bitcoin/pull/1795.
Keep the old method as deprecated and add doc alias. Also change internal usage of the method.
ACKs for top commit:
tcharding:
ACK a54e1ceab1
apoelstra:
ACK a54e1ceab1
Tree-SHA512: 52d9729bf83da164556d960f8867cb836ff57a0f619da3dd3620efffb28a974aac23b8085863ab0e072a4bdb2f13ac576efa43ad2eec9a271ad044227f4d00a4
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
We've upgraded MSRV but didn't update clippy config, so some things that
could be improved aren't caught by clippy. This updates the config and
fixes the new issues.
I also `rg '1\.41\.1'`ed for interesting changes and found one
additional improvement.
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
This improves readability of converting `BlockHash` into `ChainHash`.
It's useful in e.g. Electrum protocol which sends `BlockHash`
(serialized backward).
Closes#1751
122188f7dd Use shorter import statements (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Use shorter import statements
As per discussion [0] use the shorter form for importing crates that we
re-export (`hashes` and `secp256k1`).
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1661
ACKs for top commit:
apoelstra:
ACK 122188f7dd
sanket1729:
utACK 122188f7dd
Tree-SHA512: 3f540464d38c72ba9d68f8ceda8600540bd0c3eef0ba67531c87fa1e0e4f757af7035cf80a1a5f17aa05604a17fdd9ef59bb6bece6b4145d540dac1e5362fc01
1dc04fe10f Remove rust_v_1_46 (Tobin C. Harding)
71fa9e81e7 Bump MSRV to 1.48.1 (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Bump MSRV to 1.48.1
As per discussion [0] bump our MSRV for all crates in `rust-bitcoin`
repo to 1.48.1 [1].
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1329
[1] https://blog.rust-lang.org/2020/11/19/Rust-1.48.html
ACKs for top commit:
apoelstra:
ACK 1dc04fe10f
sanket1729:
ACK 1dc04fe10f
Tree-SHA512: 533470c55f7aeede88382db8245033dac9317d75b38ced2ad8256d38319632a524335f893044e96300a1e60048f9aaca2d1634735eb324eb8ed9ad3c9ab2f872
dbd2ea07b5 Add kilo weight unit conversion (yancy)
Pull request description:
The FeeRate module defaults to sats per `kwu` so when doing fee calculations, it would be convenient to easily convert weight to the same units.
ACKs for top commit:
Kixunil:
ACK dbd2ea07b5
tcharding:
ACK dbd2ea07b5
apoelstra:
ACK dbd2ea07b5
Tree-SHA512: fe26b631cf474f1dca627a4d21e9052c80f8ada9a0eb635c46b7f8f42095671b9b161fb5012b723699008372faf7668507d5e92bde7d1808d389f85dba33529b
We just merged a patch to enable formatting in CI but commit: `05fdead2
Feature: Add difficulty_float method for block::Header.` must have
slipped in.
Run the formatter.
913575ac91 hashes: Run the formatter (Tobin C. Harding)
52c4579057 Enable formatting for hashes (Tobin C. Harding)
3f16b6bf9f util: Run the formatter (Tobin C. Harding)
d210d2ac83 Enable formatting for util (Tobin C. Harding)
5973dce9db blockdata: Run the formatter (Tobin C. Harding)
0dcbed3c7b Enable formatting for blockdata (Tobin C. Harding)
a52746d01c psbt: Run the formatter (Tobin C. Harding)
ef306db5e2 Enable formatting for psbt (Tobin C. Harding)
296f2ed82c Make test panic instead of using code comment (Tobin C. Harding)
3ec8a12428 crypto: Run the formatter (Tobin C. Harding)
c8a3c58786 Enable formatting for crypto (Tobin C. Harding)
314e6786b4 crypto: Add rustfmt::skip attributes (Tobin C. Harding)
450a84f6e8 consensus: Run the formatter (Tobin C. Harding)
89143205f9 Enable formatting for consensus (Tobin C. Harding)
ce773af20f tests: Remove useless use of super imports (Tobin C. Harding)
ef01f4d0f6 consensus: Introduce local variables (Tobin C. Harding)
Pull request description:
One final push crew, 16 patches, only a few are big.
All non-trivial formatting is done in separate patches so the changes can be verified mechanically.
With this applied the whole `rust-bitcoin` crate will be formatted.
Big thanks to everyone for putting up with the ongoing formatting PRs, no-one likes doing these but hopefully this an improvement to the project - especially in helping us get more contributors to the project.
ACKs for top commit:
tcharding:
> ACK [913575a](913575ac91). Went through the workflow locally.
sanket1729:
ACK 913575ac91. Went through the workflow locally.
apoelstra:
ACK 913575ac91
Tree-SHA512: b30eaa2893563155de05f8fa97be4a24a7dd8bf43bb426314c5104598477ca2173af279da796da8b18cc53a0ed525908b3d4edd0504836a443465efa0773632d
Add `rustfmt::skip` attribute in a couple of places and then remove the
exclude for the `blockdata` module. Do not run the formatter, that will
be done as a separate patch to aid review.
Currently we have a code comment that is supposed to assist devs in
maintaining the `network::constants::Network` type by failing to build
if a new variant is added. This plays havoc with the formatter because
the comment is hanging at the bottom of a match block and the formatting
thinks its for the proceeding line of code.
Instead of using a code comment add a panic so the unit test fails if a
new variant is added to `network::constants::Network`.
00b46d6d9d Indent functions (Martin Habovstiak)
d56d202aeb Support weight prediction in `const` context (Martin Habovstiak)
Pull request description:
**Notes for reviewers:**
This is something that I want to use in my code and hopefully reasonably easy to review, so if this can get into 0.30 that'd be really nice. No hard feelings if it doesn't.
I tried to put extra effort into making review easier by:
* intentionally "mis-formatting" the first commit so diff is smaller and easy to understand - see individual commits.
* copying patterns from non-const fn to const fn so it's obviously correct (includes same variable names)
* not bothering with the array trick in `VarInt::len` and simply accepting the limitation of Rust 1.46+ (I use 1.48 BTW).
**Description**
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
ACKs for top commit:
apoelstra:
ACK 00b46d6d9d
tcharding:
ACK 00b46d6d9d
Tree-SHA512: 5509886a68b4de5227db0e28d92a40be8de64592e0b189c519213db21bcfe98ca03d9a1936b1024729b97db69e8ec0b55fac870a7ce9bab0d0c9a47b2087990f
Some smart contracts or simplified wallets statically know the sizes of
transactions or inputs. The possible approaches to handling them so far
were re-computing the values (and hoping the optimizer will const fold
them) or using a simple constant which may be harder to understand and
get right. It's much nicer to just use a `const` but our code didn't
support it until now.
This change adds methods that can compute the prediction in `const`
context for Rust versions >= 1.46.0 which allow use of loops (and
conditions but those could be workaround anyway).
As a side effect of this, the change also adds `const` to `VarInt::len`
in Rust 1.46+. While this one could be made unconditional using array
trick it's probably not worth it because of the planned MSRV bump.
Note: this commit is intentionally unformatted to make diff easier to
understand. Formatting will be done in future commit.
e3f95ee22b Add tests for the FeeRate type (yancy)
Pull request description:
Adds some tests for the `FeeRate` type.
ACKs for top commit:
apoelstra:
ACK e3f95ee22b
tcharding:
ACK e3f95ee22b
Kixunil:
ACK e3f95ee22b
Tree-SHA512: 74d6597c747d5aa62a6510bf9fa8de971f89ad56f571aadd496f6487e80cc88bb2b5a1c6bcfed825d09d18ca2310b2bfd6fdbe330f2760369d167a653d26bef8
There are several common spends in Bitcoin that have known input weight
predictions. It can be useful to have these as constants, so this change
adds them. However, this only adds native segwit ones as the others are
slowly fading away and might clutter the API.
42b07586ac Improve the public API (Tobin C. Harding)
Pull request description:
We created the `crypto` crate as a container for cryptography modules with the idea that it may be split out into a separate crate. There is no reason for users of the lib to know about this module. Also, we have two `taproot` modules, one in `crypto` and one at the crate root, this makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from `crate::taproot`
Fix: #1668
ACKs for top commit:
apoelstra:
ACK 42b07586ac
Kixunil:
ACK 42b07586ac
Tree-SHA512: 5713b98b2a48d2776cdd6ca2c0e798d6e2df604d780e5182bd770fb2df807cf1f65bc24663ee6f7734fc1e0c80494c5a87dc60e44c83acefcffec7f059203a47
We created the `crypto` crate as a container for cryptography modules
with the idea that it may be split out into a separate crate. There is
no reason for users of the lib to know about this module. Also, we have
two `taproot` modules, one in `crypto` and one at the crate root, this
makes for un-ergonomic usage of the lib.
Improve the public API by doing:
- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from
`crate::taproot`
7d1645aea0 Add constant for coinbase maturity (benthecarman)
Pull request description:
Not sure if this is the best place to put this but it is nice to have a constant for this instead of having other libraries make their own (ie https://github.com/lightningdevkit/rust-lightning/pull/1924#pullrequestreview-1222807626)
ACKs for top commit:
tcharding:
ACK 7d1645aea0
apoelstra:
ACK 7d1645aea0
Tree-SHA512: 5ac2a3359cadd303158c66ba45db8f4bf8cc80b6c19604262999ff361fd0bd98e2a4851c57da1962cb5c74f5789a85c8b3861f1742706a60ce1fbc57c3c200cc
Currently we have an associated type on hash types `Inner` with
accompanying methods `into_inner`, `from_inner`, `as_inner`. Also, we
provide a way to create new wrapped hash types. The use of 'inner'
becomes ambiguous with the addition of wrapped types because the inner
could be the inner hash type or the `Inner` byte array of the inner
wrapped hash type.
In an effort to make the API more clear and uniform do the following:
- Rename `Inner` -> `Bytes`
- Rename `*_inner` -> `*_byte_array`
- Rename the inner hash to/from methods to `*_raw_hash`
Correct method prefix `into_` -> `to_` because theses methods convert
owned `Copy` types.
Add the trait Bound `Copy` to the `Bytes` type because we rely on this
trait bound for the conversion methods to be correctly named according
to convention.
Because of the dependency hole created by `secp256k1` this patch changes
the secp dependency to a git tag dependency that includes changes to the
hashes calls required so that we can get green lights on CI in this
repo.
090dad770f Improve string parsing (Tobin C. Harding)
Pull request description:
Currently we implement string parsing for height/time from the `absolute` module but not the `relative` module.
Improve the macros used to implement string parsing and use the new versions to implement string parsing for the height and time types in `relative`.
Done while reviewing data structures in relation to `serde`.
ACKs for top commit:
apoelstra:
ACK 090dad770f
Kixunil:
ACK 090dad770f
Tree-SHA512: bfa88efbaf5dc35755eb46df373a08e223f112860e8a65f58db9fdd77e2c01dc9377da735b33ef58940004fe5fe11690ac09be19591fded2c9fd04cd7d2bdf73
438ee45691 Show cache construction in rustdoc (Tobin C. Harding)
Pull request description:
To make it more clear what the cache is show the cache construction line in rustdoc.
ACKs for top commit:
apoelstra:
ACK 438ee45691
Kixunil:
ACK 438ee45691
Tree-SHA512: d6da6bad57fddf9e2f4bcfb7c9b87df38bf4b2bb914e92e52d5ae8afa3405a9793536d7164223021ab6d183ddde732cf6889370834e36f37bae470127b0271fa
It may not be obvious why the condition in `push_bytes` module checks
for negation of 16 and 32 bit architectures rather than 64 bit. This
adds a comment about it being conservative.
Currently we have `TapSighash` that is used for taproot sighashes but
for non-taproot sighashes we use `hash_types::Sighash`. We can improve
the API by creating a `LegacySighash`, and `SegwitV0Sighash`.
Copy the original `Sighash` macro calls to create the two new types in
the `sighash` module.
While we are at it, put the `TapSighash` and `TapSighashTag` into the
`sighash` module also.
Script parsing is composed of several functions which implicitly rely on
various properties. Adding a type that restricts the valid values makes
local review easier.
So far we deserialized hex into `Vec<u8>` at run time. This was mainly
in tests where it had negligible performance cost. However moving the
computation to compile time has a few benefits: it allows proving the
length of the decoded bytes and identifies potential typos before the
code goes through LLVM and other compilation machinery which makes
feedback faster.
This change uses the `hex_lit` crate to move computation to compile
time. It is implemented as `const` declarative macro which doesn't blow
up compilation time.
An absolute lock time of 100 is nonsensical because we are well past
block 100. This value was used because it makes sense for _relative_
locktimes but for absolute lock times it makes the examples and tests
slightly confusing.
272cdbcf7c Flatten the types directory (Tobin C. Harding)
Pull request description:
We recently created a `types` subdirectory under `script` to keep all the `Script` and `ScriptBuf` impls together. Turns out this additional level of subdirectory is a bit annoying and we can achieve the same grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two submodules up a level, remove the `types` directory.
Fix: #1640
ACKs for top commit:
Kixunil:
ACK 272cdbcf7c
apoelstra:
ACK 272cdbcf7c
Tree-SHA512: 91fd78084829fa24f3b6420602d7d5094670647fff43e6e193d6de3126f1657132873ea133540d87db7d0d4dfc4cb9666489e39c861377085ce0254da81fd564
ae2aaaa436 Add `script_pubkey_lens` method (Martin Habovstiak)
cf068d16b0 Implement transaction weight prediction (Martin Habovstiak)
Pull request description:
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
Based on #1627
ACKs for top commit:
apoelstra:
ACK ae2aaaa436
tcharding:
ACK ae2aaaa436
Tree-SHA512: 55376601c2c2826bb0909cc25ff5b65816f0b1a2d57fb2cd8831f3db5382de0f4a364d518b312f0528bb5f44c30f3f74f8d254145eed2bfd65e2332b7c4d7c8b
We recently created a `types` subdirectory under `script` to keep all
the `Script` and `ScriptBuf` impls together. Turns out this additional
level of subdirectory is a bit annoying and we can achieve the same
grouping by just using `script/mod.rs`.
Move code from `types/mod.rs` to `script/mod.rs`, move the two
submodules up a level, remove the `types` directory.
In some cases people construct the transaction with a dummy fee output
value before calculating the weight. A method to create the iterator
over `script_pubkey` lengths is useful in such cases.
In some scenarios it's useful to create outputs with minimal relayable
value. E.g. outputs designated for fee bumping using CPFP. A method for
this is useful.
This implements a constructor of `TxOut` that computes the minimal
non-dust value from the passed script.
Closes#1459
When creating a transaction one must know the the fee beforehand to set
appropriate amounts for outputs and to know the fee, weight is required.
So far we only had a method on an already-constructed transaction. This
method clearly wasn't helpful when constructing the transaction except
for hacks like temporarily adding an all-zeroes signature.
This change adds a function that can compute the transaction weight
without knowing individual bytes of the scripts, witnesses and other
elements. It only needs to know their sizes.
To make the API less error-prone a special, trivial, type is also added
for computing the lengths of witnesses.
a7117bf8f1 Document source of logic fro read_scriptint (Tobin C. Harding)
2eb2420b40 Add comment on rountripping read/write scripint (Tobin C. Harding)
657dd51e8b Use OP_0 to better mimic bitcoin core code (Tobin C. Harding)
31d254a6a8 Fix push operators URL (Tobin C. Harding)
84cd4ca964 Deprecate script::read_uint (Tobin C. Harding)
Pull request description:
Patch one does the deprecation, the rest of the PR is made up of tiny improvements to the code around reading/writing 'scriptint's (conceptually `CScriptNum`s). I did all this while trying to decipher the discussion on #1547.
### Note Please
There are many more changes in the pipeline for all this read/write "script int" stuff. This PR was done ages ago and I believe it stall adds value.
I re-did the whole PR manually because of the recent `script` module changes. I hope no one else has to do that - if you do please feel free to holla and I'll "rebase" your PR for you.
ACKs for top commit:
Kixunil:
ACK a7117bf8f1
apoelstra:
ACK a7117bf8f1
Tree-SHA512: 5e8ee7fa8d1393a1a50e4241dd947b837cc0ddd15ff1239a49e4839489459fb95d184d6773f73633d55c436310bfab0c73f806d492ed4a4215f924c6c0993936
Currently we implement string parsing for height/time from the
`absolute` module but not the `relative` module.
Improve the macros used to implement string parsing and use the new
versions to implement string parsing for the height and time types in
`relative`.
Lock times are u32 and can necessitate encoding using 5 bytes. As such
they are "special".
Add methods `push_lock_time` and `push_sequence` for pushing absolute
lock times and sequence numbers. We do not push relative locktimes
because they are only 16 bits from the original sequence number.
Our `script::read_scriptint` function is based on the constructor
code (incl. call to `set_vch`) code from Bitcoin Core. Add rustdoc
comment saying so, emit a link because there are already multiple links
to `script.h` in this file (one just right below the added comment).
We only support reads of upto 4 bytes where as Bitcoin Core allows
reading a `CScriptNum` with more bytes than that. Add a rustdoc
comment (incl. link to Bitcoin Core) mentioning that.
Our `Builder::push_int` method is the same as Bitcoin Core `CScript`
`push_int64` method. We currently use `OP_FALSE` (equivalent to `OP_0`)
but recently we added `OP_0`, lets use it to make our code better mimic
Core (also saves devs checking that `OP_FALSE` is the same as `OP_0`).
70cf4515db Add `Weight` and `FeeRate` newtypes (Martin Habovstiak)
Pull request description:
Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion.
Part of #630
Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.)
ACKs for top commit:
apoelstra:
ACK 70cf4515db
tcharding:
ACK 70cf4515db
Tree-SHA512: ab9cc9f554a52ab0109ff23565b3e2cb2d3f609b557457b4afd8763e3e1b418aecbb3d22733e33304e858ecf900904a1af6e6fdc16dc21483b9ef84f56f103b2
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.
This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.
Part of #630
The `script` module is large and unwieldy.
Refactor the `script` module, splitting it up into a tree of modules.
Here are a few of the changes and their stated benefits
- Split the two script types out into separate files: Readers of the
methods can then tell immediately from the file name which type they are
reading.
- Put all the impls for the two script types together: Makes parsing the
API easier because one can more quickly see which traits are implemented
on what i.e., all the `AsRef` imlps are grouped together.
- Put the impls for the two script types in order, first `Script` then
`ScriptBuf`: Makes it easier for us to see if we missed something.
- Put the `Builder` and `Instruction` (and associated) types in their
own modules: Some devs find long files hard to navigate, so far there
hasn't been too much push back against short files.
- Put tests in a separate file: This idea was recently discussed.
This is only moving code and fixing import statements etc. No other
changes to the code.
bb612fdafa Set rustv_1_53 in build script (Tobin C. Harding)
Pull request description:
The rust version is supposed to be set by the build script so that users automagically get features matching the toolchain in use. Currently we have a feature in the manifest for `rustv_1_53` instead setting a compiler conditional configuration option in the build script. This causes `cargo +1.41.1 --all-features check` to fail.
## Note
I don't see `rustv_1_46` used anywhere, do we need that still?
ACKs for top commit:
apoelstra:
ACK bb612fdafa
Kixunil:
ACK bb612fdafa
Tree-SHA512: f74195d4ee5a5bc5f209e99d30789df3552cef10aee5ea8b61a5a701b753999c34d04be9fe0321ccee7a8ec14fa5a05e0b454b9dc5f8deddd7b5b8d4f3d7e744
Currently we use a wildcard to export all the hash types in
`hash_types`. We are moving to a world were we only export
normal/standard types from the crate root.
Remove the reexport of the following hash types:
- `FilterHash`
- `FilterHeader`
- `TxMerkleNode`
- `WitnessCommitment`
- `WitnessMerkleNode`
- `XpubIdentifier`
- `Sighash`
Fix: #1541
The rust version is supposed to be set by the build script so that users
automagically get features matching the toolchain in use. Currently we
have a feature in the manifest for `rustv_1_53` instead setting a
compiler conditional configuration option in the build script. This
causes `cargo +1.41.1 --all-features check` to fail.
Remove `FromHex` from hash and script types
- Remove the `FromHex` implementation from hash types and `ScriptBuf`
- Remove the `FromStr` implementation from `ScriptBuf` because it does not
roundtrip with `Display`.
- Implement a method `from_hex` on `ScriptBuf`.
- Implement `FromStr` on hash types using a fixed size array.
This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
ed6f6d11dd Implement fmt traits for ScriptBuf (Tobin C. Harding)
Pull request description:
We can improve ergonomics of the `script` module by implementing the `fmt` traits on `ScriptBuf`, trivial because we can call through to the `Script` implementations.
Fix: #1585
ACKs for top commit:
Kixunil:
ACK ed6f6d11dd
apoelstra:
ACK ed6f6d11dd
Tree-SHA512: 878a1522af4ed1e10d1d8d60d150e6571008c008b5e5c662c67462f9e09075b4f1fe4e399ed50e98cd7253b6815937c6732cd1ce02b74a5be017d5b8fcdbbd2f
We can improve ergonomics of the `script` module by implementing the
`fmt` traits on `ScriptBuf`, trivial because we can call through to the
`Script` implementations.
The `max_value` and `min_value` functions only exist to be
compatible/uniform with Rust 1.41.1 they will never change and they just
return a constant value. They can therefore be made const functions.
Some of the lock time structs (`Height`, `Time` ect.) are missing
standard constants for min, max ect.
Add standard constants taking into consideration the various locktime
corner cases.
Add `max_value` and `min_value` to be consistent with Rust 1.41.1 (incl.
`Sequence`).
Fix: #1451
a762a89b48 Add documentation to Sequence::is_final (Tobin C. Harding)
b1490a26ea Move enables_absolute_lock_time method (Tobin C. Harding)
Pull request description:
The term "final" is an archaic Bitcoin term however it is well used, it exists in Bitcoin Core code as well as in various bips. To help folks new to Bitcoin add documentation to the `is_final` method including historical notes.
Note, this does _not_ deprecate `is_final` - while writing the notes I found the term "final" in enough official places that I think its fair game to keep the term, some things people just have to learn, we can definitely help with that learning though.
Fix: #1198
ACKs for top commit:
Kixunil:
ACK a762a89b48
apoelstra:
ACK a762a89b48
Tree-SHA512: 895fbdce90223d90c0a68fb1e3d6b7aada4a3606d1294ea4df1f4194681a79d970b0434e7bb078f6d5cbf413b3550e72560d6d5cf811a5a959adf53f7f778ab2
Tobin C. Harding
Clark Moody
Andrew Poelstra
junderw
Subhradeep Chakraborty
yancy
Riccardo Casatta
Daniela Brozzoni
Steven Roose
yancy
TATHAGATA ROY
Martin Habovstiak
Peter Todd
The rustfmt Tyranny
Salvatore Ingala
benthecarman
Harshil Jani