The BIP-158 finalize code was serializing value to `Vec` only to
serialize it to writer right away. Thus the intermediary `Vec` was not
needed.
Even more, the code used `write` which, while correct in case of `Vec`,
could trigger code analysis tools or reviewers.
After MSRV bump `try_from` usually refers to `TryFrom` method but
`CommandString` used inherent one making it confusing and non-idiomatic.
This implements `FromStr` and `TryFrom<{stringly type}>` for
`CommandString` and deprecates the inherent method in favor of those.
To keep the code using `&'static str` efficient it provides
`try_from_static` inherent method that only converts from
`&'static str`.
Closes#1135
071a1c02b7 Implement string parsing for `Sequence` (Martin Habovstiak)
c39bc3909d Extend `ParseIntError` to carry more context (Martin Habovstiak)
Pull request description:
When debugging parsing errors it's very useful to know some context:
what the input was and what integer type was parsed. `ParseIntError`
from `core` doesn't contain this information.
In this commit a custom `ParseIntError` type is crated that carries the
one from `core` as well as additional information. Its `Display`
implementation displays this additional information as a well-formed
English sentence to aid users with understanding the problem. A helper
function parses any integer type from common string types returning the
new `ParseIntError` type on error.
To clean up the error code a bit some new macros are added and used.
New modules are added to organize the types, functions and macros.
Closes#1113
Depends on #994
ACKs for top commit:
apoelstra:
ACK 071a1c02b7
tcharding:
ACK 071a1c02b7
Tree-SHA512: 31cb84b9e4d5fe3bdeb1cd48b85da2cbe9b9d17d93d029c2f95e0eed5b8842d7a553afafcf8b4a87c075aa53cf0274776e893bed6dca37e7dbc2e1ee1d602b8e
Recently we introduced SPDX license ids but there was some confusion
about the use, form, and purpose of lines before (library name, author).
It was pointed out to me by Andrew that if/when folks cut'n'paste our
code they often will keep the whole blurb. Design it with that in
mind. FTR, Andrew also indicated that he did not mind his name being
removed in favour of "The rust-bitcoin developers".
At first I thought the term "The rust-bitcoin developers" was a bit
wishy-washy but yesterday I realised that I am proud to be part of that
crew, striving to deliver code to the highest possible standard.
Introduce a canonical format for the licences blurb.
- Use the name of the library
- Use "The rust-bitcoin developers"
- Use the SPDX ID
c67f74bb81 Upgrade bech32 to version 0.9.0 (Tobin C. Harding)
Pull request description:
Recently `rust-bech32` v0.9.0 was released, this release included
updating the MSRV to 1.41.1.
ACKs for top commit:
apoelstra:
ACK c67f74bb81
Kixunil:
ACK c67f74bb81
Tree-SHA512: 59aac2f40e0bc8c63a7549a988ff7cdc114ca7768421c7926cc6badbaa1d4f9de0c5cf41d987e4f5124a5c9042650e722cf5fbd892e91d0232ac175122eb042d
`Sequence` didn't have `FromStr` nor `TryFrom<{stringly type}>`
implemented by accident. This moves a macro for implementing them from
`locktime` module to the `parse` module, renames it for clarity and uses
it to implement parsing for `Sequence`.
When debugging parsing errors it's very useful to know some context:
what the input was and what integer type was parsed. `ParseIntError`
from `core` doesn't contain this information.
In this commit a custom `ParseIntError` type is crated that carries the
one from `core` as well as additional information. Its `Display`
implementation displays this additional information as a well-formed
English sentence to aid users with understanding the problem. A helper
function parses any integer type from common string types returning the
new `ParseIntError` type on error.
To clean up the error code a bit some new macros are added and used.
New modules are added to organize the types, functions and macros.
Closes#1113
0ed78e543b Add lock time types (Tobin C. Harding)
1390ee12ec Add a max scriptnum constant (Tobin C. Harding)
Pull request description:
Implement a `LockTime` type that adds support for lock time values based on nLockTime and OP_CHECKLOCKTIMEVERIFY.
For example usage in `rust-miniscript` please see https://github.com/rust-bitcoin/rust-miniscript/pull/408.
### Notes:
I probably should have opened a new PR, this is a total re-write and very different from what is being discussed below, sorry, my bad.
This is just half of the 'timelock' story, its the easier half. The reason I switched terminology from timelock to locktime is that we have to compare two u32s so it does not make sense to call them _both_ timelocks.
If I have missed, or apparently ignored, anything you said reviewers please accept my apology in advance, it was not intentional. The thread of discussion is long and this topic is complex. Please do restate your views liberally :)
Here is a useful blog post I used while touching up on timelock specifics: https://medium.com/summa-technology/bitcoins-time-locks-27e0c362d7a1. It links to all the relevant bips too.
ACKs for top commit:
Kixunil:
ACK 0ed78e543b
apoelstra:
ACK 0ed78e543b
Tree-SHA512: 486fcce859b38fa1e8e6b11cd2f494462d6d7d1d9030d096ce6b260f6c9d0342b8952afe35152bdf3afe323a234a8165ac3d6c946304afcc13406d7a0489d75a
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`.
Integers within Script can have a maximum value of 2^31 (i.e., they are
signed) but we (miniscript) often uses unsigned ints, to facilitate
checking the unsigned type is the correct size to fit in a signed int
add a const `MAX_SCRIPTNUM_VALUE`.
fe840f0b42 Fix stale toolchain docs (Tobin C. Harding)
73b506e149 Remove stale MSRV docs (Tobin C. Harding)
Pull request description:
We have stale docs referring to the old MSRV. We do not need MSRV docs
in `CONTRIBUTING` because we have them in the README already.
Fix stale docs by doing:
- Remove the MSRV docs from readme in favour of `CONTRIBUTING.md`.
- Add a sentence to the redame pointing readers towards `CONTRIBUTING.md`.
- Point readers towards `RUSTUP_TOOLCHAIN`
ACKs for top commit:
apoelstra:
ACK fe840f0b42
Kixunil:
ACK fe840f0b42
Tree-SHA512: 6d3006db7a460e5d7146b73ff4b99dc322d396e65da39f5ea47c4904aa387c8a36e4a8ae707653dbab90096e692bf69918e6c19c4d3d989c85b22dba4dcba904
74f3a5aeda Run clippy from the test script (Tobin C. Harding)
aa8109a791 Use struct field short form (Tobin C. Harding)
d1a05401f4 Remove redundant calls to clone (Tobin C. Harding)
196492554d Use assert instead of assert_eq (Tobin C. Harding)
3173ef9dbb Remove unnecessary explicit reference (Tobin C. Harding)
Pull request description:
Currently we run clippy in CI using a github action. The invocation has a couple of shortcomings
1. it does not lint the tests (this requires `--all-targets`)
2. it does not lint the examples
I could not find a way to lint the examples without explicitly linting each example by name.
**This PR does the following:**
- Fix clippy issues (patch 1-4)
- Move the clippy control to `test.sh`
- Add an env var `DO_LINT` to control it
- Remove the separate CI job
- Run the linter during the `Test` job using the stable toolchain.
- Run clippy with ` --all-features` AND `--all-targets` (only recently made possible).
- Run each example explicitly
Thanks to dunxen for noticing all the errors in my psbt example on review and prompting me to work out why clippy was not running :)
ACKs for top commit:
apoelstra:
ACK 74f3a5aeda
Kixunil:
ACK 74f3a5aeda
Tree-SHA512: 56dc6262144f4caa5efa6fdc46aeecf7bddc050ef134a639b31a34d4c5e01abcdeda18a00f4ded443866bbdfc982b4e5b67b0089639e0c253e207f0b54777f57
We have introduced `rustfmt` but forgot to update the docs section about
it. Since a large portion of the codebase is currently ignored by our
`rustfmt` configuration, point out that `rusntfmt` is work in progress.
We have docs that use an env var `BITCOIN_MSRV` that no longer exists.
Point readers towards `RUSTUP_TOOLCHAIN`.
(Note that we still use `TOOLCHAIN` in the script but it is being phased
out, `RUSTUP_TOOLCHAIN` works today.)
We have stale docs referring to the old MSRV. We do not need MSRV docs
in `CONTRIBUTING` because we have them in the README already.
Fix stale docs by doing:
- Remove the MSRV docs from readme in favour of `CONTRIBUTING.md`.
- Add a sentence to the redame pointing readers towards `CONTRIBUTING.md`.
Currently we run clippy in CI using a github action. The invocation has
a couple of shortcomings
1. it does not lint the tests (this requires `--all-targets`)
2. it does not lint the examples
I could not find a way to lint the examples without explicitly linting
each example by name.
Move the clippy control to `test.sh` and add an env var `DO_LINT` to
control it. Remove the explicit CI job and run the linter during the
`Test` job using the stable toolchain.
Clippy emits two warnings of form:
warning: redundant field names in struct initialization
Remove the redundant field names and use struct short initialization
form.
Clippy emits various warnings of type:
warning: this expression creates a reference which is immediately
dereferenced by the compiler
As suggested, remove the unnecessary explicit reference.
ee29910911 BIP152: Test net msg ser/der and diff encoding (0xb10c)
cd1aaaf344 BIP152: Add Compact Block unit test based on Elements (Steven Roose)
d4d92a838e BIP152: Add Compact Blocks network messages (Steven Roose)
f2fcdc86e6 BIP152: Add basic Compact Block structures (Steven Roose)
a9a39c4b08 blockdata: Derive PartialOrd, Ord and Hash for BlockHeader (Steven Roose)
Pull request description:
> Adds the basic structures for BIP152 and a method to create a compact block from a full block.
This is a rebase of #249 by stevenroose (see https://github.com/rust-bitcoin/rust-bitcoin/pull/249#issuecomment-1170562141) with a milestone for 29.0. I've added deserialization and serialization tests for the network messages and fixed an off-by-one bug in the deserialization of the differentially encoded varints in the `getblocktxn` message (see https://github.com/rust-bitcoin/rust-bitcoin/pull/249#discussion_r914989521).
Closes#249.
ACKs for top commit:
tcharding:
ACK ee29910911
apoelstra:
ACK ee29910911
Tree-SHA512: 462a91576281f5a2ffdc2610769ea93970b60dac75a150c827966c48daec7cf93f526f9f202e7ba1dbb1410b49148579880260a3c3df298b98330c0d891a4cca
3c2869465b Use to_be_bytes (Tobin C. Harding)
Pull request description:
Now that MSRV is > 1.32 we can use `u16::to_be_bytes` to ensure network byte order when encoding the port number of a `AddrV2Message`.
Remove the TODO and use `to_be_bytes` as suggested.
ACKs for top commit:
Kixunil:
ACK 3c2869465b
apoelstra:
ACK 3c2869465b
Tree-SHA512: b5dd9b0f43257f84defb9e9ecf9d02469f1959669367c5ad02cfb43003b780cf07ea344579b52a75c424fd85f8fa02dee3713b967c9b3a33eec6b7aa914763cd
7f498ee2a2 Add docs re Rust 1.51.1 (Tobin C. Harding)
Pull request description:
After auditing the code base for 'msrv' the only remaining mention (excl. md files) is on `unsigned_abs`.
Add docs to save the next guy looking up what version of Rust introduced `unsigned_abs`. (I also added an item to the [msrv tracking issue](https://github.com/rust-bitcoin/rust-bitcoin/issues/1060).)
ACKs for top commit:
Kixunil:
ACK 7f498ee2a2
apoelstra:
ACK 7f498ee2a2
Tree-SHA512: 9b4bdca09d3f7ac1c0584f4eb5207983a064cfe81ed26952d00396b2e0019ef40eee192b7f09d36c68b8c4e1f8de9cac2d1f3ee0946626bae089b46fe38704ab
Now that MSRV is > 1.32 we can use `u16::to_be_bytes` to ensure network
byte order when encoding the port number of a `AddrV2Message`.
Remove the TODO and use `to_be_bytes` as suggested.
517059e148 Use u8::try_from (Tobin C. Harding)
Pull request description:
Currently we use a cast to get the `u8` depth, as suggested by the TODO in the code we can now use `TryFrom` since we bumped the MSRV.
Use `u8::try_from.expect("")` since depth (node count) is guarded by `TAPROOT_CONTROL_MAX_NODE_COUNT`.
ACKs for top commit:
Kixunil:
ACK 517059e148
apoelstra:
ACK 517059e148
Tree-SHA512: 98e58617247a05025ec13ad3d00a464cb2351c98c6d871be43b252d3982e291b7187e25780e7fe367f5a3a64fa20f3b328876a8901af4da09e675f50727a26cf
This adds tests for serialization of BIP152 network messages and
tests specifically for the differential encoding of the transaction
indicies of 'getblocktx'. Previously, this code contained an
off-by-one error.
Currently we use a cast to get the `u8` depth, as suggested by the TODO
in the code we can now use `TryFrom` since we bumped the MSRV.
Use `u8::try_from.expect("")` since, assuming the code comment is
correct, the depth is guaranteed to be less that 127.
cda097dda8 Add ci check for duplicate dependencies (Tobin C. Harding)
Pull request description:
Add a call to `cargo tree --duplicates` in the ci script to ensure that we do not have any duplicated dependencies.
Kudos to Kixunil for the idea (over in: https://github.com/rust-bitcoin/rust-bitcoin/pull/1104)
ACKs for top commit:
apoelstra:
ACK cda097dda8
Kixunil:
ACK cda097dda8
Tree-SHA512: 77f07dd5c6794b5a59293bd62bda0fe61384a30cf8258e79aca9ce32090f869f0a13929b6a7a4c35e10fc653968b12ddd4c291df9ecd0962632017f59c81d025
b1faf63e82 Use listener.accept() (Tobin C. Harding)
Pull request description:
During test network simulation we only accept a single connection, we can simplify the code by using `accept`.
Done as a follow up to review suggestion:
https://github.com/rust-bitcoin/rust-bitcoin/pull/1042#discussion_r898013799
ACKs for top commit:
Kixunil:
ACK b1faf63e82
apoelstra:
ACK b1faf63e82
Tree-SHA512: b2ead15d3108db3e01d9faab5e3521403dad6a0f4c3cf505f88fefd020110c520a89b9406484c10b04c9a34073c8abc465941577b17b5a193b54502c23b14c61
Replace all instances of
`secp256k1::Message::from_slice(_).expect(_)` with
`secp256k1::Message::from(_)`.
Also adds an implementation of ThirtyTwoByteHash for
TapSighashHash.
Solves https://github.com/rust-bitcoin/rust-bitcoin/issues/824
21a1cc791c Use pub(crate) for macros instead of macro_use (Tobin C. Harding)
23ee0930c7 Remove extern crate (Tobin C. Harding)
da8b1b5439 Remove unused extern crate test (Tobin C. Harding)
01a8cc6848 Remove extern crate bitcoin_hashes (Tobin C. Harding)
Pull request description:
Do some clean up of `extern crate` statements now we have edition 2018.
4 separate but very similar patches, separated to ease review.
ACKs for top commit:
sanket1729:
utACK 21a1cc791c
apoelstra:
ACK 21a1cc791c
Tree-SHA512: fba33ed8fd261cc756dad8dd94f186a5b38aaf20cf31c3a83ad7633e7bb60a390681c39ebfd913e9e242fffed3b502491d067250d72ebfe666b4d03e93c8b945
df73576515 Upgrade to secp v0.24.0 (Tobin C. Harding)
Pull request description:
We just released a new version of `rust-secp256k1`, lets use it.
This also fixes a bug where we upgraded our `bitcoin_hashes` dependency
before secp had done theirs.
ACKs for top commit:
sanket1729:
utACK df73576515
apoelstra:
ACK df73576515
Tree-SHA512: 614eb096203753a6581e444aa11d41c7060342afd7ad103f6118c64bccf604850819ec2852730d1ac83fdea193770fe7600ea70c082a19afaef7ad3e8016d0e4
c1d74a3eae Run the formatter (Tobin C. Harding)
Pull request description:
We recently merged a PR that enables the formatter on the `examples/` directory, at the same time we merged `examples/ecdsa-psbt.rs` but it had formatting issues under the new formatter config.
Run `cargo +nightly fmt` to format the `examples/` directory.
ACKs for top commit:
sanket1729:
utACK c1d74a3eae
apoelstra:
ACK c1d74a3eae
Tree-SHA512: 645bf01c7cbb21e2eec23e6fa9c2f5d26e03566059d906e1985af5a9c0186470ef8a0c295f7be0a5226e2f948e2a94aa336de4b3704a31fd8715764f1625737d
We just released a new version of `rust-secp256k1`, lets use it.
This also fixes a bug where we upgraded our `bitcoin_hashes` dependency
before secp had done theirs.
We recently merged a PR that enables the formatter on the `examples/`
directory, at the same time we merged `examples/ecdsa-psbt.rs` but CI
had already run so the formatter was not run.
Run `cargo +nightly fmt` to format the `examples/` directory.
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. Notable exceptions are `alloc` and
`test`. Also leave the serde rename because touching it opens a can of
worms.
Recently we removed the "unstable" feature, I missed the duplicate
`extern crate test` when doing so :(
Since we no longer have the "unstable" feature this line of code is
never compiled in.
Remove the unused ``extern crate test`, we have the correct line further
up the file `#[cfg(bench)] extern crate test;`.
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.
ded1a32a59 Add cargo crev reminder to readme (Tobin C. Harding)
Pull request description:
As suggested by the `cargo-crev` project; add a comment to the readme reminding people to use `cargo-crev` to check their dependencies.
### Notes
Today I explored `cargo-crev`, it was new to me before today. I completed proofs for `bech32`, `rust-bitcoinconsenus`, `bitcoin_hashes`, `rust-secp256k1`, and `rust-bitcoin`. I published the proofs to https://github.com/tcharding/crev-proofs.
If I'm understanding correctly proofs are only useful if the author is connected to a web of trust. So far I only found @dpc within the active rust-bitcoin devs with a `crev-proofs` repo (that includes an ID). Since he wrote `cargo-crev` its not surprising he has one :) Two other devs have `crev-proofs` repos but they are both incomplete (no ID) so I was unable to climb onto their web, so to speak. I am not a particularly well know dev so I imagine it would be more useful if some of you more well know fellas publish proofs as well.
If we can get a web of trust between all the regular hackers here then we can start doing reviews/proofs of our dependencies and publishing them.
ACKs for top commit:
Kixunil:
ACK ded1a32a59
apoelstra:
ACK ded1a32a59
sanket1729:
ACK ded1a32a59
Tree-SHA512: c2d3b195a522095fcabcf51bb956b339f3a421541652f646f8e56286ebf850aa106d4acbf4defd344b5b0f57dd9626d1dbafe50c9d54b1436fd9e2c8b434fc07
ee9a3ec6a1 Enable formatter for "src" (Tobin C. Harding)
743b197124 Add use for Unexpected (Tobin C. Harding)
fd217e72c3 Use f instead of formatter (Tobin C. Harding)
7b50a96ade Do not format prelude module (Tobin C. Harding)
01471f7e65 Shield hash_newtypes from rustfmt (Tobin C. Harding)
65d19d9044 Refactor compile_error message (Tobin C. Harding)
6461e2db8d Run formatter on examples/ (Tobin C. Harding)
fd1c6589ba Add a rustfmt configuration file with explicit ignores (Tobin C. Harding)
Pull request description:
Looks like we are getting to a place that rustfmt _might_ be able to be used. In an effort to introduce `rustfmt` without requiring devs to do excessively mundane reviews introduce `rustfmt` in a non-invasive manner. What this means is
- Add a fully fleshed out `rustfmt` config file that explicitly ignores all the source files
- Enable sections of code one by one so review is easier (including preparatory patches before doing each section).
This PR currently does `examples` and all the source files at the root level of the crate (i.e. excludes all directories in `src/`). The other directories can then be done one at a time.
Please see discussion on: https://github.com/rust-bitcoin/rust-bitcoin/pull/959 for more context.
ACKs for top commit:
sanket1729:
ACK ee9a3ec6a1
apoelstra:
ACK ee9a3ec6a1
Tree-SHA512: f4ff3c031b5d685777e09bac2df59ed8217576b807da7699f44fe00aa509d0b7fe47617a8b3eff00d3125aeece3e010b8f9dd8733d315ccb2adc0e9a7d7f07e3
Martin Habovstiak
Andrew Poelstra
Tobin C. Harding
0xb10c
Steven Roose
Arturo Marquez
sanket1729