Currently we have a bunch of sighash types defined in the `transaction`
module and we have a newer `sighash` module that defines a bunch of
related types. This is kruft from the development process as we added
the new types. If we put things that are related together it makes it
easier for devs to find their way around the library and grok the code.
Move all the sighash types from the `blockdata::transaction` module to
the `sighash` module.
Refactor only, no logic changes.
Add a `LockTime` type to hold the nLockTime `u32` value. Use it in
`Transaction` for `lock_time` instead of a `u32`. Make it public so this
new type can be used by rust-miniscript and other downstream projects.
Add a `PackedLockTime` type that wraps a raw `u32` and derives `Ord`,
this type is for wrapping a consensus lock time value for nesting in
types that would like to derive `Ord`.
Clippy emits two warnings of form:
warning: redundant field names in struct initialization
Remove the redundant field names and use struct short initialization
form.
For internal macros used only in this crate we do not need to use
`macro_use` and pollute the top level namespace now that we have edition
2018. We can add a `pub(crate) use` statement to each and then path
imports work for the macros like normal types.
Now we have edition 2018 we do not need to use `macro_use` or `extern
crate`; `pub use` works with macros.
Remove the extern crate statement and replace it with a pub use
statement. Requires fixing up various imports statements and also
requires importing `sha256t_hash_newtype` macro directly instead of
using a qualified path to it.
73bc2bb058 Remove leading colons from ::core::cmp::Ordering (Tobin C. Harding)
bffe0e840d Remove _most_ leading double colons (Tobin C. Harding)
Pull request description:
Leading double colons are a relic of edition 2015. Attempt to remove _all_ leading double colons (assuming I didn't miss any).
- Patch 1 is done mechanically so it can be repeated by reviewers, just search-and-replace ' ::' with '::' (note the leading space).
- Patch 2 does a single other instance of leading `::`
ACKs for top commit:
apoelstra:
ACK 73bc2bb058
sanket1729:
utACK 73bc2bb058.
Tree-SHA512: 8f7aafdda1aed5b69dcc83f544e65085dfec6590765839f0a6f259b99872805d1f00602fd9deac05c80a5cac3bc222d454dde0117dff4484e5cd34ea930fdfa1
e34bc538c3 Add new type for sequence (Noah Lanson)
Pull request description:
#1082
Created a new type for txin sequence field with methods to create sequences with relative time locks from block height or time units.
ACKs for top commit:
Kixunil:
ACK e34bc538c3
tcharding:
ACK e34bc538c3
apoelstra:
ACK e34bc538c3
Tree-SHA512: 6605349d0312cc36ef9a4632f954e59265b3ba5cfd437aa88a37672fe479688aa4a3eff474902f8cc55848efe55caf3f09f321b3a62417842bfc3ec365c40688
91ff2f628c Introduce SPDX license identifiers (Tobin C. Harding)
Pull request description:
When `rust-bitcoin` was started in 2014 the SPDX license list and short identifiers where not a thing. Now that we have short identifiers and they are gaining popularity in other projects we can consider using them.
- Add links to the SPDX website in the readme
- Shorten the author section to a single line
- Remove all the licence information in each file and replace it with an
SPDX ID (see https://spdx.dev/ids/#how)
Of note:
- If the author of a file is explicitly listed, maintain this information
- If the 'author' is listed as the generic 'Rust Bitcoin developers' just remove the attribution, this is implicit. This does loose the date info but that can be seen at any time from the git index using
`git log --follow --format=%ad --date default <FILE> | tail -1`
apoelstra, please confirm that I'm not treading on your toes here, especially, are you ok with the new 'written by' string format?
### Ref
- https://spdx.dev/ids/#how
- https://spdx.org/licenses/CC0-1.0.html
- https://spdx.dev/ids/
ACKs for top commit:
apoelstra:
ACK 91ff2f628c
sanket1729:
ACK 91ff2f628c. I am also in IDGAF camp, but I like more red lines in diff.
Kixunil:
ACK 91ff2f628c
Tree-SHA512: ca8aac00f015c18ec18de83dfeb50dd6f4f840653c7def85daa2436a339021ada5f3c34ad0cdf6b18e3e39c45a6d58a8313742e4001d467785b10eee7fdbc938
Leading double colons are a relic of edition 2015. Remove all leading
double colons that follow a space, done like this so that reviewers can
do the same and verify the diff. Done with
search-and-replace ' ::' '::'
And, for the record:
```bash
function search-and-replace() {
if (($# != 2))
then
echo "Usage: $0 <this> <that>"
return
fi
local this="$1"
local that="$2"
for file in $(git grep -l "$this")
do
perl -pi -e "s/$this/$that/g" "$file"
done
}
```
When `rust-bitcoin` was started in 2014 the SPDX license list and short
identifiers where not a thing. Now that we have short identifiers and
they are gaining popularity in other projects we can consider using
them.
- Add links to the SPDX website in the readme
- Shorten the author section to a single line
- Remove all the licence information in each file and replace it with an
SPDX ID (see https://spdx.dev/ids/#how)
Of note:
- If the author of a file is explicitly listed, maintain this
information
- If the 'author' is listed as the generic 'Rust Bitcoin developers'
just remove the attribution, this is implicit. This does loose the date
info but that can be seen at any time from the git index using
`git log --follow --format=%ad --date default <FILE> | tail -1`
Fix#1020 (see more relevant discussion there)
This definitely makes the amount of generics compiler
has to generate by avoding generating the same functions
for `R`, &mut R`, `&mut &mut R` and so on.
old:
```
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 9947832 Jun 2 22:42 target/release/deps/bitcoin-07a9dabf1f3e0266
> strip target/release/deps/bitcoin-07a9dabf1f3e0266
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 4463024 Jun 2 22:46 target/release/deps/bitcoin-07a9dabf1f3e0266
```
new:
```
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 9866800 Jun 2 22:44 target/release/deps/bitcoin-07a9dabf1f3e0266
> strip target/release/deps/bitcoin-07a9dabf1f3e0266
> ls -al target/release/deps/bitcoin-07a9dabf1f3e0266
-rwxrwxr-x 1 dpc dpc 4393392 Jun 2 22:45 target/release/deps/bitcoin-07a9dabf1f3e0266
```
In the unit-test binary itself, it saves ~100KB of data.
I did not expect much performance gains, but turn out I was wrong(*):
old:
```
test blockdata::block::benches::bench_block_deserialize ... bench: 1,072,710 ns/iter (+/- 21,871)
test blockdata::block::benches::bench_block_serialize ... bench: 191,223 ns/iter (+/- 5,833)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 37,543 ns/iter (+/- 732)
test blockdata::block::benches::bench_stream_reader ... bench: 1,872,455 ns/iter (+/- 149,519)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 136 ns/iter (+/- 3)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 51 ns/iter (+/- 8)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 5 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_size ... bench: 3 ns/iter (+/- 0)
```
new:
```
test blockdata::block::benches::bench_block_deserialize ... bench: 1,028,574 ns/iter (+/- 10,910)
test blockdata::block::benches::bench_block_serialize ... bench: 162,143 ns/iter (+/- 3,363)
test blockdata::block::benches::bench_block_serialize_logic ... bench: 30,725 ns/iter (+/- 695)
test blockdata::block::benches::bench_stream_reader ... bench: 1,437,071 ns/iter (+/- 53,694)
test blockdata::transaction::benches::bench_transaction_deserialize ... bench: 92 ns/iter (+/- 2)
test blockdata::transaction::benches::bench_transaction_serialize ... bench: 17 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_serialize_logic ... bench: 5 ns/iter (+/- 0)
test blockdata::transaction::benches::bench_transaction_size ... bench: 4 ns/iter (+/- 0)
```
(*) - I'm benchmarking on a noisy laptop. Take this with a grain of salt. But I think
at least it doesn't make anything slower.
While doing all this manual labor that will probably generate conflicts,
I took a liberty of changing generic type names and variable names to
`r` and `R` (reader) and `w` and `W` for writer.
Clippy warns about creating a reference that is immediately
de-referenced.
Remove unnecessary explicit `&`, while we are at it remove unnecessary
explicit types that appear on the same lines of code.
57dd6739c3 Do not print error when displaying for std builds (Tobin C. Harding)
b80cfeed85 Bind to error_kind instead of e (Tobin C. Harding)
241ec72497 Bind to b instead of e (Tobin C. Harding)
01f481bf5c Bind to s instead of e (Tobin C. Harding)
5c6d369289 network: Remove unused error variants (Tobin C. Harding)
e67e97bb37 Put From impl below std::error::Error impl (Tobin C. Harding)
6ca98e5275 Remove error TODO (Tobin C. Harding)
Pull request description:
As part of the ongoing error improvement work and as a direct result of [this comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/987#issuecomment-1135563287) improve the `Display` implementations of all our error types so as to not repeat the source error when printing.
The first 5 patches are trivial clean ups around the errors. Patch 6 is the real work.
EDIT: ~CC @Kixunil, have I got the right idea here bro?~ Patch 6 now includes a macro as suggested.
ACKs for top commit:
Kixunil:
ACK 57dd6739c3
apoelstra:
ACK 57dd6739c3
sanket1729:
ACK 57dd6739c3. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future.
Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de
2e7effc604 Feature `use-serde` renamed to `serde` (Martin Habovstiak)
Pull request description:
Features activating external crates are supposed to have same name as
those crates. However we depend on same feature in other crates so we
need a separate feature. After MSRV bump it is possible to rename the
crates and features so we can now fix this inconsistency.
Sadly, derive can't see that the crate was renamed so all derives must
be told to use the other one.
Replaces #373
ACKs for top commit:
apoelstra:
ACK 2e7effc604
Tree-SHA512: b20364b9e8f30c2269bef915e821b2b2ec929e71dd0e88af2bc3a021821f87011d35e095cb8efe99add77a23dde940a17537eb387fb4582b05c57c8679969eb0
Adding an error variant to a public enum is an API breaking change, this
means making what could be small refactorings or improvements harder. If
we use `non_exhaustive` for error types then we mitigate this cost.
There is a tradeoff however, downstream users who explicitly match on
our public error types must include a wildcard pattern.
We implement `source` for all our error types. This means that we should
not display the source error explicitly because users can call `source`
to get the source error.
However, `std::Error::source()` is only available for "std" builds, so
that we do not loose the error source information in "no-std" builds add
a macro that conditionally adds the source onto the error message.
Features activating external crates are supposed to have same name as
those crates. However we depend on same feature in other crates so we
need a separate feature. After MSRV bump it is possible to rename the
crates and features so we can now fix this inconsistency.
Sadly, derive can't see that the crate was renamed so all derives must
be told to use the other one.
97a5bb1439 Implement std::error::source codebase wide (Tobin C. Harding)
0a9191b429 Add parenthesis around left hand side of companion (Tobin C. Harding)
7cf8af2f86 Put Error impl block below Display (Tobin C. Harding)
2384712364 Re-order Display match arms (Tobin C. Harding)
Pull request description:
Now that we have MSRV of 1.41.1 we should use `source` instead of `cause`. Audit the whole codebase and implement `source` for _every_ error type we have.
The first three patches are preparatory cleanup, patch 3 is particularly shameful (adds parenthesis to make my editor work).
CC @Kixunil because he is championing the error stuff.
ACKs for top commit:
apoelstra:
ACK 97a5bb1439
Tree-SHA512: 46313a28929445f32e01e30ca3b0246b30bc9d5e43db5754d4b441e9c30d3e427efaf247100eb6b452f98beec5a4fcde1daba7943a772114aa34f78ab52cbc60
Improve documentation in `psbt/mod.rs` by doing:
- Use full sentences (full stops and capitalisation)
- Use 100 line column width
- Use back ticks and links as appropriate
- Use `Errors` section
- Use third person tense to describe functions
Use cargo to upgrade from edition 2015 to edition 2018.
cargo fix --edition
No manual changes made. The result of the command above is just to fix
all the use statements (add `crate::`) and fix the fully qualified path
formats i.e., `::Foo` -> `crate::Foo`.
5afb0eaf40 API to get an iterator for funding utxos in psbt (violet360)
Pull request description:
### Current status
The API returns a vector of UTXOs and has return type `Result<Vec<&TxOut>, Error>`
### Expected
The return statement should be of type `sighash::Prevouts` as pointed in #849
ACKs for top commit:
Kixunil:
ACK 5afb0eaf40
tcharding:
ACK 5afb0eaf40
sanket1729:
ACK 5afb0eaf40. Thanks for being patient with this.
Tree-SHA512: 724fc3dffdbb1331584f89bbe84527e1af0d193a344fe43b36f2f2a628652d259001a3abf6b3909df53524cd3fbdbe3af760b7004d40d3bee1848fbb83efff5b
Programmers are inherently lazy and for good reason. I'm yet to see
anyone write `PartiallySignedTransaction` in code that uses
`rust-bitcoin`, its too obvious to add a type alias for PSBTs, let's
just do it ourselves to save everyone else having to do so.
Add public type alias `Psbt` for `PartiallySignedTransaction`.
Recently we added a bunch of additional sighash types, some of the code
comments became stale. Use the non-specific term 'sighash type' instead
of a particular sighash identifier in comments to make the comments more
applicable.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `PsbtSigHashType` to `PsbtSighashType`.
Our usage of `SigHash` implies that 'sighash' is _two_ words; 'sighash'
is a well known word in the Bitcoin ecosystem it should appear in
identifiers as `Sighash`.
Rename `EcdsaSigHashType` to `EcdsaSighashType`.
In this library we specifically do not use rustfmt and tend to favour
terse statements that do not use extra lines unnecessarily. In order to
help new devs understand the style modify code that seems to use an
unnecessary number of lines.
None of these changes should reduce the readability of the code.
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for combining together two PSBTs. Doing so broke the
`psbt` module.
Pull the merge logic out of the `Map` trait and put it in methods on
each individual type (`Input`, `Output`, `PartiallySignedTransaction`).
Doing so allows for simplification of return types since combining
inputs/outputs never errors.
Use the term 'combine' instead of 'merge' since that is the term used in
BIP 174.
We currently have the `map` module private but containing a bunch of
types that are needed in the public API (specifically in a
`PartiallySignedTransaction`).
Re-export the publicly required types to the `psbt` module and then
again at the root level of `rust-bitcoin` as we do for other types.
abe52f681b Cleanup/Dedup psbt (De)Serialization code (sanket1729)
fbd86dcf63 Update documentation of EcdsaSig::from_slice (sanket1729)
85009a7b50 Update documentation of from_u32_consensus (sanket1729)
0fed04e2d5 Change EcdsaSig hash type deser (sanket1729)
Pull request description:
Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value
ACKs for top commit:
dr-orlovsky:
ACK abe52f681b
apoelstra:
ACK abe52f681b
Kixunil:
ACK abe52f681b
Tree-SHA512: 1d5dbe3aa5885ca16649cf8ea05a7476e8dd977dd870b79358d97a3ce383bee93754d2b88163e7db3792cdc4b9cb867356409c8eea4e110877577ad196ba0786
The `Map` trait has been deemed confusing and not that useful to users
of the library, we still use it internally within the `psbt` module
though so make it visible only in `psbt` and `psbt::map`.
Tobin C. Harding
Andrew Poelstra
Noah Lanson
Dawid Ciężarkiewicz
sanket1729
Martin Habovstiak
Riccardo Casatta
violet360
Dr Maxim Orlovsky