Currently we calculate the parity during `tap_tweak` but do not return
it, this means others must re-do work done inside `tap_tweak` in order
to calculate the parity. We can just return the parity along with the
tweaked key.
Keeping inline with the method on `UntweakedPublicKey` that outputs a
`TweakedPublicKey` we can use the same name, for the same reasons.
Use `dangerous_assume_tweaked` as the constructor name to highlight the
fact that this constructor should probably not be being used.
We currently run `tweak_add_check` and use the result as a conditional
branch, the error path of which uses `unreachable`. This usage of
`unreachable` is non-typical. An 'unreachable' statement is by
definition supposed to be unreachable, it is not clear why we would need
to have a conditional branch to check an unreachable statement.
Use `debug_assert!` so programmer errors get caught in un-optimised
builds but in optimised builds the call to `tweak_add_check` is not even
done.
We have two types for tweaked/untweaked schnorr public keys to help
users of the taproot API not mix these two keys up. Currently the
`taproot` module uses 'raw' `schnoor::PublicKey`s.
Use the `schnoor` module's tweak/untweaked public key types for the
`taproot` API.
The merkle_root of an empty tree is undefined, this is the only error
case we have for the two `bitcoin_merkle_root*` functions. We can fully
describe this error case by returning an `Option` if args are found to
be empty. We can do the same for the wrapper functions in `block`
module.
While we are at it, refactor out a recursive helper function to make
reading the code between the two functions easier.
04a8f89f05 Implement `FusedIterator` for `Instructions` (Martin Habovstiak)
Pull request description:
`Instructions` guarantee to return `None` from empty iterator so we
should signal this in type system so that the code can be optimized
better. This also adds a test to make sure this property holds.
ACKs for top commit:
sanket1729:
utACK 04a8f89f05. Any special reasons for doing this?
RCasatta:
ACK 04a8f89f05
Tree-SHA512: 3c6284e97e3bdd28ac5e948e3e9946eb8aa285cba753a6a0bdcbf971ebceab6d93c206d284128c232531b3de5996ece91187e4369d88bdfe6c531b4b7f787dd8
5b21a9cb1f Use TapTweakHash method for computing tweak (Noah)
Pull request description:
Quick follow up PR to #691 using a method from #677.
### Changes
- Updated `UntweakedPublicKey::tap_tweak(...)` to use `TapTweakHash::from_key_and_tweak(...)`
ACKs for top commit:
Kixunil:
ACK 5b21a9cb1f
dr-orlovsky:
utACK 5b21a9cb1f
Tree-SHA512: d00455bba51981e9ec942a6cf69672666e227850d073b1fdcd92d2eb6ad553659fb2967aec2ce12d3ed109cee5fa125cdda649cddb25404f08adae2bfd3e19bb
`Instructions` guarantee to return `None` from empty iterator so we
should signal this in type system so that the code can be optimized
better. This also adds a test to make sure this property holds.
e7b84e20d3 Use expect for concensus_encode on Vec (Tobin Harding)
4031fbf4ba Use expect for concensus_encode on sinks (Tobin Harding)
fa513bb5b5 Use expect for concensus_encode on engines (Tobin Harding)
a2efafcf9a Use error instead of err (Tobin Harding)
Pull request description:
Calls to `unwrap` outside of tests are generally unfavourable. We currently call `unwrap` in a bunch of places on calls to `consensus_encode` when passing writers that do not fail.
Remove `unwrap` calls on all calls to `consensus_encode` that pass a writer argument for which write functions do not fail. Use `expect` with a descriptive string instead.
Fixes: #714
ACKs for top commit:
Kixunil:
ACK e7b84e20d3
RCasatta:
ACK e7b84e20d3
Tree-SHA512: 3f84598a14ecf3dcde4f418ad1a1dc5278b3ef8b2604f4e9fc4cf4e9aed8390a4a1cf0df47edb5956cc5b667d6c8864e34621c0dae974ea75d6daf1b133165dd
Calls to `unwrap` outside of tests are typically unfavourable.
In memory writers (`Vec`) do not error. We can use `expect` with a
descriptive message string to indicate this.
Calls to `unwrap` outside of tests are typically unfavourable.
Sink writers do not error. We can use `expect` with a descriptive
message string to indicate this.
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.
In the name of uniformity use the same error message as argument to
`expect` througout the codebase.
Use "engines don't error" instead of "engines don't err".
f2a6827982 Fix BinaryHeap direction for Taproot Huffman Encoder (Jeremy Rubin)
cccd75d004 Fix Weighting Addition to never error on overflow + prevent overflows from ever happening with wider integers (Jeremy Rubin)
Pull request description:
I noticed one cleanup & one bugfix while looking into the huffman algorithm:
1) the cleanup: we can use a u128 to guarantee no overflows, and saturating_add to guarantee reasonable behavior in any case
2) the bug: the binary heap is a max heap so the behavior ends up merging the nodes of the most likely entries repeatedly. a huffman encoder requires merging the least likely elements, so it should be reversed.
ACKs for top commit:
sanket1729:
ACK f2a6827982
dr-orlovsky:
utACK f2a6827982
Tree-SHA512: 07cadb8dd5cc2b7e6ae3ebc2c1639de054e41bcd7f3b7d338a93e77fd200c9591a89915aaae5d9f5313eff3d94032fdfe06d89fda1e2398881b711d149e9afe9
822c99222d Improve constructor rustdocs for Address (Tobin Harding)
804a38cb67 Improve documentation of `WitnessVersion` (Tobin Harding)
eb8278fd2e util/address: Improve docs (Tobin Harding)
Pull request description:
Improve documentation of the `address` module by doing:
- Add full stops to all sentences
- Use code ticks even inside links e.g., [`WitnessVersion`]
- Use 100 character line length
- Do grammar fixes
- Use comment sections (e.g. `# Returns`)
- Use 3rd person for function comments e.g. 'Converts foo to bar' instead of 'Convert foo to bar'
- Use ticks for scriptPubkey
This patch does a single file because a bunch of these changes pick an
arbitrary stlye, if we can bikeshed on this PR then future PRs should be
able to progress more quickly. I'll take lack of comment on any of the
above as approval and I'll attempt to be uniform when doing the rest of
the codebase. I plan on just chipping away at this, I can only do so
much docs work in a day without getting bored of it :)
Notes:
- I didn't touch 'segwit' vs 'SegWit', seems both are widely used.
- Using ticks inside links may be an overkill but seems more correct?
- I'm not totally sure where the line is in the Rust ecosystem between
readability in an editor and rendering as HTML, open to input on this.
ACKs for top commit:
Kixunil:
ACK 822c99222d
dr-orlovsky:
ACK 822c99222d
Tree-SHA512: bfbaeec74803dd0704ed3e39b9a4966db34dbb3d7ea850ed6230abf220b877687ac1479f4940b7bf39d7e8172cd62c36b232bfaa8186a92cc58b3d7e642674f6
e4774e74eb fixups to taptweaking code (sanket1729)
Pull request description:
This was my bad for not clearly stating the expected spec #687 . Changed values to references so that we only take ownership where it is required.
This should simplify the #697
ACKs for top commit:
Kixunil:
ACK e4774e74eb
dr-orlovsky:
utACK e4774e74eb
Tree-SHA512: adacbfa8a77f46b2c85720f3760ed12a437f40d8422731d0207662d7947c95dda79d576923f6056c77f57977a3dcd25afd270f0ee11e9c3be9d067ccdc63371a
We test `bitcoin_merkle_root` over in the `blockdata::block` module.
Although the `bitcoin_merkle_root` and `bitcoin_merkle_root_inline`
functions are almost identical there is enough index manipulation done
that it is not immediately obvious that the code is error free.
Add a unit test that verifies that the two functions return the same
resulting merkle root.
Improve the rustdocs for the various `Address` constructors by putting
the brief description on a separate line with further description in its
own paragraph. This is the layout best practice for function documentation
using rustdocs.
Also, favour 'creates' over 'constructs' because it is more common in
the docs of this struct.
Improve documentation of the `address` module by doing:
- Add full stops to all sentences
- Use code ticks even inside links e.g., [`WitnessVersion`]
- Use 100 character line length
- Do grammar fixes
- Use comment sections (e.g. `# Returns`)
- Use 3rd person for function comments e.g. 'Converts foo to bar' instead of 'Convert foo to bar'
- Use ticks for scriptPubkey
This patch does a single file because a bunch of these changes pick an
arbitrary stlye, if we can bikeshed on this PR then future PRs should be
able to progress more quickly. I'll take lack of comment on any of the
above as approval and I'll attempt to be uniform when doing the rest of
the codebase. I plan on just chipping away at this, I can only do so
much docs work in a day without getting bored of it :)
Notes:
- I didn't touch 'segwit' vs 'SegWit', seems both are widely used.
- Using ticks inside links may be an overkill but seems more correct?
- I'm not totally sure where the line is in the Rust ecosystem between
readability in an editor and rendering as HTML, open to input on this.
0af5a433b6 Return the correct `LeafVersion` when building a Taproot `ControlBlock` (Alekos Filini)
Pull request description:
ACKs for top commit:
sanket1729:
ACK 0af5a433b6
Tree-SHA512: 6b887e86b32b070a2a42ba1a2309b094c36d5a0b0bbf7d4c49c4fd2d8d2b4a7b1d87da699f1bd5f7116926c590413609a292d900b55c27c6bdbadc408529999f
0d463ec19e tests: improve coverage for P2tr and AddressType (Leonardo Comandini)
Pull request description:
The new AddressType test shows addresses that are valid but have
no type. If in the future some of those get a type or become
invalid (either voluntarily or due to a regression), this will
highlight it.
ACKs for top commit:
dr-orlovsky:
utACK 0d463ec19e
sanket1729:
ACK 0d463ec19e
Tree-SHA512: 9e062a1807173638cb62a61a2e8ea5be8324449a8944c356073e8bd9f53941dea369c65a35dfa0019bd8323eaa5dd26a9907c1823522fef9a524e919728973a6
The new AddressType test shows addresses that are valid but have
no type. If in the future some of those get a type or become
invalid (either voluntarily or due to a regression), this will
highlight it.
55c627715f Moving globals into PSBT struct (Dr Maxim Orlovsky)
Pull request description:
I took the most non-invasive approach to reduce diff size. Many parts of the code can be improved in style or further refactored (like some functions are not necessary and can be just moved to be part of other functions), but I'd prefer to do that as a separate PR once this will be merged.
My approach with this PR:
1. Remove `Global` struct by moving its fields right into `PartiallySignedTransaction` - but keep the `util/psbt/map/global.rs` file with all its logic
2. Keep existing `Map for Global` implementation in the same file, but just change it to `Map for PartiallySignedTransaction`
3. With serialization, convert `Global` deserialization into crate-private function and use it from `PartiallySignedTransaction` deserialization
4. Refactor the tests and imports as required to get the thing compile and pass tests
The refactoring will be followed by PR(s) adding support for Taproot
ACKs for top commit:
apoelstra:
ACK 55c627715f
sanket1729:
ACK 55c627715f . Reviewed range diff with ac0c908 that I previously ACKed
Tree-SHA512: 79b329b6e4e60af905e4e00507d6abc558261d921bcf8f5d4ee34dd685322d7a529b18015423da50a388ba6732b7b662a92bc95ad078228cc809254ad010d467
Ambiguous TweakedPublicKey and UntweakedPublicKey type aliases and methods to convert
Use structs for Untweaked and Tweaked key type
swap dangerous api to work on tweaked keys
remove unecessary allocations and rename methods
Use type alias for UntweakedPublicKey
TweakedPublicKey::new(...) method added
minor naming and doc changes
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.
This refactors `Script::bytes_to_asm_fmt`` function to use an iterator
instead of index. Such change makes it easier to reason about overflows
or out-of-bounds accesses. As a result this also fixes three unlikely
overflows and happens to improve formatting to not output space at the
beginning in some weird cases.
To improve robustness even better it also moves `read_uint`
implementation to internal function which returns a more specific error
type which can be exhaustively matched on to guarantee correct error
handling. Probably because of lack of this the code was previously
checking the same condition twice, the second time being unreachable and
attempting to behave differently than the first one.
Finally this uses macro to deduplicate code which differs only in single
number, ensuring the code stays in sync across all branches.
5d71a9dd89 Correct input length check for uin128 fuzzer (Matt Corallo)
9c256cc88e Add a fuzz check for `Uint128::increment` (Matt Corallo)
a15f263c4e Move the `increment` fn into the uint macro to add it to Uint128 (Matt Corallo)
d52b88b525 Fix increment of Uint256 with carry (carolcapps)
Pull request description:
This is #578 with review feedback addressed.
ACKs for top commit:
apoelstra:
ACK 5d71a9dd89
sanket1729:
ACK 5d71a9d
Tree-SHA512: 32e5ea6387943ecad8f190a0de336a545fda72b6ff7388d3479037a5f880434276a7d0607f5cf61710d45e984c01954f4e3199a60c542be48b397717afb3d406
This clearly states lack of support for 16-bit architectures as well as
adds readable `compile_error!()` call. It also fixes a few stylistic
mistakes - headings (top-level should not be repeated) and missing
newlines.
Closes#660
76cf74fa9b Added test for the overflow bug and few others (Martin Habovstiak)
a0e1d2e706 Check for overflow in Script::bytes_to_asm_fmt() (Martin Habovstiak)
Pull request description:
This adds an overflow check in `Script::bytes_to_asm_fmt()` motivated by
`electrs` issue. While it was not tested yet, I'm very confident that
overflow is the cause of panic there and even if not it can cause panic
becuase the public function takes unvalidated byte array and reads
`data_len` from it.
The `electrs` issue: https://github.com/romanz/electrs/issues/490
~~Strangely, this breaks a test case and I can't see why. I'm publishing in case someone wants to help.~~
Edit: One damn character. :D Should be OK now.
ACKs for top commit:
apoelstra:
ACK 76cf74fa9b
Tree-SHA512: 4ffeca442a71b10c132f055f056128ae64e66cbdc1891662c3a4e743b82fa5d27075a44513e844be37888b33068eef3bbf6bcced5def70c17c9c5bd5b9d870cc
This adds a test case for script formatting which caused overflow in the
past and a few others from the same "interesting" transaction. Note that
to trigger the bug one has to run the test on 32 bit architecture.
This adds an overflow check in `Script::bytes_to_asm_fmt()` motivated by
`electrs` issue. While it was not tested yet, I'm very confident that
overflow is the cause of panic there and even if not it can cause panic
becuase the public function takes unvalidated byte array and reads
`data_len` from it.
The `electrs` issue: https://github.com/romanz/electrs/issues/490
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.
This refactors the code to make it possible to use `get_or_insert_with`
instead of unwrapping in `segwit_cache()`. To achieve it `common_cache`
is refactored into two functions: one taking only the required borrows
and the original calling the new one. `segwit_cache` then calls the new
function so that borrows are OK.
Apart from removing unwrap, this avoids calling `common_cache` multiple
times.
There was a question whether this is equally performant. There are
multiple good reasons why it should be:
1. `get_or_insert_with` is marked `#[inline]`
2. Any good optimizer will inline a function that is used exactly once
3. 1 and 2 conclude that the closure will get inlined
4. Computing self.tx can then be moved to the only branch where it is
required.
5. Even if get_or_insert_with didn't get optimized, which is extremely
unlikely, the `tx` field is at the beginning of the struct and it
probably has pointer alignment (`Deref` suggests it's a pointer).
Alignment larger than pointer is not used, so we can expect the
fields to be ordered as-defined. (This is not guaranteed by Rust but
there's not good reason to change the order in this case.) We can
assume that offset to tx is zero in most cases which means no
computation is actually needed so the expression before closure is
no-op short of passing it into the closure as an argument.
At the time of writing `#[inline]` can be seen at
https://doc.rust-lang.org/src/core/option.rs.html#933
Instead of using magic numbers we can define constants for the address
prefix bytes. This makes it easier for future readers of the code to see
what these values are if they don't know them and/or see that they are
correct if they do know them.
Tobin Harding
Riccardo Casatta
Dr. Maxim Orlovsky
Martin Habovstiak
Jeremy Rubin
Noah
sanket1729
Alekos Filini
Leonardo Comandini
Marko Bencun
Noah Lanson
Andrew Poelstra
Antoine Poinsot
Clark Moody
Vikas S Shetty
Ben Carman