6ecc41d126 Return error when constructing pubkey from slice (Tobin C. Harding)
Pull request description:
This PR fixes a bug introduced by me in #2473, and uncovered by #2563 - amazing that it was found so quickly!
Constructing a pubkey using `PublicKey::from_slice` can fail for reasons other than just incorrect length - we should not be using `expect` but rather returning the error.
A purist might argue that we are now returning a nested error type with an unreachable variant:
`ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)`
Is this acceptable or do we want to further improve this?
ACKs for top commit:
sanket1729:
ACK 6ecc41d126
apoelstra:
ACK 6ecc41d126
Tree-SHA512: ae8299b21c4787a104f98533105308e8e7678cd5a29b78c30012982d741c05ba5f2bb1edd1d61d3a5ce028235d18c1511e1f94207479bc19e88cfec7a7ca1737
56132f59d5 Remove the `:#` formatting for `hex_fmt_impl` macro (448 OG)
Pull request description:
This commit attempts to solve #2505 by ensuring that formatting is not forced using the `:#` in the hex macro code generating in macro rule `hex_fmt_impl` in the hashes/utils.rs file.
The write! macro forces all formatting to add the prefix `0x` by adding an alternate by (#) default
```rust
impl<$($gen: $gent),*> $crate::_export::_core::fmt::Debug for $ty<$($gen),*> {
#[inline]
fn fmt(&self, f: &mut $crate::_export::_core::fmt::Formatter) -> $crate::_export::_core::fmt::Result {
write!(f, "{:#}", self) // <-- This is where the formatting is being forced.
}
}
```
By removing this formatting, the `:#` must be specified by the user in order for a prefix to be added.
```rust
let outpoint = bitcoin::OutPoint::default();
println!("{:?}", &outpoint);
println!("{:#?}", &outpoint);
println!("{:#}", &outpoint);
println!("{:x}", &outpoint.txid);
// `{:#}` must be specified to pretty print with a prefix
println!("{:#}", &outpoint.txid);
dbg!(&outpoint);
dbg!(&outpoint.txid);
```
The PR also adds testcase for this when running `cargo test` .
ACKs for top commit:
tcharding:
ACK 56132f59d5
apoelstra:
ACK 56132f59d5
Tree-SHA512: 9e4fc9f30ab0b3cf2651d3c09f7f01d8245ac8ea7ae3a82bb4efd19f25c77662bf279020a31fa61b37587cc0c74284696c56045c59f1ba63b2dd42a210d98ebc
f8de7954b2 Remove unused pow::TryFromError type (Tobin C. Harding)
43c5eb765c Fix witness_version leaf error type (Tobin C. Harding)
2af764e859 hashes: Fix leaf error type (Tobin C. Harding)
Pull request description:
In light of recent discussion go over the codebase and look for some places that the leaf errors are wrong. Does not do the whole code base, excludes `p2p` and a couple of other places.
ACKs for top commit:
apoelstra:
ACK f8de7954b2
Kixunil:
ACK f8de7954b2
Tree-SHA512: 2905878363869ee205cce49c58c060c712c9b7b55965ee60bb856128842968a4be86c93a194ffffdb35e215b2bea8ad33b04ee47e8e17cc784b0641ea48518e5
Constructing a pubkey using `PublicKey::from_slice` can fail for reasons
other than just incorrect length - we should not be using `expect` but
rather returning the error.
A purist might argue that we are now returning a nested error type with
an unreachable variant:
`ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)`
Is this acceptable or do we want to further improve this?
This fixes the issue where pretty debug like `dbg` or `{:#}` introduce the use of
`0x` prefix to hex encoded transaction ID.
The transaction id is being forced to pretty print inside the `hex_fmt_impl` macro
using `{:#}` in the line `write!(f, "{:#}", self)` debug formatter.
Resolves: #2505
f337dec2b1 hashes: Remove unnecessary feature guard from test (Tobin C. Harding)
0cea90d505 Test hashes honour Formatter::precision (Tobin C. Harding)
4bfb466bb9 Upgrade hex dependency (Tobin C. Harding)
f0558e8eb9 Use fmt_hex_exact (Tobin C. Harding)
6820f51408 hashes: Add fmt roundtrip tests (Tobin C. Harding)
e302e30e7c Import with super::* in unit test (Tobin C. Harding)
Pull request description:
Upgrade to use the newly released `hex` code.
- Patch 1: Does trivial preparatory cleanup
- Patch 2: Adds some unit tests to check we roundtrip hashes correctly (added because in the test PR I had the `Midstate` iml wrong and it was not being caught).
- Patch 3: Uses macro in place of `forward_hex` and `backward_hex` - needs concept review, I hacked this without understanding why the functions existed in the first place.
- Patch 4: Does the upgrade, I've attempted to make minimal changes, so there is room for a bunch of cleanups if/when this merges.
- Patch 5: Adds a unit test to verify that we can close#2494
- Patch 6: Removes unnecessary feature gate from unit test.
ACKs for top commit:
Kixunil:
ACK f337dec2b1
apoelstra:
ACK f337dec2b1
Tree-SHA512: 7913d1b3079cf5ba1b0e70f5c33e091c5ef1258026c8f27bbe8a050100bbc7622b6555d560b15be3b3d90d47ce873f137a73cf2d772108d2915fb30ed129bded
3c8edae25b Split relative locktime error up (Tobin C. Harding)
Pull request description:
The `relative` module has a single general error type, we are moving away from this style to specific error types.
Split the `relative::Error` up into three error structs.
I forget the policy on public inner fields.
ACKs for top commit:
sanket1729:
utACK 3c8edae25b
apoelstra:
ACK 3c8edae25b
Tree-SHA512: f3079f81a825125f1efe54657fbba64618530b25aecaa3844902900517bf23bec26ff5399cf22f4e63e44316ebb603e8692cbaece2782ecafe09ffed3eab553c
Test that the new version of `hex` honours `Formatter::precision` for
new wrapped hash types (ie, types created with `hashes::hash_newtype`).
Fix: #2494
08a9962035 Replaced Deprecated Function (Sh0g0-1758)
Pull request description:
Changed deprecated Function with a supported one.
ACKs for top commit:
apoelstra:
ACK 08a9962035 yep, this seems reasonable. Thanks!
tcharding:
ACK 08a9962035
Tree-SHA512: fd0a55ab25cd15a3254a6c353e4906b4f4c74175d648e1f532be8b8642490e5187b96aa0aa7365771016e10a481054d3377b9074b93cd001da515177315b0d92
The `relative` module has a single general error type, we are moving
away from this style to specific error types.
Split the `relative::Error` up into three error structs.
Note the change of parameter `h` to `height`, and using `h` as the
pattern matched variable - this makes sense because it gives the
variable with large scope the longer name.
3a56ecc677 Add consts to Params for individual networks (Tobin C. Harding)
Pull request description:
Add consts to the `Params` type for the individual networks.
ACKs for top commit:
apoelstra:
ACK 3a56ecc677
Kixunil:
ACK 3a56ecc677
sanket1729:
ACK 3a56ecc677
Tree-SHA512: 0d265a14dd6a591a267da5381d3dcfd0d313f950dec4922f96d25349047d0c8a366c41dcdc1fc523fe4b178ec6a00b717bda25286625e222194f345cee5e7a97
ec67456172 Fix CJDNS marker byte check (Ava Chow)
Pull request description:
Only the first byte of a CJDNS address is 0xfc, the second byte should be ignored.
See https://github.com/hyperboria/peers for examples of CJDNS addresses.
ACKs for top commit:
apoelstra:
ACK ec67456172
sanket1729:
urACK ec67456172.
Kixunil:
ACK ec67456172
Tree-SHA512: 0da1054a8e997b6bf6e0aaedc40943edb6a6c7b23f660c92b34dc9395e6153e7e10b0268335a77fbcb5bb635352e1ed92839b350188fd6d33dabe558e88c00bb
b873a3cd44 Do infallible int from hex conversions (Tobin C. Harding)
4d762cb08c Remove the FromHexStr trait (Tobin C. Harding)
026537807f Remove mention of packed (Tobin C. Harding)
Pull request description:
The `FromHexStr` trait is used to parse integer-like types, however we can achieve the same using inherent methods.
Move the hex parsing functionality to inherent methods, keeping the same behaviour in regard to the `0x` prefix.
Patch 1 is trivial preparatory cleanup.
ACKs for top commit:
apoelstra:
ACK b873a3cd44
sanket1729:
ACK b873a3cd44
Tree-SHA512: a280169b68304fcc1a531cc9ffb6914b70238efc4c2241a766105053911a373a0334b73e5ea3525c331ccb81ce98c43fea96dae77668804e608376a48d5ed8ac
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
We have three integer wrapping types that can be created from hex
strings where the conversion from an integer is infallible:
- `absolute::LockTime`
- `Sequence`
- `CompactTarget`
We would like to improve our handling of the two prefix characters (eg
0x) by making it explicit.
- Modify the inherent `from_hex` method on each type to error if the
input string does not contain a prefix.
- Add an additional inherent method on each type `from_unprefixed_hex`
that errors if the input string does contain a prefix.
This patch does not touch the wrapper types that cannot be infallibly
constructed from an integer (i.e. absolute `Height` and `Time`).
The `FromHexStr` trait is used to parse integer-like types, however we
can achieve the same using inherent methods.
Move the hex parsing functionality to inherent methods, keeping the same
behaviour in regard to the `0x` prefix.
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.
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
Andrew Poelstra
Tobin C. Harding
448 OG
Sh0g0-1758
Ava Chow
Liam Aharon
geekvest
harshit933
conduition
yancy