Add MessageSignature type for dealing with signed messages #413
Conversation
674f5e1 to
a501365
Compare
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
==========================================
+ Coverage 85.81% 85.83% +0.01%
==========================================
Files 40 40
Lines 7461 7575 +114
==========================================
+ Hits 6403 6502 +99
- Misses 1058 1073 +15
Continue to review full report at Codecov.
|
a501365 to
c94189e
Compare
|
concept ACK, would be nice to have this out of the box |
|
Too bad you didn't make https://github.com/stevenroose/rust-base64-compat as a fork so we can easily see what's changed and can also easily see changes in upstream. for anyone who wants to review the changes this is the only commit with logic change: stevenroose/rust-base64-compat@ae7de46 |
It is a fork. It's a Git fork, not a GitHub fork. The Git history is preserved.. |
e3a90c7 to
0026764
Compare
There was a problem hiding this comment.
utACK the code
concept ACK adding the dependency. cc @TheBlueMatt are you ok with this? it is Steven's base64 compat library.
|
Ah, for a second I was about to scream. His fork seems reasonable, though it would be nice to pull it into the org if we're gonna depend on it. Independently, given its for a feature not all users are going to use, is there any real cost to making it optional? |
|
@TheBlueMatt @elichai I made the base64-compat crate opt-out with a base64 feature. I also exported the base64 crate (for easier usage of the base64 error type). Let me know if that's ok for you. |
94fad7a to
4318dc2
Compare
af03df2 to
6dee624
Compare
|
I noticed we still need https://github.com/stevenroose/rust-base64-compat because rust-base64 has MSRV 1.34.0 but since rust-bitcoin bumped to 1.29.0 things like |
@RCasatta yeah plus the Does it matter much to implement them manually or use stdlib? Do you think the stdlib variants have some better compiler optimizations perhaps? Otherwise I wouldn't mind keeping |
|
I'm ok with pulling the base64-compat crate into the org, though, as @TheBlueMatt suggested. Though ideally the crate would never be updated and we can fix the version with |
Didn't know the other problem on |
|
See my comment #330 (comment) .. I think we shouldn't feature-gate the enum variants if we can avoid it. A couple options would be
|
|
@apoelstra Why does this PR have |
0de9238 to
4b19d23
Compare
I think because of which is a feature-gated enum variant and @apoelstra said they could break compatibility in #330 (comment) |
| fuzztarget = ["secp256k1/fuzztarget", "bitcoin_hashes/fuzztarget"] | ||
| unstable = [] | ||
| rand = ["secp256k1/rand-std"] | ||
| use-serde = ["serde", "bitcoin_hashes/serde", "secp256k1/serde"] | ||
| secp-recovery = ["secp256k1/recovery"] |
There was a problem hiding this comment.
This is a breaking change, you can add that to default features, but removing a feature is a breaking change
There was a problem hiding this comment.
Not sure if this has been released yet, @apoelstra ? It's only recently added by @RCasatta .
There was a problem hiding this comment.
I mean we can still release the secp-recovery feature in 0.25.1 and then remove it in 0.26.0? Making it a default feature means that all the code in this PR needs to be feature-gated.
There was a problem hiding this comment.
@apoelstra says that it wasn't yet released, so you can restore this back? sorry.
I'm not sure if people still want this to be feature gated or not, personally I'm fine with it not feature gated.
4b19d23 to
df75ae0
Compare
|
I don't remember why I added the Regarding @elichai's comment about deleting a feature, yeah, that would be breaking except that #486 has not been released. So this is fine to go into 0.25.1. |
|
@elichai noted that it's a breaking change that the |
df75ae0 to
f7422fb
Compare
|
Added back the feature. |
There was a problem hiding this comment.
Left one comment, other than that verified that it matches the logic by bitcoin core: https://github.com/bitcoin/bitcoin/blob/61b8c04d78fb116e1722659ac455ad27856c6604/src/pubkey.cpp#L187
and by my own tool :) https://github.com/elichai/bitsign/blob/master/src/main.rs#L155
src/util/misc.rs
Outdated
| Some(AddressType::P2wpkh) => { | ||
| // Only compressed pubkeys are allowed in p2wpkh. | ||
| pubkey.compressed && | ||
| *address == Address::p2wpkh(&pubkey, address.network).expect("compressed pk") | ||
| } |
There was a problem hiding this comment.
This is not aligned with bitcoin-core AFAICT
https://github.com/bitcoin/bitcoin/blob/a12d9e5fd24a25bef476c10620317e43a5905754/src/util/message.cpp#L34
There was a problem hiding this comment.
Interesting. It looks like Core will only run verifymessage on p2pkh addresses. I think we should replicate this @stevenroose and implement BIP322 in a separate PR for new-style addresses.
9c4d2ef to
3f65fb1
Compare
I do realize that adding a dependency might be controversial. Also, if anyone knows a better base64 crate, please let me know. The one I used is one I created some months ago based on the
base64crate but making it compatible with Rust v1.19.0, something they refused to do.