While I believe the original commit used 80 bytes for the entire script as the limit,
Bitcoin Core as of [this commit](7a172c76d2/src/policy/policy.h)
will relay OP_RETURN outputs as long as the data itself is not above 80 bytes, meaning a script of maximum size 83 bytes should be standard.
As we have been doing in other places deprecate `to_bytes` in favour of
`to_vec`. Note this is only for functions that return a `Vec`, the `key`
module has `CompressedKey::to_bytes` still as it returns an array.
The `Script::fmt_asm` function is a legacy from days yore before
`Display` printed asm. We no longer need it.
Deprecate `Script::fmt_asm` and use the private `bytes_to_asm_fmt` or
`Display` impls.
Done in preparation for moving the script types to `primitives`.
The script types have a bunch of functionality to support scriptPubkeys,
and scriptPubkeys are an address thing.
Create a module under `address` and in it create a bunch of extension
traits to hold all scriptPubkey functionality.
Includes adding an ugly-as-hell macro to create the traits.
This is a continuation of the previous commit, but separated to make
review a little easier. This one replaces test vectors that were
previously computed by hashing garbage into Txids and various other hash
types with new test vectors which are directly-computed garbage
converted to hashes with from_byte_array.
In one case (src/hash_types.rs) this results in changing a bunch of
fixed test vectors. This is okay; this test is supposed to check the
direction of string serialization, which is unaffected by this commit
(or any commit in this PR). The existing test vectors, because they hash
the empty string, result in different bytes depending on the underlying
hash algo (sha256, sha256d, sha256t, etc). The new ones just use the
same fixed test vector for all of them.
This commit also updates a doctest in crypto/sighash.rs which
demonstrates how to manually feed sighash data into a hash engine and
correctly handle the sighash single bug. Because you can no longer
directly get a sighash object from an engine, this particular example
should maybe be rewritten to just encode to a Vec rather than a hash
engine, explaining that maybe you'd do this when implementing a HWW, to
verify the exact data being hashed. Or something.
Unrelatedly, you can check that there are no API changes in this commit
or the last several. The next commit will remove GeneralHash impls and
that's when you'll see changes.
There are two limits that the Bitcoin network enforces in regard to
hashing scripts
- For P2SH the redeem script must be less than 520 bytes
- For P2WSH the witness script must be less than 10,000 bytes
Currently we are only enforcing the p2sh limit when creating an address
with `Address::p2sh`.
There are various ways to create addresses from script hashes and if
users manually hash a script then use the `ScriptHash` (or
`WScritpHash`) our APIs assume the script that was hashed is valid. This
means there is the potential for users to get burned by creating
addresses that cannot be spent, something we would like to avoid.
- Add fallible constructors to `ScriptHash` and `WScriptHash`
- Add `TryFrom` impls as well to both types
- Remove the `From` impls
The script in a p2wsh is typically referred to as the witness script not
the redeem script - rename the local test variable to follow suit.
Refactor only, no logic changes.
This test does an odd combination of function calls, its not obvious
what it is supposed to be testing. The `to_p2wsh.is_p2wsh` is already
tested above. The leading `to_p2sh` does not prove anything, one can put
currently pass any script to `to_p2wsh` so this tests nothing.
In preparation for patching the script hashing functionality first
remove this odd test.
9f01871c11 api: Run just check-api (Tobin C. Harding)
7929b51640 Pass keys by value (Tobin C. Harding)
Pull request description:
We should pass `Copy` types by value not by reference. Pass the key types by value.
This is patch 1 from #2404
ACKs for top commit:
apoelstra:
ACK 9f01871c11 this will annoy some people but I think we should do it
Tree-SHA512: 18afab537edf4ade4dc1c1e5992e50060b8935531f1e3cbe1d3b94b2fcb87aafa39947f342e0e762835bda3b4091dd35b3b74ea79f4dbb3b21660ffd21d1f82e
Currently we have a trait `Hash` that is required for `Hmac`, `Hkdf`,
and other use cases. However, it is unegonomic for users who just want
to do a simple hash to have to import the trait.
Add inherent functions to all hash types including those created with
the new wrapper type macros.
This patch introduces some duplicate code but we are trying to make
progress in the hashes API re-write. We can come back and de-dublicate
later.
Includes making `to_byte_array`,`from_byte_array`, `as_byte_array`, and
`all_zeros` const where easily possible.
TxOut::minimal_non_dust has 3 problems.
1. There is an invisible dependency on Bitcoin Core's default minrelaytxfee value. It has been made explicit.
2. There is an off by one error. The dust limit comparison uses < and therefore `+ 1` was not needed. It has been fixed.
3. It was not returning 0 amount for OP_RETURN outputs.
Script::dust_value has 2 problems.
1. The dust amount depends on minrelaytxfee which is configurable in Bitcoin Core. This method was not configurable.
2. The division operation was done before multiplying the byte amount, which can cause small differences when using uncommon scripts and minrelaytxfee values.
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
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
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`.
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.
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.
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.
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.
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.
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.
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.
Rob N
yancy
Tobin C. Harding
Chris Hyunhum Cho
Fmt Bot
Jamil Lambert, PhD
Ryan Breen
Andrew Poelstra
Divyansh Gupta
Sh0g0-1758
GoodDaisy
Jonathan Underwood
Clark Moody
Martin Habovstiak