27b3c1e0e6 Improve the ScriptHash and WScriptHash types (Tobin C. Harding)
2197f1377f Improve PubkeyHash and WPubkeyHash (Tobin C. Harding)
Pull request description:
Total re-write since review. Now this PR moves the hash type definitions out of `hash_types`. Please see https://github.com/rust-bitcoin/rust-bitcoin/issues/1909#issuecomment-1603634440 for more.
No longer adds unit tests.
Fix: #1909
ACKs for top commit:
apoelstra:
ACK 27b3c1e0e6
Tree-SHA512: 216b9bed05d1a4a4fc493262664ceb5d60f9c30685b63d6f6675d21a7bf811053320a002165487b29599c52f345057d9c92babb0fc1ccd4628671ec468c804f9
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
Improve the script hash types by doing:
- Define the types in the `crypto::script` module
- Put the From impls directly below the type definitions
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.
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.
07041d584d Apply rustfmt (The rustfmt Tyranny)
dada6d65b7 script: Move some inspector methods from ScriptBuf to Script (Steven Roose)
Pull request description:
Noticed that these methods belong in Script.
ACKs for top commit:
tcharding:
ACK 07041d584d
sanket1729:
ACK 07041d584d.
apoelstra:
ACK 07041d584d
Tree-SHA512: cdcbdf22f0457123205621ec2834164c4598be1e5b221cf859d60e88110b19f8c1e484e86f60653af237e9c2acbcdbe5d2b4c98ccf239924386639c4ba6222f7
As part of an ongoing effort to make our error types stable and useful
add a stand set of derives to all error types in the library.
`#[derive(Debug, Clone, PartialEq, Eq)]`
Add `Copy` if possible and the error type does not include
`#[non_exhaustive]`.
If an error type includes `io::Error` it only gets `#[derive(Debug)]`.
bb8bd16302 internals: Remove hex module (Tobin C. Harding)
2268b44911 Depend on hex-conservative (Tobin C. Harding)
db50509cd3 Add usage docs to the "core2" feature (Tobin C. Harding)
Pull request description:
Use the newly released `hex-conservative` crate, by doing 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`
- Remove all the old hex code from `internals`
- Remove the now unused `internals::prelude`
- Fix all the import statements (makes up the bulk of the lines changes in this patch)
ACKs for top commit:
apoelstra:
ACK bb8bd16302
sanket1729:
utACK bb8bd16302
Tree-SHA512: ec83b3941cae6f32272471779f28461bb04959a3f6a126a68bbf2c748d83ff9518ff8932d9e937a6f389c10028bf3eb58c6b6d71ea066924dd7a34faaec7a087
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)
7b402e930c schemars: Add pinning docs (Tobin C. Harding)
0848ab7e25 Fix clippy warnings for embedded build (Tobin C. Harding)
5b1443a91c hashes/embedded: Add script dir and README (Tobin C. Harding)
94732aecbf Add patch section to test crates (Tobin C. Harding)
512d982275 Remove path field from internals dependency (Tobin C. Harding)
Pull request description:
Do a bunch of infrastructure fixes that either are needed for adding additional crate deps (hex) or updating deps (internals, hashes), or just make the tests more maintainable.
ACKs for top commit:
apoelstra:
ACK 7b402e930c
sanket1729:
ACK 7b402e930c
Tree-SHA512: 9349bb20225363914acc774cca672a23e6562fb02aea644777c558074d5eeb65289d68a93b5be59a93e9b32167e2494f6599caedc8a0d9cfbee2f94d406edbfc
9787ba6c96 Rename Script::empty to Script::new (Tobin C. Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK 9787ba6c96
RCasatta:
ACK 9787ba6c96
Tree-SHA512: 2dee0f61fa9097a48369a0df802ebf238b00ad3e9ed520fbf31affa1cb2a1820cbb910b525be63513e4586acb2aa0b593cecddcad0b6cd894cdac0ba7fcf0871
Pull all the code that depends on `bitcoinconsensus` out into a separate
module `consensus::validation`.
Leave transaction testing of bitcoinconsensus code in the transaction
module.
There is not need to return the general `script::Error` from the
transaction verify functions. We can better describe the error path by
returning a custom error.
There is no need no nest the `bitcoinconsensus::Error` type within the
`script::Error`, it is the only error type returned by the verify
functions so just return it directly.
Add rustdocs to `WitnessProgram` commenting on why we carry the witness
version number around with the witness program. This is mainly a dev
comment but it helps document the invariants so make it a rustdoc
comment.
From BIP 141:
> A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that
> consists of a 1-byte push opcode (for 0 to 16) followed by a data push
> between 2 and 40 bytes gets a new special meaning. The value of the
> first push is called the "version byte". The following byte vector
> pushed is called the "witness program".
`WitnessVersion` and `WitnessProgram` are scriptPubkey concerns and
scriptPubkey is basically synonymous with address so in one way it makes
sense that these types are in `address` however we are in the process of
overhauling the `Address` (and `AddressInner`) types so lets move the
witness stuff to `script` and put it in individual sub-modules.
This move helps simplify the address error type also.
Note please, there are a bunch of formatting changes in here in the
error type that I cannot explain and could not remove.
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.
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.
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.
fabcde036f Use package in manifest and shorten import (Tobin C. Harding)
Pull request description:
We can use `package` to rename `bitcoin_hashes` to `hashes` and `bitcoin_internals` to `internals`. This makes imports more terse with no loss of meaning.
ACKs for top commit:
apoelstra:
ACK fabcde036f
Kixunil:
ACK fabcde036f
Tree-SHA512: bc5bff6f7f6bf3b68ba1e0644a83da014081d8c6c9d578c21cb54fdd56a018f68733dd1135d05b590ba193ed9efd12fa9019182c1fed347e604d8548f6ef9103
If we use `#![cfg_attr(docsrs, feature(doc_auto_cfg))]` instead of
`#![cfg_attr(docsrs, feature(doc_cfg))]` we no longer need to manually
mark types with `#[cfg_attr(docsrs, doc(cfg(feature = "std")))]`.
Sweeeeeet.
We can use `package` to rename `bitcoin_hashes` to `hashes` and
`bitcoin_internals` to `internals`. This makes imports more terse with
no loss of meaning.
122188f7dd Use shorter import statements (Tobin C. Harding)
Pull request description:
Just patch 2, patch 1 is #1728
From the commit log of patch 2
Use shorter import statements
As per discussion [0] use the shorter form for importing crates that we
re-export (`hashes` and `secp256k1`).
[0] https://github.com/rust-bitcoin/rust-bitcoin/discussions/1661
ACKs for top commit:
apoelstra:
ACK 122188f7dd
sanket1729:
utACK 122188f7dd
Tree-SHA512: 3f540464d38c72ba9d68f8ceda8600540bd0c3eef0ba67531c87fa1e0e4f757af7035cf80a1a5f17aa05604a17fdd9ef59bb6bece6b4145d540dac1e5362fc01
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.
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.
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.
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.
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`).
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.
Tobin C. Harding
Andrew Poelstra
Steven Roose
junderw
yancy
Salvatore Ingala
Martin Habovstiak