`s.parse` is more idiomatic and produces more helpful error messages.
This has been changed repo wide in the main codebase, not including
examples, rustdocs, and in the test module.
`use std::str::FromStr;` has been removed where this change makes
it unnecessary.
441aac0a08 fix: vec! macro enabled only for test module (Chris Hyunhum Cho)
a050618fd8 feat: remove zeroed vector by pushing front (Chris Hyunhum Cho)
Pull request description:
Remove useless zeroed vector following https://github.com/rust-bitcoin/rust-bitcoin/issues/3131.
1. Init vector with capacity not to realloc when pushing.
2. If vector is empty(the first iteration), push data from the front.
3. Iterate vector from the front, and copy it from the end.
ACKs for top commit:
apoelstra:
ACK 441aac0a08 successfully ran local tests
jamillambert:
ACK 441aac0a08
Kixunil:
ACK 441aac0a08
Tree-SHA512: dee934f675786e6977216de9516ecb7f03fa17304891f060b33d46ff3a02342b817628b68dc429c65730a52e1d99c3d0d5e9d47afee5b727b06d3c800767369e
bab2fc44b6 Bump units version (yancy)
Pull request description:
The master branch version of units is behind that of crates: https://crates.io/crates/bitcoin-units
This breaks the ability for those of us trying to patch and work off of master since Cargo ignores the patched version 0.1.1 which is technically ahead.
I'm not sure how this got out of sync and if there's a preferable way/commit to fix this.
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3171
ACKs for top commit:
Kixunil:
ACK bab2fc44b6
apoelstra:
ACK bab2fc44b6 successfully ran local tests
Tree-SHA512: 0243309dbe0ff2e650f88c136e7e3c22c798bcc744419d5ef72b7929d045acc6015c8710af2861e8782ea5ae3446a9a4c78ae18c394c6e605cd2afc3d7f71bdc
8fb9bf8262 ci: fix cron update-cargo-semver-checks (Jose Storopoli)
Pull request description:
typo in the comparison inside jq
Closes#3229
ACKs for top commit:
Kixunil:
ACK 8fb9bf8262 looks like something like `!.yanked` ought to work but I don't care enough to check and force rewrite.
tcharding:
ACK 8fb9bf8262
apoelstra:
ACK 8fb9bf8262 successfully ran local tests
Tree-SHA512: 8be05d08969e2b13eeb77c202352d736b076f5bc023f9144818c247c139e51c5187e331264723709ff67dcdc638f97403fbbe2c77b0a18b1b37c201d0d647183
8ae156fe8b Automated update to Github CI to rustc nightly-2024-08-25 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 8ae156fe8b
Tree-SHA512: 25ea39dd579630dba3683469f518f825b7b0078fa6def4cc3cce428944cc5d276f5d649b004e77aac0fdc9bcf610ab6fd69c45f7e65f3d2db08c1c16e8129aee
3e034d5ede Add Arbitrary dependency (yancy)
Pull request description:
Adds an example draft showing what is needed to use Arbitrary for coin selection.
Shot out to how nice Arbitrary is for fuzzing a target by taking unstructured randomness and creating structured rust-bitcoin types for fuzzing. Is there a way we could add this to rust-bitcoin for structuring the fuzz data needed?
This is then the example to fuzz test a SRD algo (after applying this PR to rust-bitcoin) using rust-bitcoin types :)
```
#![no_main]
use arbitrary::Arbitrary;
use bitcoin::{Amount, FeeRate};
use bitcoin_coin_selection::{select_coins_srd, WeightedUtxo};
use libfuzzer_sys::fuzz_target;
use rand::thread_rng;
#[derive(Arbitrary, Debug)]
pub struct Params {
target: Amount,
fee_rate: FeeRate,
weighted_utxos: Vec<WeightedUtxo>,
}
fuzz_target!(|params: Params| {
let Params { target: t, fee_rate: f, weighted_utxos: wu } = params;
select_coins_srd(t, f, &wu, &mut thread_rng());
});
```
ACKs for top commit:
tcharding:
ACK 3e034d5ede
Kixunil:
ACK 3e034d5ede
apoelstra:
ACK 3e034d5ede successfully ran local tests
Tree-SHA512: accd565815de3b37730d2ff12a24fcfc84e52ad357e5c940b1500a1e0bb17f4ff5fd6e52d31e8e96bb5290ee4fa050cfd2a9bbd6bbae13fc378f43093b64177f
9fb5edb39e ecdsa: Improve error types (Tobin C. Harding)
Pull request description:
There are a couple of issues around the ECDSA signature decoding / parsing code. We have duplicate code in `from_str` and `from_slice` and both use the same error type even though it is impossible to get a hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `FromStrError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
ACKs for top commit:
Kixunil:
ACK 9fb5edb39e
apoelstra:
ACK 9fb5edb39e successfully ran local tests
Tree-SHA512: 3b3ae31887d603f1739d261b491b99f7847987f94dbbfefb9aa84d4250736eba2d007d28746bbb064946d3055e4cca01510677bf2cdbb11bbf83d7388dbd2620
c427d8b213 bitcoin: Compile time assert on index size (Tobin C. Harding)
49a6acc1a0 internals: Remove double parenthesis in const_assert (Tobin C. Harding)
2300b285ef units: Remove compile time pointer width check (Tobin C. Harding)
Pull request description:
3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately.
- Patch 1: Don't check pointer width in `units` because its not consensus code
- Patch 2: Modify internal macro `const_assert`
- Patch 3: Use index size to enforce not building on a 16 bit machine
ACKs for top commit:
Kixunil:
ACK c427d8b213 though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later.
apoelstra:
ACK c427d8b213 successfully ran local tests; unsure if we want to merg this or wait for #3215
Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8
a2be82c0c9 Use TBD in deprecated attribute (Tobin C. Harding)
Pull request description:
Our `release` job checks for 'TBD', I can't remember exactly why but I thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when we used TBD - clearly this is not the case now because we have a bunch of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
ACKs for top commit:
Kixunil:
ACK a2be82c0c9
apoelstra:
ACK a2be82c0c9 successfully ran local tests
Tree-SHA512: b383cc4095484291a7b4dca593ad5e017e3a9de9bfae9d6e9447ae36da32aa1c0d1fd593f49fd52c04db5ca5cdbaae8b30a772f792df13542f0a157a86295746
6e5bd473a6 Improve siphash's `as_u64` -> `to_u64` rename (Martin Habovstiak)
1a91492204 Clean up the siphash mess (Martin Habovstiak)
Pull request description:
Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. Never break SSOT. ...
Previously we had removed `Default` impl on `siphash24::HashEngine` by
reimplementing the type manually. This was a really bad idea as it
inevitably led to API differences that broke the build which we didn't
notice because of unrelated bug. It should've just split the macro from
the start as was suggested but it was claimed to be difficult, I don't
think was the case as can be seen by this PR.
This commit does what the previous one should've done: it renames the
macro to have `_no_default` suffix, creates another one of the original
name that calls into `_no_default` one and moves anything related to
`Default`. This cleanly ensures all previous hashes stay the same while
siphash gets `Default` removed. This also removes all now-conflicting
impls from `siphash24` module which makes the module almost identical to
what it looked like before the change. The only differences are removed
`Default`/`new`, fixes in tests and recent rename of `as_u64` to
`to_u64`.
FTR both of our recent bugs were caused by breaking SSOT. It confirms again my life experience that breaking it will almost certainly bite one in the ass. If there is the most important principle in programming it's quite likely this one.
I was trying to be more "friendly" towards contributors breaking it but I no longer feel that this is a good choice, especially for complicated PRs. Thus I plan to be more aggressive here and NACK pretty much all breaks of SSOT that aren't provably unsolvable.
ACKs for top commit:
apoelstra:
ACK 6e5bd473a6 successfully ran local tests
Tree-SHA512: 03f413f302a41d07b52ac4645552231bb136aad9eb15010758fbad935ac2a686287d1adab7637372dd2629c37434b6ff9a085fba6792b8b6ec6c2a357e99538e
96e0e720fd feat(bip158): compute canonical filter hash (Rob N)
Pull request description:
From [BIP-157](https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki#filter-headers)
> The canonical hash of a block filter is the double-SHA256 of the serialized filter.
If a user forgets the "double" in double-SHA256 they will be computing a nonsensical filter hash when this is easily handled by the API.
ACKs for top commit:
Kixunil:
ACK 96e0e720fd
tcharding:
ACK 96e0e720fd
Tree-SHA512: 5fc0b1632e2327adacbd0ab56b4cd7edce0774f8be2f819782519c51b9691a748f4b3c01887f3bf1146dee49a35f9dfac833f53ae69ee7a53858bd2cedcec01a
There are a couple of issues around the ECDSA signature decoding /
parsing code. We have duplicate code in `from_str` and `from_slice`
and both use the same error type even though it is impossible to get a
hex error in `from_slice`.
Create two errors:
- A `DecodeError` returned by `from_slice`
- A `ParseSignatureError` that has a decode variant and a hex variant
Call through to `from_slice` after parsing hex into a byte vector.
Removes an instance of `unreachable!`.
Fix: #1193
Our `release` job checks for 'TBD', I can't remember exactly why but I
thought we introduced `0.0.0-NEXT-RELEASE` because CI was failing when
we used TBD - clearly this is not the case now because we have a bunch
of `TBD`s in the code base.
Change all the instances of `0.0.0-NEXT-RELEASE` to be `TBD`.
The previous change was just a dumb rename with no deprecation and it
also kept the `self` type which should be taken by value since the hash
is `Copy`. This improves on it by adding a deprecated method of the
original name and changing the type to be `self` instead of `&self`.
Previously we had removed `Default` impl on `siphash24::HashEngine` by
reimplementing the type manually. This was a really bad idea as it
inevitably led to API differences that broke the build which we didn't
notice because of unrelated bug. It should've just split the macro from
the start as was suggested but it was claimed to be difficult, I don't
think was the case as can be seen by this PR.
This commit does what the previous one should've done: it renames the
macro to have `_no_default` suffix, creates another one of the original
name that calls into `_no_default` one and moves anything related to
`Default`. This cleanly ensures all previous hashes stay the same while
siphash gets `Default` removed. This also removes all now-conflicting
impls from `siphash24` module which makes the module almost identical to
what it looked like before the change. The only differences are removed
`Default`/`new`, fixes in tests and recent rename of `as_u64` to
`to_u64`.
d03e5456a3 ci: fix `rev` to be `ref` when pinning bitcoin-maintainer-tools (Andrew Poelstra)
d04b6aabe5 bitcoin: add a couple missing prelude imports (Andrew Poelstra)
30f6bd43a4 Add primitives to the CRATES list (Andrew Poelstra)
e1e0230827 primitives: Enable alloc from serde (Tobin C. Harding)
ce36a37d68 primitives: Test ordered feature (Tobin C. Harding)
Pull request description:
This is a rebase of #3195 on master (since #3200 was merged) and adds one commit which fixes a few missing imports, which it appears was missed in the other "fix CI" PRs.
ACKs for top commit:
Kixunil:
ACK d03e5456a3
tcharding:
ACK d03e5456a3
Tree-SHA512: 6f5dc5c2cbb42716e83e46e70dbf4d644f9e80f6b3d2e75a276e37d33d461f4a86e67eb5c32444dbd23bb9767abedc88bfe958b1beba39852c382cf73736711f
Apparently the `checkout` action will just grab random shit in the case
that you configure it with an unknown key. There is a warning in the
middle of the CI output so ok, I guess there's that.
This commit does not change the pinned version of
rust-bitcoin-maintainer-tools, though we do want to update the pin,
because I want to make sure that the pin is actually working.
This should have been done when we introduced the `primitives`
crate - epic fail.
Intentionally put it before `fuzz` because of the known bug in the CI
script where nothing after `fuzz` gets tested.
Currently the `serde` feature requires an allocator, this is a hang over
from code being written for the `bitcoin` crate.
This was not found because we are not testing `primitives` correctly in
CI (there is the `fuzz` bug and also `primitives` is not even in the
CRATES list).
60b3cabb41 Panic in const context (Tobin C. Harding)
Pull request description:
Now that our MSRV is past 1.57 we can panic in const contexts.
Fix: #2427
ACKs for top commit:
Kixunil:
ACK 60b3cabb41
apoelstra:
ACK 60b3cabb41 successfully ran local tests
Tree-SHA512: 705a8b7d52a11826e6084684706cb7e01dfaa554e4e369739e64e64263537b0c8c0e518b04e96249ecdeea1f22b534594ffd2a89e17ebba85b369d896e820239
c97389596b Remove stale docs from sha256t_hash_newtype (Tobin C. Harding)
39f7dcb816 Reduce API surface of tagged wrapped hash types (Tobin C. Harding)
Pull request description:
Recently we made it so that wrapper types created with `hash_newtype` were not general purpose hash types i.e., one could not easily hash arbitrary data into them. We would like to do the same for tagged wrapped hash types.
In `hashes` do:
- Create a new macro `sha256_tag` that does just the tag/engine stuff out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256_tag` and `hash_newtype` to create tagged wrapped hash types.
Note that we do not add private helper functions `engine` and `from_engine` to the tagged wrapper types as we do for legacy/segwit in `sighash`. Can be done later if wanted/needed.
Fix: #3135
ACKs for top commit:
Kixunil:
ACK c97389596b
apoelstra:
ACK c97389596b successfully ran local tests
Tree-SHA512: d937a8eac1a77298231f946f9dfbc2f7739af8da00f2075b0b54803b4111c0cec810bc6564515153769193056cf102a9c954e216664f055b249d4a6153b14bca
39f10339ad Document the new build script magic (Tobin C. Harding)
Pull request description:
This is a follow up to #3182 which introduced a new way of conditionally including code based on the compiler version.
When originally reviewing I missed the fact that the two loops were controlled by the current compiler version (`minor`) so the created macro is different dependent on the compiler used to build the code.
To help the next guy notice, add a comment.
ACKs for top commit:
Kixunil:
ACK 39f10339ad
apoelstra:
ACK 39f10339ad successfully ran local tests
Tree-SHA512: 9a223718e2691a0f3b6a72c6d666ef4c238764b3ed967e15779311625f6535aea41c2e10795c5a98e69d0d50f23fc928edd72c01ed4a0074cf320f7c1a032b6a
4ead0adcb5 Add blanket impl of io traits for `&mut T` (Martin Habovstiak)
Pull request description:
The impl wasn't previously available because we thought we can do a blanket impl for `std` traits. We've removed the `std` blanket impl when we realized it's broken but forgot to add the `&mut T` impls. This adds them.
Note: this is in part an experiment to see if this is API breaking. I suspect it might be.
ACKs for top commit:
apoelstra:
ACK 4ead0adcb5 successfully ran local tests
tcharding:
ACK 4ead0adcb5
Tree-SHA512: 1e4411fdf019f3793e6366eda7e8b46246e3263bd7e41802877c20cc5e8ea451a79d89b4c59204ea9c1eb590054975de52a791a9d17a1d180a5cfac316efa959
009e747323 CI: Put fuzz last in CRATES list (Tobin C. Harding)
aed61bd2d4 Implement FromStr and serde impls for siphash (Tobin C. Harding)
f8846101ae siphash: Make functions inherent (Tobin C. Harding)
321d82ca53 hashes: Pin in extra_test (Tobin C. Harding)
42c7617a46 Fix shchemars test (Tobin C. Harding)
b6fda6517c Implement JsonSchema for siphash::Hash (Tobin C. Harding)
1af6ff4394 hashes: Feature gate hash_reader unit test (Tobin C. Harding)
5230d3309c Remove hash_reader from sha256t_hash_newtype (Tobin C. Harding)
Pull request description:
This is just the bug fixes pulled out of #2861
The root problem here is the bug described in https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/issues/10
This PR works around the bug by putting `fuzz` last in the list.
ACKs for top commit:
apoelstra:
ACK 009e74732321f9f3e6d09e158dd577a7d9ebca35; confirmed that local CI STILL does not pass after this; needs #3195 as well; but it is improved
Kixunil:
ACK 009e747323 but I'd much rather see the siphash problems addressed by splitting up the macro. I can make a PR for that (but just that) later.
Tree-SHA512: f724570003b862e994787192b720c4698e6b904ececa7188770ee507dd9e12382ae64db3363fa15ab3f347341c99ed9a75938f4da0438ce3a0fa88717497a233
4b10490521 Automated update to Github CI to rustc nightly-2024-08-21 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 4b10490521
Tree-SHA512: 445a50d3c11b8ef6b18130244f4a79c96ad034fa58b79baff463caca1bb3badf6ab829a99e56529dddc1a67d5bd488591af668684135d10cba6264be2915096c
Recently we made it so that wrapper types created with `hash_newtype`
were not general purpose hash types i.e., one could not easily hash
arbitrary data into them. We would like to do the same for tagged
wrapped hash types.
In `hashes` do:
- Create a new macro `sha256t_tag` that does just the tag/engine stuff
out of the `sha256t_hash_newtype` macro.
- Deprecate the `sha256t_hash_newtype` macro.
In `bitcoin` do:
- Use a combination of `sha256t_tag` and `hash_newtype` to create tagged
wrapped hash types.
Note that we do not add private helper functions `engine` and
`from_engine` to the tagged wrapper types as we do for legacy/segwit in
`sighash`. Can be done later if wanted/needed.
This is a follow up to #3182 which introduced a new way of
conditionally including code based on the compiler version.
When originally reviewing I missed the fact that the two loops were
controlled by the current compiler version (`minor`) so the created
macro is different dependent on the compiler used to build the code.
To help the next guy notice, add a comment.
c9053511b2 Remove misleading version metadata (Martin Habovstiak)
Pull request description:
The metadata in dependency specification was misleading because the version was not guaranteed to be the same anyway this was correctly linted but nobody so far cared to fix it. This change fixes it and adds a hint how to get the real version since some people seem still confused about how these things work.
ACKs for top commit:
apoelstra:
ACK c9053511b2 successfully ran local tests
tcharding:
ACK c9053511b2
Tree-SHA512: 99d849b1c1b202a429bc7eb25394ff84ba720b3ca6e73bce615f767e0924f5ef63a87553214f0f4fadd3b68ea84246ab8284d75df56cbbe794da7dea0d169dd9
In recent work making functions on hash types inherent it seems we
missed `siphash`. Add inherent getters/setters to the `siphash::Hash`
type and call through to them from the `Hash` trait impl.
Note:
- The `extra_test.sh` script runs for all toolchains.
- The `schemars` crate does not use the repo lock files.
- We need to pin some deps when building the `schemars` test.
Pin in the `extra_test.sh` script, and mention it in the docs so the
docs don't go stale next MSRV update.
This was previously unnoticed because of the `run_task` bug.
ref: rust-bitcoin/rust-bitcoin-maintainer-tools#10
Jamil Lambert, PhD
merge-script
yancy
Chris Hyunhum Cho
merge-script
Update Nightly Rustc Bot
Jose Storopoli
Martin Habovstiak
Rob N