More complete `MessageSignature.is_signed_by_address` #684

Shatnerz · 2021-10-28T18:25:30Z

I noticed that the current version only seems to support p2pkh and all other address types default to false with no error thrown.

I effectively just ported this piece from bitcoinjs-message: https://github.com/bitcoinjs/bitcoinjs-message/blob/c43430f4c03c292c719e7801e425d887cbdf7464/index.js#L181-L233

I'm fairly new to rust, but I'm open to all suggestions to learn and to get this merged.

Testing

I included unit tests for

p2pkh
p2wpkh
p2shwph

I also amended the test_message_signature test which was asserting false on values that should clearly be true, but were just returning false due to lack of support.

revocery_id is for convenience. segewit_type will aid with signature verification.

I noticed that only 1 address type was supported so this is an attempt to make signature vreification less specific. Mostly ported from: https://github.com/bitcoinjs/bitcoinjs-message/blob/c43430f4c03c292c719e7801e425d887cbdf7464/index.js#L181-L233

Makes more sense with how it is being used.

tcharding

Thanks for the PR! I spent a bunch of time reviewing this one for you but I'm concerned I might have come across as overly picky. I'm not a core rust-bitcoin dev in any way so this review is more stylistic than reviewing the logic. Normally I wouldn't do this until the logic was reviewed by others but since you said you were new to Rust I thought I'd take the chance to help you and help myself get deeper into rust-bitcoin also. I hope this helps.

FTR I can confirm that cargo +1.36 test --features=secp-recovery,base64 test_message_signature fails with the minimal diff below. (Which is what this PR fixes).

diff --git a/src/util/misc.rs b/src/util/misc.rs
index 553fa6a..9657a9c 100644
--- a/src/util/misc.rs
+++ b/src/util/misc.rs
@@ -333,9 +333,9 @@ mod tests {
let p2pkh = ::Address::p2pkh(&pubkey, ::Network::Bitcoin);
assert_eq!(signature2.is_signed_by_address(&secp, &p2pkh, msg_hash), Ok(true));
let p2wpkh = ::Address::p2wpkh(&pubkey, ::Network::Bitcoin).unwrap();
-        assert_eq!(signature2.is_signed_by_address(&secp, &p2wpkh, msg_hash), Ok(false));
+        assert_eq!(signature2.is_signed_by_address(&secp, &p2wpkh, msg_hash), Ok(true));
let p2shwpkh = ::Address::p2shwpkh(&pubkey, ::Network::Bitcoin).unwrap();
-        assert_eq!(signature2.is_signed_by_address(&secp, &p2shwpkh, msg_hash), Ok(false));
+        assert_eq!(signature2.is_signed_by_address(&secp, &p2shwpkh, msg_hash), Ok(true));
}
}

src/util/misc.rs

tcharding · 2021-10-28T21:24:20Z

src/util/misc.rs

+        /// Segwit type realted to this signature
+        pub segwit_type: Option<SegwitType>,
+        /// Recovery Id for easy access
+        pub recovery_id: RecoveryId,


Perhaps pub recid: RecoveryId would be equally as descriptive?

recid sounds like something to do with receiving. recovery_id is a great name.

I kept as recovery_id but I was using recid in a few places. In 5ea6b6a I made it so I consistently call it recovery_id

src/util/misc.rs

tcharding · 2021-10-28T21:31:50Z

src/util/misc.rs

-            })
+
+            // Use .serialize because we always want compact serialization
+            let pubkey_hash = super::hash160(&pubkey.key.serialize().to_vec()).to_vec();


Rust note, usage of super is, as far as I understand, typically restricted to test modules (use super::*) or cases where it really makes sense [1]. Use here is likely to make moving this code harder, so makes the code less maintainable. This might be just personal opinion but I'd prefer to see util::hash160 here and a use statement use crate::util. Just mentioning since you said you were newish to Rust, hope this is helpful :)

For example the import you add in this PR that brings functions from outside this module, but in this file, into scope seems like a sane usage to me.

Interesting, I was never under the impression that super:: is restricted to anything despite being quite long and deep in Rust. Maybe I missed it?

Also now that I had a bit of time to look at it better, I noticed .to_vec(). I think with a bit of effort the allocation can be avoided if hash160 accepts any reader.

Also I'm not convinced that the latter to_vec() is needed.

@tcharding I'm going to go with

use crate::util::misc::{bech32_decode, segwit_redeem_hash, hash160, get_payload_bytes};

and remove the use of super outside of the tests. Let me know if that is sufficient.

Will look into to_vec() usage shortly

Resolved super usage in 5ea6b6a

Will need to look at it better to write the exact code (hopefully tomorrow) but generally, hasher can be considered a writer and data can be considered reader.

The latter to_vec shouldn't be needed because you only need slice later and borrowing rules allow it. (Unless I'm missing something very subtle - LMK if you get a compile error.)

Actually, serialize() produces Vec<u8> this is not great but proves to_vec is not needed because it's essentially a clone().

That link is PublicKey.serialize which is easily confused with PublicKey.key.serialize() (and PublicKey.key.serialize_uncompressed()). PublickKey.keyis a secpk256k1::PublicKey. Removing the to_vec() in &pubkey.key.serialize().to_vec() results in

error[E0308]: mismatched types --> src/util/misc.rs:200:39 | 200 | let pubkey_hash = hash160(&pubkey.key.serialize()).to_vec(); | ^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::vec::Vec`, found array `[u8; 33]` | = note: expected reference `&std::vec::Vec<u8>` found reference `&[u8; 33]` error: aborting due to previous error

Implemented it here: Kixunil@858c83f (I can't make a PR to your branch, no clue why).

is easily confused with PublicKey.key.serialize()

Oh, that's even better as there's no Vec, so no more work needed to remove the allocation!

Nice! Thanks. This is great to see.

I can't make a PR to your branch, no clue why
Probably a permission issue on my fork? Anyway I manage to create a PR and merge it (Shatnerz#1)

src/util/misc.rs

tcharding · 2021-10-28T22:32:06Z

src/util/misc.rs

+        assert_eq!(signature2.is_signed_by_address(&secp, &p2shwpkh, msg_hash), Ok(true));
+    }
+
+    mod is_signed_by_address {


Great to see you adding unit tests, and please don't take this the wrong way; what are these tests testing that is not already tested above in test_message_signature?

It would have been super nice if the changes above to test_message_signature were in a separate patch after the patch that does the work. Then reviewers could quickly re-arrange the patches and run the tests to see them fail. Admittedly this might be an overkill in this example because its trivial to just change the true to false myself but thought I'd note it for your future patches!

Honestly, I don't think they technically add that much. They just happened to be values that I had on hand that I new were accurate and should work. I also like that they are each separate tests and every signature type isn't forced into single unit test. Imo it helps debug when something inevitably breaks the tests.

I'm willing to change or remove anything related to my tests here. It just helped me get off the ground and test my changes as I went along.

Yes, having them split up is a good thing. Didn't review so much that I could comment on anything else.

I definitely like to see one test one test function :)

src/util/misc.rs

Shatnerz · 2021-10-29T07:16:04Z

@tcharding This is fantastic feedback. Thanks. I will try to resolve within the next 72 hours. Hopefully today

src/util/misc.rs

@tcharding

Changes: - Change `from_flag_bytes` response to just an Option - `recid` -> `recovery_id` - remove superfluous parentheses - remove usage of `super` outside of testing - cleaner if/else usage in `bech32_from_words` - remoe nested import Thank you @tcharding and @Kixunil for the suggestions

@tcharding

Changes: - Change `from_flag_bytes` response to just an Option - `recid` -> `recovery_id` - remove superfluous parentheses - remove usage of `super` outside of testing - cleaner if/else usage in `bech32_from_words` - remoe nested import - use checked_sub when setting flag_byte Thank you @tcharding and @Kixunil for the suggestions

Shatnerz · 2021-10-31T12:46:45Z

Addressed all feedback as of this comment. I closed threads on anything with direct actionable feedback. Hopefully that's not a faux pas.

@Kixunil

@Kixunil's suggestion

@tcharding

as suggested by @tcharding

Note: Did not fix the existing minor violations

Shatnerz · 2021-11-01T10:13:05Z

./contrib/test.sh now passes locally after f613214

This removes a bunch of `to_vec()` calls elliminating allocations and simplifies the implementation of `hash160` helper function by using hash from `bitcoin_hashes`.

Remove needless allocations and simplify the code

Shatnerz · 2021-11-02T19:29:35Z

I can't seem to reproduce this CI result locally: https://github.com/rust-bitcoin/rust-bitcoin/runs/4081690675?check_suite_focus=true

Can anyone offer assistance so I fixing this doesn't require such long cycle times? I assume the 1.29.0 is the rust version. I tried rustup default 1.29.0 and then rerunning but then I get

error: failed to parse lock file at: /home/shatnerz/dev/rust/rust-bitcoin/Cargo.lock

Caused by:
  invalid serialized PackageId for key `package.dependencies`

edit: pushed attempted fix. I'll go through the workflow file when I have a chance and make sure I can recreate the CI results

- accessing extern crates through prelude - use of "crate" in path

Shatnerz · 2021-11-14T15:39:17Z

If there is ever indication that this might get reviewed, I can also start looking into a follow up MR to support taproot addresses when using is_signed_by_address

dr-orlovsky · 2021-11-14T16:10:13Z

Sorry for leaving this PR for some time w/o review; all maintainers are currently focused on completing Taproot support as highest priority. I will be able to review this PR as soon as Taproot release is out and I can read the specs on how the message signing should be done. But for sure I'd like to have this functionality into the library - especially with Taproot follow-up

Shatnerz · 2021-11-14T16:14:03Z

@dr-orlovsky sounds good and no worries.I forgot that the taproot launch was so soon (lol days ago now). Hopefully I can get some time to look into taproot signature verification

Existing bit manipulation was off. Changed 4 -> 12

Shatnerz · 2022-01-11T15:09:26Z

@apoelstra

Sorry for being late, but AFAIK there is no standard for message signatures using wpkh or p2shwpkh addresses. The closest thing is BIP322, which is not implemented anywhere.

I don't quite understand this comment. Does that actively change anything about verifying a signature using an address?

Are you referring to a specific piece of code. Sorry some time has passed so I may need a refresher.

apoelstra · 2022-01-11T15:54:23Z

I don't quite understand this comment. Does that actively change anything about verifying a signature using an address?

No, nothing has changed about verifying a siganture using an address since it was implemented in Bitcoin Core in 2011.

@tcharding

Changes: - Change `from_flag_bytes` response to just an Option - `recid` -> `recovery_id` - remove superfluous parentheses - remove usage of `super` outside of testing - cleaner if/else usage in `bech32_from_words` - remoe nested import - use checked_sub when setting flag_byte Thank you @tcharding and @Kixunil for the suggestions

Do a bunch of refactorings while playing around with the original PR. ref: rust-bitcoin#684

tcharding

Thanks for your patience. I had another play around with this. I did a bunch of minor changes but I don't want to derail your PR with too fine a review before some of the other lads, with more knowledge than myself, do their higher level reviews. I pushed a branch [1] with the same name to my own fork with a single refactoring patch that you can look at if it amuses you. My main contribution was refactoring all the unit tests into small concise tests (incl. helper functions), feel free to use any, or none, of my ideas as your own if you wish. Apart from that there was a few re-names (that are kind of personal preference) and a few Rust/stlylistic things).

In future, it would probably be easier to review if there were less patches and later patches didn't make changes to earlier patches. Instead rebase and squash makes reviewing easier. I know this one has been open a long time so don't feel obliged to do that here, I just wanted to mention it. Thanks for your effort, its super appreciated.

[1] https://github.com/tcharding/rust-bitcoin/tree/feat/proper-is-signed-by-address

tcharding · 2022-01-12T01:37:48Z

src/util/misc.rs

+}
+
+/// Convert u5 vec into u8 vec
+fn bech32_from_words(words: &[bech32::u5]) -> Result<Vec<u8>, Bech32DecodingError> {


This whole function is already provided by bech32 library. Function implementation can be replaced by

bech32::convert_bits(words, 5, 8, true).map_err(|e| Bech32DecodingError::InvalidEncoding(e))

EDIT: the map_err is just to make it build, you may want to do something different.

Thank you, good catch

apoelstra · 2022-01-12T18:05:10Z

concept NACK this PR. There is no signmessage standard for non-pkh address types.

Kixunil · 2022-01-13T13:28:49Z

Agree with @apoelstra on concept NACK but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address. The former could reduce foot-gunnyness of it and the latter would enable people to conveniently build a new signing standard separately and iterate on it for a while.

dr-orlovsky · 2022-01-13T17:44:54Z

When I get funds, some non-profits are asking me to sign with my address - and I am hesitant to move to the old P2PKH just because there is not standard to sign with an address for P2WPKH or P2TR. If there is no standard for that, let's make it. @Shatnerz you are welcome to do this contribution to a descriptor-wallet library (https://github.com/LNP-BP/descriptor-wallet) extending rust-bitcoin for wallet applications - and also propose a new standard to LNPBPs set (https://github.com/LNP-BP/LNPBPs)

Kixunil · 2022-01-13T22:12:54Z

Yes, let's make it! I think this is BIP-worthy.

sanket1729 · 2022-01-13T22:30:24Z

Have you looked at BIP-322? bitcoin/bitcoin#24058

Kixunil · 2022-01-13T23:22:06Z

I would ACK a correctly written implementation of BIP-322 or, preferably, required additional methods to make it implementable in a separate crate. We need to stop the scope creep.

dr-orlovsky · 2022-01-14T09:53:28Z

Also found this: rust-bitcoin/rust-miniscript#240

Shatnerz · 2022-01-17T02:36:47Z

Sorry guys. The timing is unfortunate because I'm incredibly busy this month. I'll try to review the above comments more closely when I get a chance.

It looks like a negative response to this PR. I'm probably missing something at a glance. Comments seem to talk about signing when this PR only affects verification.

Do you have any suggestions on fixing is_signed_by_address for P2SH, P2WPKH, and P2WSH as currently they are a tad misleading as they always return false (https://github.com/rust-bitcoin/rust-bitcoin/blob/master/src/util/misc.rs#L166-L174) in a manner that will be accepted? Aside: I don't think this will work for P2TR unless a pubkey is provided as well. I have some ideas for P2TR but would like to see other addresses handled properly first.

Shatnerz · 2022-02-05T12:28:30Z

I'm just closing due to inactivity.

Feel free to reach out if you ever need a hand expanding MessageSignature support.

Inspired by this comment: rust-bitcoin#684 (comment)

tcharding · 2022-02-07T08:35:08Z

Thanks for showing an interest in this @Shatnerz, I look forward to your future PRs!

Inspired by this comment: rust-bitcoin#684 (comment)

…_address()` 79cee4c fix: Error on unsuported addresses in `is_signed_by_address` (Andrew Ahlers) Pull request description: This is a direct response to this comment: #684 (comment) Currently unsupported addresses simply return `false` which is highly misleading as it is entirely possible that the keys related to a given address were used to sign the message. If this is successful I can follow up with > a method to check if a PublicKey is associated with an address ACKs for top commit: apoelstra: ACK 79cee4c Kixunil: ACK 79cee4c Tree-SHA512: 0c2f15696c63272e8ad1ecc0959c828f2d6c4aa16a7d099235c3f6bd287a0c20e034752331644c4bcc3af61ba4d739ad6795cbcea61c9e615c1eb7b43ebf0eeb

51fef76 feat: Add Address.is_related_to_pubkey() (Andrew Ahlers) Pull request description: ## Motivation This is addressing the second half of this comment: #684 (comment) > but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address. (The first half was addressed [here](#819)) These changes will help build out and improve message signature verification. We don't necessarily need to add it to this crate but it allows for easy verification with something such as: 1. recovering a pubkey 2. checking if that pubkey relates to the given address ## Possible Improvements - There is likely a better name than `is_related_to_secp256k1_key()` - This could drop the `secp256k1` part of the name and take in a Pubkey enum that also supports Schnorr pubkeys and then this could be used for taproot addresses as well. This felt like a much larger change that will likely get turned down. Verifying taproot is simple enough and if absolutely desired, similar functions can be added for schnorr keys (tweaked and untweaked) ACKs for top commit: Kixunil: ACK 51fef76 for merging after TR apoelstra: ACK 51fef76 Tree-SHA512: c9ab8c0f101fb4c647713e7f500656617025d8741676e8eb8a3132009dde9937d50cf9ac3d8055feb14452324a292397e46639cbaca71cac77af4b06dc42d09d

feat: Add segwit_type and recover_id to MessageSignature

bc30c77

revocery_id is for convenience. segewit_type will aid with signature verification.

Shatnerz mentioned this pull request Oct 28, 2021

WIP: feat: Add support for p2wpkh in MessageSignature.is_signed_by_address() #682

Closed

feat: Improve is_signed_by_address()

4f06288

I noticed that only 1 address type was supported so this is an attempt to make signature vreification less specific. Mostly ported from: https://github.com/bitcoinjs/bitcoinjs-message/blob/c43430f4c03c292c719e7801e425d887cbdf7464/index.js#L181-L233

Shatnerz force-pushed the feat/proper-is-signed-by-address branch from 358f8d9 to 4f06288 Compare October 28, 2021 18:29

Shatnerz added 2 commits October 28, 2021 23:13

fix: Minor fix to base58_check_decode

a826540

refactor: Rename base58_check_decode to get_payload_bytes

0545340

Makes more sense with how it is being used.

tcharding reviewed Oct 28, 2021

View reviewed changes

Kixunil reviewed Oct 29, 2021

View reviewed changes

src/util/misc.rs Outdated Show resolved Hide resolved

Shatnerz force-pushed the feat/proper-is-signed-by-address branch from 37d3cd8 to 5ea6b6a Compare October 31, 2021 12:36

fix: Remove superfluous u8 int check

368c506

@Kixunil's suggestion

Shatnerz force-pushed the feat/proper-is-signed-by-address branch from b2f2564 to 368c506 Compare October 31, 2021 22:09

Shatnerz added 4 commits November 1, 2021 00:11

chore: Use explicit match

623f8b8

as suggested by @tcharding

style: Missing newline

5679675

chore: Satisfy clippy on is_signed_by_address changes

5800506

Note: Did not fix the existing minor violations

fix: ./contrib/test.sh now passes

f613214

Kixunil and others added 3 commits November 1, 2021 12:22

Remove needless allocations and simplify the code

858c83f

This removes a bunch of `to_vec()` calls elliminating allocations and simplifies the implementation of `hash160` helper function by using hash from `bitcoin_hashes`.

Merge pull request #1 from Kixunil/feat/proper-is-signed-by-address

fc6579a

Remove needless allocations and simplify the code

chore: Bump minor version to 0.27.1

8c9a17d

fix: Remove rust 1.29 experimetnal features

4314c43

- accessing extern crates through prelude - use of "crate" in path

Shatnerz force-pushed the feat/proper-is-signed-by-address branch from c6565b8 to 4314c43 Compare November 3, 2021 16:39

fix: Correctly set MessageSignature.compressed

425c8e7

Existing bit manipulation was off. Changed 4 -> 12

tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Jan 12, 2022

Refactor

9579389

Do a bunch of refactorings while playing around with the original PR. ref: rust-bitcoin#684

tcharding reviewed Jan 12, 2022

View reviewed changes

Shatnerz closed this Feb 5, 2022

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 5, 2022

fix: Error on unsuported addresses in is_signed_by_address

70dc6e8

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz mentioned this pull request Feb 5, 2022

fix: Throw Error on unsuported addresses in is_signed_by_address() #819

Merged

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 5, 2022

fix: Error on unsuported addresses in is_signed_by_address

df20813

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 5, 2022

fix: Error on unsuported addresses in is_signed_by_address

5d5d178

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 5, 2022

fix: Error on unsuported addresses in is_signed_by_address

c271b83

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 5, 2022

fix: Error on unsuported addresses in is_signed_by_address

782182e

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 7, 2022

fix: Error on unsuported addresses in is_signed_by_address

13deba9

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 7, 2022

fix: Error on unsuported addresses in is_signed_by_address

2f34b8e

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 7, 2022

fix: Error on unsuported addresses in is_signed_by_address

2bc9115

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 9, 2022

fix: Error on unsuported addresses in is_signed_by_address

31ec64d

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz added a commit to Shatnerz/rust-bitcoin that referenced this pull request Feb 9, 2022

fix: Error on unsuported addresses in is_signed_by_address

79cee4c

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz mentioned this pull request Feb 20, 2022

feat: Add Address.is_related_to_*_key() #839

Merged

More complete MessageSignature.is_signed_by_address #684

More complete MessageSignature.is_signed_by_address #684

Conversation

Shatnerz commented Oct 28, 2021

Testing

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shatnerz Oct 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shatnerz commented Oct 29, 2021

Shatnerz commented Oct 31, 2021 • edited

Shatnerz commented Nov 1, 2021

Shatnerz commented Nov 2, 2021 • edited

Shatnerz commented Nov 14, 2021

dr-orlovsky commented Nov 14, 2021

Shatnerz commented Nov 14, 2021

Shatnerz commented Jan 11, 2022

apoelstra commented Jan 11, 2022

tcharding left a comment

Choose a reason for hiding this comment

tcharding Jan 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 12, 2022

Kixunil commented Jan 13, 2022

dr-orlovsky commented Jan 13, 2022 • edited

Kixunil commented Jan 13, 2022

sanket1729 commented Jan 13, 2022

Kixunil commented Jan 13, 2022

dr-orlovsky commented Jan 14, 2022

Shatnerz commented Jan 17, 2022

Shatnerz commented Feb 5, 2022

tcharding commented Feb 7, 2022

More complete `MessageSignature.is_signed_by_address` #684

More complete `MessageSignature.is_signed_by_address` #684

Shatnerz Oct 31, 2021 •

edited

Shatnerz commented Oct 31, 2021 •

edited

Shatnerz commented Nov 2, 2021 •

edited

tcharding Jan 12, 2022 •

edited

dr-orlovsky commented Jan 13, 2022 •

edited