We have a bunch of methods that use the prefix `get_`, they are not
exactly getters because they do more than just access a struct fields so
Rust convention relating to getters does not apply, however, the `get_`
prefix does not add to the descriptiveness of name hence the shorter
form can be used with no loss of clarity.
Improve docs and deprecate any methods changed that are pubic.
d1abfd9c30 Add unit test for sighash single bug (Tobin Harding)
82f29b4267 Use 1 signature hash for invalid SIGHASH_SINGLE (Tobin Harding)
3831816a73 Move test helper function (Tobin Harding)
3e21295b88 Remove unnecessary whitespace character (Tobin Harding)
Pull request description:
Fix up the logic that handles correctly returning the special array 1,0,0,...,0 for signature hash when the sighash single bug is exploitable i.e., when signing a transaction with SIGHASH_SINGLE for an input index that does not have a corresponding transaction output of the same index.
- Patch 1 and 2: Clean up
- Patch 3: Implements the fix
- Patch 4: Adds a passing test that fails if moved to before patch 3
Resolves: #817
ACKs for top commit:
apoelstra:
ACK d1abfd9c30
dr-orlovsky:
ACK d1abfd9c30
Tree-SHA512: f2d09e929d2f91348ae0b0758b3d4be6c6ce0cb38c4988e0bebb29f5918ca8491b9e7b31fe745f7c20d9348612fe2166f0a12b782f256aad5f6b6c027c2218b7
In this library we specifically do not use rustfmt and tend to favour
terse statements that do not use extra lines unnecessarily. In order to
help new devs understand the style modify code that seems to use an
unnecessary number of lines.
None of these changes should reduce the readability of the code.
Vector initialisation uses neither "Block" nor "Visual" stlye, this is
irregular for no added benefit.
Elect to use "Block" style (as defined by `rustfmt`).
Our usage of `where` statements is not uniform, nor is it inline with
the typical layout suggested by `rustfmt`.
Make an effort to be more uniform with usage of `where` statements.
However, explicitly do _not_ do every usage since sometimes our usage
favours terseness (all on a single line).
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
When signing a transaction will result in the sighash single bug being
exploitable we should return the 'one array' (equivalent to 1 as a
uint256) as the signature hash.
Add a unit test to verify we return uint256 1 value when use of
SIGHASH_SINGLE is invalid.
When signing a transaction will result in the sighash single bug being
exploitable we should return the 1 array (equivalent to 1 as a uint256)
as the signature hash.
Currently we are using the correct array value but are re-hashing it,
instead we should directly return it.
Improve the rustdocs for the `blockdata::transaction` module:
- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording/grammar
- Use backticks in links
- Use 100 character column width if it improves readability
Nothing too controversial here :)
The line of code `let mut have_witness = self.input.is_empty();` is
puzzling if one does not know _why_ we serialize in BIP141 style when
there are no inputs.
Add a code comment to save devs spending time trying to work out _why_
this is correct.
The two methods `encode_signing_data_to` and `signature_hash` use the
same docs (one is a public helper for the other). The docs have gotten a
bit stale (refer to deprecated types).
Instead of duplicating all the text, add a statement pointing readers
from the docs of `signature_hash` to the docs on
`encode_signing_data_to`.
Rust idiomatic style is to put the rustdoc _above_ any attributes on
types, functions, etc.
Audit the codebase and move comments/attributes to the correct place.
Add a trailing full stop at times to neaten things up a little extra.
106acdc3ac Add fuzzing for Witness struct (Riccardo Casatta)
2fd0125bfa Introduce Witness struct mainly to improve ser/de performance while keeping most usability. (Riccardo Casatta)
Pull request description:
At the moment the Witness struct is `Vec<Vec<u8>>`, the vec inside a vec cause a lot of allocations, specifically:
- empty witness -> 1 allocation, while an empty vec doesn't allocate, the outer vec is not empty
- witness with n elements -> n+1 allocations
The proposed Witness struct contains the serialized format of the witness. This reduces the allocations to:
- empty witness -> 0 allocations
- witness with n elements -> 1 allocation for most common cases (you don't know how many bytes is long the entire witness beforehand, thus you need to estimate a good value, not too big to avoid wasting space and not too low to avoid vector reallocation, I used 128 since it covers about 80% of cases on mainnet)
The inconvenience is having slightly less comfortable access to the witness, but the iterator is efficient (no allocations) and you can always collect the iteration to have a Vec of slices. If you collect the iteration you end up doing allocation anyway, but the rationale is that it is an operation you need to do rarely while ser/de is done much more often.
I had to add a bigger block to better see the improvement (ae860247e191e2136d7c87382f78c96e0908d700), these are the results of the benches on my machine:
```
RCasatta/master_with_block
test blockdata::block::benches::bench_block_deserialize ... bench: 5,496,821 ns/iter (+/- 298,859)
test blockdata::block::benches::bench_block_serialize ... bench: 437,389 ns/iter (+/- 31,576)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 108,759 ns/iter (+/- 5,807)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 670 ns/iter (+/- 49)
test blockdata::transaction::benches::bench_transaction_get_size ... bench: 7 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 51 ns/iter (+/- 5)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 13 ns/iter (+/- 0)
branch witness_with_block (this one)
test blockdata::block::benches::bench_block_deserialize ... bench: 4,302,788 ns/iter (+/- 424,806)
test blockdata::block::benches::bench_block_serialize ... bench: 366,493 ns/iter (+/- 42,216)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 84,646 ns/iter (+/- 7,366)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 648 ns/iter (+/- 77)
test blockdata::transaction::benches::bench_transaction_get_size ... bench: 7 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 50 ns/iter (+/- 5)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 14 ns/iter (+/- 0)
```
With an increased performance to deserialize a block of about 21% and to serialize a block of about 16% (seems even higher than expected, need to do more tests to confirm, I'll appreciate tests results from reviewers)
ACKs for top commit:
apoelstra:
ACK 106acdc3ac
sanket1729:
ACK 106acdc3ac
dr-orlovsky:
utACK 106acdc3ac
Tree-SHA512: e4f23bdd55075c7ea788bc55846fd9e30f9cb76d5847cb259bddbf72523857715b0d4dbac505be3dfb9d4b1bcae289384ab39885b4887e188f8f1c06caf4049a
Witness struct is in place of the Vec<Vec<u8>> we have before this commit.
from_vec() and to_vec() methods are provided to switch between this type and Vec<Vec<u8>>
Moreover, implementation of Default, Iterator and others allows to have similar behaviour but
using a single Vec prevent many allocations during deserialization which in turns results in
better performance, even 20% better perfomance on recent block.
last() and second_to_last() allows to access respective element without going through costly Vec
transformation
826fed53f2 transactions: add a note about `get_vsize` and standardness rules (Antoine Poinsot)
Pull request description:
If they ever hit a discrepancy they must really be doing something dodgy but hey :)
ACKs for top commit:
dr-orlovsky:
ACK 826fed53f2
Tree-SHA512: c618a80b047797625a233939d2c1146e8b4ce44215648841813f78178577afc844f5e561e4e60b4084e315735894ecb354af8d81f4702f5354e5d5cd05b52ac4
This is the initial step towards using and maybe enforcing clippy.
It does not fix all lints as some are not applicable. They may be
explicitly ignored later.
Calls to `unwrap` outside of tests are typically unfavourable.
Hash engines do not error when calling `consensus_encode`. Instead of
the current usage of `unwrap` we can use `expect` with a descriptive
string as is done in other parts of the codebase.
Docs can always do with a bit of love.
Clean up the module level (`//!`) rustdocs for all public modules.
I claim uniform is better than any specific method/style. I tried to fit
in with what ever was either most sane of most prevalent, therefore
attaining uniformity without unnecessary code churn (one exception being
the changes to headings described below).
Notes:
* Headings - use heading as a regular sentence for all modules e.g.,
```
//! Bitcoin network messages.
```
as opposed to
```
//! # Bitcoin Network Messages
```
It was not clear which style to use so I picked a 'random' mature
project and copied their style.
* Added 'This module' in _most_ places as the start of the module
description, however I was not religious about this one.
* Fixed line length if necessary since most of our code seems to follow
short (80 char) line lengths for comments anyways.
* Added periods and fixed obvious (and sometimes not so obvious)
grammatically errors.
* Added a trailing `//!` to every block since this was almost universal
already. I don't really like this one but I'm guessing it is Andrew's
preferred style since its on the copyright notices as well.
c704ee7ffe [docs-only] Use backtick in addition to square parentheses for types references, clarify legacy, non_exhaustive comment, remove std:: (Riccardo Casatta)
f223be618f Rename access_witness to witness_mut and return Option (Riccardo Casatta)
c9bc0b928a [fmt-only] autoformatting with `rustfmt src/util/sighash.rs` (Riccardo Casatta)
07774917c2 Use get_or_insert_with in segwit_cache (Martin Habovstiak)
497dbfb7c3 Use get_or_insert_with in common_cache() (Martin Habovstiak)
ca80a5a030 Use get_or_insert_with in taproot_cache (Martin Habovstiak)
6e06a32ccc Wrap ErrorKind in Io enum variant, fix doc comment for the IO variant (Riccardo Casatta)
1a2b54ff23 introduce constant KEY_VERSION_0 (Riccardo Casatta)
417cfe31e3 Derive common traits for structs and enum, make internal struct not pub (Riccardo Casatta)
55ce3dd6ae Fix validation error if SINGLE with missing corresponding output, remove check_index and check with get().ok_or(), more details in errors (Riccardo Casatta)
2b3b22f559 impl Encodable for Annex to avoid allocation (Riccardo Casatta)
1a7afed068 Add Reserved variant to SigHashType for future use (ie SIGHASH_ANYPREVOUT) (Riccardo Casatta)
53d0e176d3 Deprecate bip143::SigHashCache in favor of sighash::SigHashCache (Riccardo Casatta)
15e3caf62d [test] Test also sighash legacy API with legacy tests (Riccardo Casatta)
24acfe3672 Implement Bip341 signature hash, create unified SigHashCache for taproot, segwit and legacy inputs (Riccardo Casatta)
683b9c14ff add [En|De]codable trait for sha256::Hash (Riccardo Casatta)
Pull request description:
Adds https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki message signature algorithm
The base is taken from `bip143::SigHashCache`, some code results duplicated but I think it's more clear to keep things separated
Would mark some bullet point on https://github.com/rust-bitcoin/rust-bitcoin/issues/503
Test vectors are taken by running d1e4c56309/test/functional/feature_taproot.py with a modified `TaprootSignatureHash` function to print intermediate values that I cannot found in the bip341 [test vector json](https://raw.githubusercontent.com/bitcoin-core/qa-assets/main/unit_test_data/script_assets_test.json)
UPDATE: Latest version includes the suggestion from @sanket1729 to create a unified tool for signature message hash for legacy, segwit, and taproot inputs. In particular, makes sense for mixed segwit v0 and taproot v1 inputs because cached values could be shared
ACKs for top commit:
sanket1729:
ACK c704ee7ffe. Reviewed the diff from a37de1ade475e0c31c932121abaa7aec701b9987 which I previously ACKed
dr-orlovsky:
utACK c704ee7ffe by diffing it to 6e06a32ccc having my ACK before.
apoelstra:
ACK c704ee7ffe
Tree-SHA512: 35530995fe9d078acd0178cfca654ca980109f4502c91d578c1a0d5c6cafacab7db1ffd6216288eac99f6a763776cbc0298cfbdff00b5a83e98ec4b15aa764e8
This documents cargo features in two ways: explictly in text and in code
using `#[doc(cfg(...))]` attribute where possible. Notably, this is
impossible for `serde` derives. The attribute is contitional and only
activated for docs.rs or explicit local builds.
This change also adds `package.metadata.docs.rs` field to `Cargo.toml`
which instructs docs.rs to build with relevant features and with
`docsrs` config activated enabling `#[doc(cfg(...))] attributes.
I also took the opportunity to fix a few missing spaces in nearby code.
Based on the original work by Justin Moon.
*MSRV unchanged from 1.29.0.*
When `std` is off, `no-std` must be on, and we use the [`alloc`](https://doc.rust-lang.org/alloc/) and core2 crates. The `alloc` crate requires the user define a global allocator.
* Import from `core` and `alloc` instead of `std`
* `alloc` only used if `no-std` is on
* Create `std` feature
* Create `no-std` feature which adds a core2 dependency to polyfill `std::io` features. This is an experimental feature and should be
used with caution.
* CI runs tests `no-std`
* MSRV for `no-std` is 1.51 or so
Tobin Harding
Dr. Maxim Orlovsky
sanket1729
Riccardo Casatta
Martin Habovstiak
Antoine Poinsot
Andrew Poelstra
Devrandom
Vis Virial
Collins Muriuki