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
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
8c0e5213d3 Delegate debug for ScriptBuf to Script (Tobin C. Harding)
Pull request description:
Currently the derived implementation of `Debug` for `ScriptBuf` prints the inner vector of u8s as integers, this is ugly and hard to read. The `Script` implementation of `Debug` prints the script opcodes and data as hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
ACKs for top commit:
Kixunil:
ACK 8c0e5213d3
apoelstra:
ACK 8c0e5213d3
Tree-SHA512: ca07d9fb191f4e0379cbd96b2944e6881094a8334d39b97209b6bf452a3c15d4aede53b9c88176b9b7667b7a539d47897940bc561dc9f8cd83ce1990a08047e1
Addresses with Segwitv0 not having len 20/32 are invalid and cannot be
constructed. Also cleans up a API bug in
ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.
Currently the derived implementation of `Debug` for `ScriptBuf` prints
the inner vector of u8s as integers, this is ugly and hard to read. The
`Script` implementation of `Debug` prints the script opcodes and data as
hex, we can just delegate to it.
With this applied we get debug output of form:
Script(OP_DUP OP_HASH160 OP_PUSHBYTES_20 \
3bde42dbee7e4dbe6a21b2d50ce2f0167faa8159 OP_EQUALVERIFY OP_CHECKSIG)
Fix: #1516
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.
In preparation for deprecating the `is_final` method; move the
`enables_absolute_lock_time` method to be directly above the `is_final`
method.
Refactor only, no logic changes.
70fe07f1ce Export the DisplayHex trait from within prelude (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 70fe07f1ce
apoelstra:
ACK 70fe07f1ce
Tree-SHA512: 96a89135cb0b829b7b5926a3b344f78e178b5b48e772a69da5133fab6d2e14e7b7bbaa56b7a417a5c1a64337546a1c7bac32307d3a1f27aa199ed61f590902bf
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.
94b678e73f Rename `push_scriptint` and make it private (Martin Habovstiak)
Pull request description:
`push_scriptint` is a significant footgun with an unclear name. This renames it and unpublishes to avoid mistakes by downstream crates.
Closes#1517
ACKs for top commit:
apoelstra:
ACK 94b678e73f
Tree-SHA512: 9e1772c6fb326d8b0c78d702ad9926a79a91589feb8650aed7c5e75bfbdbf0164357b4d5b190877c40b8469e0e3be3d3453fe407741b5dae0c5758176f756417
089a1e452d Replace `Vec::from_hex` with `hex!` (Martin Habovstiak)
Pull request description:
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
Note that while the change appears big it's nearly entirely mechanical, so should be pretty easy to review. (But I don't feel it's `trivial`.)
ACKs for top commit:
apoelstra:
ACK 089a1e452d
tcharding:
ACK 089a1e452d
Tree-SHA512: c55357b8cffc86f8e107c0f8390490ede75948260018f63dc926455700cbcaf422f6ae72444f93943065e6f4ffa4335bee9160a64184ea4f8e91721f30a46ace
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
The additional `use` items were added to improve the style of
documentation. Because they were only used for doc they were `cfg`ed.
But because this is independent from being built by `docs.rs` the `cfg`
should've been `doc` not `docsrs`.
IOW `docsrs` means roughly `all(doc, nightly)` and the added items are
unrelated to `nightly`.
Martin Habovstiak
Tobin C. Harding
Andrew Poelstra
sanket1729
bnabi