We have a convention in `rust-bitcoin` to use external crates directly
when importing them not via `crate::foo`.
Update all the import paths for `io` to use this form.
c34e3cc7cc Re-write size/weight API (Tobin C. Harding)
73f7fbf520 Add code comments to transaction serialization (Tobin C. Harding)
29f20c1d0b Add segwit serialization constants (Tobin C. Harding)
Pull request description:
Audit and re-write the weight/size API for `Block` and `Transaction`. First two patches are trivial, patch 3 contains justification and explanation for this work, copied here:
```
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
```
Please note, this PR introduces panics if a sciptPubkey overflows the calculation `weight = spk.size() * 4`.
Fix#2049
ACKs for top commit:
apoelstra:
ACK c34e3cc7cc
sanket1729:
ACK c34e3cc7cc.
Tree-SHA512: 4944f652e6e362a282a5731140a9438a82d243a4c646b4627d9046a9f9cf13c476881750d432cfbc6b5fe5de1f0c4c9c44ed4569dac4bc11b55a5db28793803c
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.
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
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.
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
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)
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.
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],
],
}
```
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.
We use `internals::hex::display::DisplayHex` in many places, we can
improve ergonomics of the `internals` crate by re-exporting it from the
`prelude` module.
The `ToHex` trait was replaced by either simple `Display`/`LowerHex`
where appropriate or `DisplayHex` from `bitcoin_internals` which is
faster.
This change replaces the usages and removes the trait.
This makes the code less noisy and is a preparation for changing it to
`const`-based literal. Because of the preparation, places that used
variables to store the hex string were changed to constants.
There are still some instances of `Vec::from_hex` left - where they
won't be changeable to `const` and where `hex!` is unavailable
(integration tests). These may be dealt with later.
See also #1189
Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.
Improve the content of various rustdoc comments in the `blockdata`
module.
Suggested content came from reviewers, all mistakes are my own :)
We are trying to flatten the `util` module. The `taproot` module can
live in the crate root. If/when we create a `crypto` module/crate we may
wish to pull some stuff out of this module but for now moving it gets us
closer to removing `util` without making the directory structure any
worse.
Includes adding rustfmt attributes to skip formatting of macros.
d78a996bf6 Add `Witness::from_slice()` and depreciate `Witness:from_vec()` (Noah Lanson)
d5bdf5d225 Add non-generic `Witness::push_slice()` method (Noah Lanson)
Pull request description:
Cleanup PR to improve the `Witness` API by:
- Adding `Witness::from_slice()` and depreciating `Witness::from_vec()` methods (#1371).
- Making `Witness::push()` not generic and take in `&[u8]` instead of `AsRef<[u8]>` (#1372).
Note: `Witness::from_vec()` has been marked for depreciation from `0.30.0`. Let me know if this should be different.
ACKs for top commit:
tcharding:
ACK d78a996bf6
apoelstra:
ACK d78a996bf6
Tree-SHA512: 3a0b11b1ea77966a773cf7c9e9853822192897eac495fc0a23068bad3b0c46714fc839b20ceeb6e076aa10ea8ff0c023dfc418feff2f892cf11e8c057e5b0c7d
00c7b6e06f Witness: Fix nits from PR 1323 (junderw)
Pull request description:
Ref: #1323
This is just to quickly fix some of the smaller nits. Larger changes (deprecations, adding / refactoring of methods) should be in a separate PR.
ACKs for top commit:
Kixunil:
ACK 00c7b6e06f
tcharding:
ACK 00c7b6e06f
sanket1729:
ACK 00c7b6e06f
Tree-SHA512: 5f661187a7003060669d15d873e323c017c905a00b62eb56ca3afc2fc27084b245ad62dfcf6d2fd14eac361430be954e7636f6b9ff668aefaad0424789a2f826
3c0d5aed73 Add get_tapscript to Witness (junderw)
4226d60205 Add Index<usize> and nth(index) to Witness (junderw)
Pull request description:
Ref: https://github.com/rust-bitcoin/rust-bitcoin/pull/672#issuecomment-980636502
[Add Index<usize> and nth(index) to Witness](4226d60205)
[4226d60](4226d60205)
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
---
[Add get_tapscript to Witness](a7501d9599)
[a7501d9](a7501d9599)
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
---
Edit: This is the previous PR body:
> In a taproot script payment with annex, quick access to the 3rd to last element (which is the actual script in this case) is convenient.
>
> This feels like kicking the can down the road again, but I think it's a nice to have method.
>
> RCasatta dr-orlovsky were discussing this issue. I would like to ask if they have any thoughts on the addition of this.
ACKs for top commit:
tcharding:
ACK 3c0d5aed73
apoelstra:
ACK 3c0d5aed73
Kixunil:
ACK 3c0d5aed73
Tree-SHA512: 0038eed6ad56786b8dd6d98db0d1846753b8b25de0bc1089cdc75d5850d0ccc66dde9a10be7fe09589ad7db118fd50ee9f7993695968df5c389457ccfcdaa761
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
It is considered idiomatic for types that have `iter()` method to also
implement `IntoIterator` for their references. `Witness` was missing
this so it is added here.
Done as part of the effort to flatten the `util` module.
The `sighash` module can stand alone in the crate root, it provides a
discreet set of functionality - the `SighashCache` and associated types.
Create a directory `bitcoin` and move into it the following as is with
no code changes:
- src
- Cargo.toml
- contrib
- test_data
- examples
Then do:
- Add a workspace to the repository root directory.
- Add the newly created `bitcoin` crate to the workspace.
- Exclude `fuzz` and `embedded` crates from the workspace.
- Add a contrib/test.sh script that runs contrib/test.sh in each
sub-crate
- Fix the bitcoin/contrib/test.sh script
Tobin C. Harding
Andrew Poelstra
Riccardo Casatta
Martin Habovstiak
Jiri Jakes
Casey Rodarmor
Noah Lanson
sanket1729
junderw