7e2a81d03b Remove unused address::Error type (Tobin C. Harding)
a92dc9c35c Add NetworkValidationError (Tobin C. Harding)
Pull request description:
Replaces #2502 because there is going to be way too much arguing on this to bother a newer contributor with.
This PR takes into consideration #2507 but does not improve the issue, it also does not make it worse. I propose to do this and then consider #2507 since this is a step forwards IMO.
Remove the `address::Error` because its not good. Add a `NetworkValidationError` and return it from `require_network` - leaving the door open for resolving Kix's issue in 2507.
ACKs for top commit:
Kixunil:
ACK 7e2a81d03b
apoelstra:
ACK 7e2a81d03b
Tree-SHA512: 0a220dbec1457f35e3d95f32399f258e65c0477e8b4d14dbe2dfad0a44966ab0339d4c2d9828da318bf131db45cecb2447c0e32d5669a5f4c4739b90c83d3c0d
9d688396c9 base58: Use pub extern crate instead of module (Tobin C. Harding)
Pull request description:
We don't add any implementations to the `base58` types so we can just `pub extern` the crate instead of using a module and re-exporting.
ACKs for top commit:
Kixunil:
ACK 9d688396c9
apoelstra:
ACK 9d688396c9
Tree-SHA512: 521af0fd1ae365a6d060dd3c38fc18c1fb3a6c46adb7cba64e53128c7a898c766bcc603078b4cb8a3672df96559633495b23203a33147b5b4959b0c690ab7451
c2d658ac05 Add `P2shError` for handling errors related to P2sh (harshit933)
5182a8d7a8 Remove unused variants from `Address::Error` (harshit933)
05b24946eb Add the `FromScriptError` for handling errors in `address` (harshit933)
Pull request description:
This commit adds the `FromScriptError` struct to handle the errors while generating address from any script. It includes :
- Unrecognized script error.
- Witness Program error.
- Witness Version error.
ACKs for top commit:
tcharding:
ACK c2d658ac05
apoelstra:
ACK c2d658ac05
Tree-SHA512: 891eed787129aaf1b664cc16d325178d5d2f77cc41a0543a3d9d1a5af1b58188daece1f6a653bdc6b76b82db0490a39e9bba7fc090e3727d15ee9b8977733698
ac88bc03fd Make constructors const (Tobin C. Harding)
Pull request description:
Audit the codebase for any function that starts with `/// Creates` and see if we can make it const. Inline them at the same time.
ACKs for top commit:
Kixunil:
ACK ac88bc03fd
apoelstra:
ACK ac88bc03fd
Tree-SHA512: 0c71e38018e74b3ce1aae871fc6208b87655a0970ae6cca7a538b9f896c95542c6905eb4d7e02539847e88d8a22a873039a5734130c2a46efb4d1b2b9ffd9f4a
Add a new `base58` crate to the workspace and move the `bitcoin::base58`
module to it.
Done as part of crate smashing, specifically so that we can make `bip32`
into a separate crate.
This commit adds the `FromScriptError` struct to handle the errors
while generating address from any script. It includes :
- Unrecognized script error.
- Witness Program error.
- Witness Version error.
We attempted to release with the current 0.1.0 version forgetting that
we had previously released an empty crate with that version to reserve
the name on crates.io.
Bump the version to 0.1.1 and release the actual code.
Fixes a gap in the API of the taproot module. Callers can now use
TapTree::root_hash or NodeInfo::node_hash to extract the taproot
tree merkle root hash for fast validation without any ECC overhead.
7e1ba7895f Remove broken kani test (Tobin C. Harding)
Pull request description:
This test is failing. I do not want to dive back into kani right now, just remove it.
This is what I originally did in #2454 but changed directions and tried to fix it. Running kani test takes ages and I'd need to dig back to refresh my memory to work with kani. I don't have the motivation to do that at the moment. Just remove the test.
FTR I added the test recently without fulling thinking it through and it has never passed so we are not loosing any coverage. Doing this was the original mistake I should not have made.
ACKs for top commit:
Kixunil:
ACK 7e1ba7895f
apoelstra:
ACK 7e1ba7895f
Tree-SHA512: cb76807173b637be9d5ce790b015e711ca76add95ce0f0acfdc56947c075f57ea89774c09c4314dbc89086dcf7a8e21053552bfae805fd5dc9c91051cd53c468
10cf51c4c5 Inline private ScriptBuf::p2wpkh function (Tobin C. Harding)
Pull request description:
This function is a bit unclear and is only called once, just inline it.
Refactor only, no logic changes.
ACKs for top commit:
apoelstra:
ACK 10cf51c4c5
Kixunil:
ACK 10cf51c4c5
Tree-SHA512: 3907923f2258089a5fc1cc1e1d0b34e99457d69a5822cefa7bf90405d7ac05d570fb2855f62e2b5b4b871485e349e8dc09eb8f14c0676a8bdd70593e345b9b41
d3d5ee1047 Improve error handling in errors emmited by `keys` (harshit933)
Pull request description:
For now I have tried to group those functions which can produce more than one error and changed the functions which were generating single error from `Key::Error` to the respective error. Let me know if this needs to be changed.
Also in `psbt/error.rs` I have changed the `InvalidPublicKey(crate::crypto:🔑:Error)` to `InvalidPublicKey(crate::crypto:🔑:FromSliceError)`. What should be done here?
Changes -
- in `from_slice` changed the `error` to `FromSliceError`.
- in `verify` changed to `secp256k1::Error` as it can return only one error.
- in `from_str` changed to `FromSliceError`.
- in `CompressedPublicKey` changed `verify` from `Error` to `secp236k1::Error` as it only returns one error.
- introduces CompressedPublicKeyError
- Removes impl from `bip32.rs`
Potential fix#2291
ACKs for top commit:
Kixunil:
ACK d3d5ee1047
tcharding:
ACK d3d5ee1047
Tree-SHA512: 21681bbf87c37eb0caaefe4b356a8a5e1d9b17de3207a0c9294de66b367ab348a7dda1916eb866fe4382e852af14ccab7b9f25a279291cd5beb56bb60b2523c2
ccbd09d5fb Remove unnecessary m/ prefix requirement (josibake)
Pull request description:
`m` in BIP0032 is a variable, not a constant. Requiring it as a constant here is confusing and can lead to erroneous conclusions if using this library as a means of understanding BIP0032.
Fixes#2449
ACKs for top commit:
Kixunil:
ACK ccbd09d5fb
apoelstra:
ACK ccbd09d5fb
Tree-SHA512: b641679f958f20a51c1890b23bbaa0153716802d6180dfd1f649e104f291c5a99143e02b75d292b22254201b28e5c53a04ecd7b6a88ff6f964073106419c5ec1
47569302fc Fix broken kani test (Tobin C. Harding)
Pull request description:
Recently we added a kani test that doesn't work because of `debug_assert` calls in ops traits.
Instead of opening the can of worms that is correct panic behaviour in ops lets just remove the test.
ACKs for top commit:
Kixunil:
ACK 47569302fc
apoelstra:
ACK 47569302fc
Tree-SHA512: f4a862d99173c1502e70fe4c2b9085a1f23dd4501f2ae25dc8a92e3edda7804b42b0580ef32fef2a3d5ea0d98e16b6f0fdba456cf4f0926c5b051ec8a6e54c78
In BIP0032, m is used as a variable for the root extended key. It is not
meant to be used as a constant prefix when serializing paths.
Update the DerivationPath parser to no longer require the m prefix.
Remove the m prefix from the unit tests and the bip32, ecdsa-psbt,
and taproot-psbt examples.
close#2449
Changes -
- in `from_slice` changed the `error` to `FromSliceError`.
- in `verify` changed to `secp256k1::Error` as it can return only one error.
- in `from_str` changed to `FromSliceError`.
- in `CompressedPublicKey` changed `verify` from `Error` to `secp236k1::Error` as it only returns one error.
- introduces CompressedPublicKeyError
- Removes impl from `bip32.rs`
- introduces `ParsePubKeyError` to return errors while generating publickey from string
3c62f74684 Add public functions p2wpkh_script_code (Tobin C. Harding)
a246dc98a4 Run sighash example in CI (Tobin C. Harding)
Pull request description:
This was done to fix#1920, it may be of questionable value though.
- Patch 1 is definitely useful, its a CI fix.
- Patch 2 adds two new API functions.
Fix: #1920
ACKs for top commit:
Kixunil:
ACK 3c62f74684
apoelstra:
ACK 3c62f74684
Tree-SHA512: 58743612c48e392f9ac0a94477588aee959c5fe9191dd04405bbb71aed7b0730b5927ad98f9da34dc93caaaac939617348c3f71318cc7e65c2c154b0f3897b89
c084afa8b2 Print hex in Debug for Sequence (Tobin C. Harding)
Pull request description:
Printing the `Sequence` as a decimal is not super useful when debugging, print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
ACKs for top commit:
Kixunil:
ACK c084afa8b2
apoelstra:
ACK c084afa8b2
Tree-SHA512: d60cd8896ca56a30fc8bd030cf3dd1bc1fd3a1609e99bfc2f26b9bd665b11c34c9df93b3f3ad731506d916513ca4a192dde476e16d99f2d4c4b2697f70a7bc98
Add two public API functions on the two public keys, both called
`p2wpkh_script_code` to do exactly as the name suggests.
Of note, I was not able to find anywhere to use these in example code,
this is because of we always use the new `p2wpkh_signature_hash`
function. The new functions may be useful for a user calling
`segwit_v0_encode_signing_data_to`. The may help document the library as
well.
Printing the `Sequence` as a decimal is not super useful when debugging,
print it in hex instead.
Using code:
let seq = Sequence::from_consensus(0xFFFFFFFF);
println!("sequence: {:?}", seq);
Before applying this patch we get:
sequence: Sequence(4294967295)
And after applying we get:
sequence: Sequence(0xffffffff)
Using `non_exhaustive` as well as a public inner field is incorrect, it
prohibits users from creating or matching on the error and does not
achieve forward comparability.
This was never right, we shouldn't have done it.
c69caafefc Remove attribute comments (Tobin C. Harding)
3e83ef9276 Remove consensus error wrapper TODO (Tobin C. Harding)
bfabea94e9 Remove unwrap comment (Tobin C. Harding)
8bdaf4a34d Remove carrying_mul TODO (Tobin C. Harding)
Pull request description:
Add issues and remove the TODOs from the code.
Resolves: #2368
ACKs for top commit:
apoelstra:
ACK c69caafefc
Kixunil:
ACK c69caafefc
Tree-SHA512: b10a3de8da7ace890735023f8441605dd11b0227c27a2357556b8aaa8276a7f34ed220e3bcbc93aad4b35357319318ff7de27210e8f60dd90f6c55af23e21470
Use `bash` instead of `sh` to run shell scripts.
We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
0997382772 io: Enable alloc from std (Tobin C. Harding)
ba1166a63b Make crate level attributes uniform (Tobin C. Harding)
Pull request description:
Make the trait level attributes uniform across all released crates in the repo. Excludes things that are obviously not needed, eg, bench stuff if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an open PR at the moment to add that along with the required docs.
ACKs for top commit:
apoelstra:
ACK 0997382772
Kixunil:
ACK 0997382772
Tree-SHA512: ef1f638aca171536287cce369be98998e871d26468ad2d8c39d9004db610b406471809c283540a4a19bcede78b12b8976a1bb37e5d431fbff8c8a3e53a64d4e3
The errors returned from various lock time functions had several issues.
Among the obvious - `Error` being returned from all operations even when
some of its variants were unreachable, there were subtle issues around
error messages:
* `ParseIntError` didn't contain information whether the parsed object
is `Height` or `Time`.
* Logically overflow and out-of-bounds should be the same thing but
produced different error messages.
* Mentioning integers is too technical for a user, talking about upper
and lower bound is easier to understand.
* When minus sign is present `std` reports it as invalid digit which is
less helpful than saying negative numbers are not allowed.
It is also possible that `ParseIntError` will need to be removed from
public API during crate smashing or stabilization, so avoiding it may be
better.
This commit significantly refactors the errors. It adds separate types
for parsing `Height` and `Time`. Notice that we don't compose them from
`ParseIntError` and `ConversionError` - that's not helpful because they
carry information that wouldn't be used when displaying which is
wasteful. Keeping errors small can be important.
It's also worth noting that exposing the inner representation could
cause confusion since the same thing: out of bounds can be represented
as an overflow or as a conversion error. So for now we conservatively
hide the details and even pretend there's no `source` in case of
overflow. This can be expanded in the future if needed.
The returned errors are now minimal. `LockTime` parsing errors are
currentlly unchanged.
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
The code originally used `if` and incorrectly casted the value into
`usize` rather than `u64`. This change replaces the whole thing with
`unsigned_abs`.
Closes#1247
6ddb5cce37 Use Magic::BITCOIN in unit tests (Tobin C. Harding)
Pull request description:
We are currently calling `From` to create the magic bytes, this is unnecessary since `Magic` provides consts.
Refactor only, no logic changes.
ACKs for top commit:
Kixunil:
ACK 6ddb5cce37
apoelstra:
ACK 6ddb5cce37
Tree-SHA512: 20e2e017683f123309e3c0876bba42d86a9411bb225f07c486716184fc79837e04a832338ec8b18874ac76791260f6a4620b932ede92c8b222dac08d468cef8a
5eb2de1660 Remove TODO about rand trait (Tobin C. Harding)
66cc007c2b p2p: Remove TODO comments (Tobin C. Harding)
0b5fb45ea0 consensus: Remove HEX_BUF_SIZE todo (Tobin C. Harding)
579668892a consensus: Remove TODO (Tobin C. Harding)
53beb9db30 Remove ancient todos in test code (Tobin C. Harding)
abe2241828 units: Remove "alloc" TODO (Tobin C. Harding)
5386ef0fd2 psbt: Delete TODO comments (Tobin C. Harding)
14c8a2232b examples: Remove TODO (Tobin C. Harding)
Pull request description:
Done while working on #2368. There are 5 left. Do we want to leave the MSRV ones in there?
```bash
bitcoin/src/blockdata/weight.rs:66: // TODO replace with panic!() when MSRV = 1.57+
bitcoin/src/consensus/serde.rs:101: // TODO: statically prove impossible cases
bitcoin/src/pow.rs:445: // TODO: Use `carrying_mul` when stabilized: https://github.com/rust-lang/rust/issues/85532
units/src/amount.rs:595: // TODO replace whith unwrap() when available in const context.
units/src/amount.rs:599: // TODO replace with panic!() when MSRV = 1.57+
```
ACKs for top commit:
Kixunil:
ACK 5eb2de1660
apoelstra:
ACK 5eb2de1660
Tree-SHA512: 285b1711a6e6fba126e2c4159b25454c7f894122b76fde1d3d29e57b2ec0a6e90230e46ac79d70aa133da177c75d267fc5a13489b69881862649de771027ec8e
6715e93e89 Add Witness::p2tr_key_spend function (Tobin C. Harding)
Pull request description:
Add a function for creating the witness when doing a key path spend for a P2TR output.
This mirrors what we did for P2WPKH when adding `Witness::p2wpkh`.
Includes update to the taproot signing example to use the new constructor.
ACKs for top commit:
Kixunil:
ACK 6715e93e89
apoelstra:
ACK 6715e93e89
Tree-SHA512: aab51329e8fda471442bb9cebd6327636548dd157bb9842fe66993fcdd211bb04b2b829aa9d5962dd619f5c0b73d19644a44529c1a5958df1a6bc892147b44f5
Tobin C. Harding
Andrew Poelstra
harshit933
conduition
yancy
josibake
Martin Habovstiak