New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BIP-322: Generic signed message format #16440
BIP-322: Generic signed message format #16440
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
9fa4b81
to
37a2542
Compare
Concept ACK |
a138b79
to
ef8fa5c
Compare
I really like the idea of BIP322, but I don't like that this changes existing RPCs in way that's not backwards compatible (e.g. a signmessage created and verified on 0.18 produces a "CDataStream::read(): end of data: iostream error" here). Similarly, a P2PKH signmessage created on this branch results in false
when checked with 0.18. Either P2PKH signing/verifying should be special cased to use the legacy signmessage format, or BIP322 message signing should use different RPCs (or at least a special RPC parameter or configuration option to indicate the user wants to use BIP322).
Just on this branch, I also created a 2-of-3 P2WSH address using addmultisigaddress
and then attempted to signmessage
with it. I received the error, ScriptPubKey does not refer to a key
. Unless I'm misunderstanding BIP322, this should be possible and it would be cool if the wallet knew how to do it (however, even if the wallet doesn't know how to do it, as long as it's possible by BIP322, the error message should be changed). I didn't test any more advanced scripts, although I think that would be worthwhile to test later---in theory, the wallet should be able to signmessage for any script where its solver could sign for a spend. I think importing some advanced scripts using descriptors and then signmessage'ing for them might make a good addition to the integration tests in this PR.
Another concern is that I don't believe the discussion over how BIP322 should handle timelocking opcodes (CLTV and CSV) was ever resolved. I think that's probably important before this could be released.
Finally, I skimmed the code and left one documentation-related note. Thanks for working on this!
src/rpc/misc.cpp
Outdated
{"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message to create a signature of."}, | ||
}, | ||
RPCResult{ | ||
"\"signature\" (string) The signature of the message encoded in base 64 (note: the address for the private key is the derived BECH32 address; any other type will fail validation)\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "the derived BECH32 address" will become ambiguous if subsequent versions of segwit are adopted. E.g. if taproot is adopted, there would be two possible bech32 addresses for the same public key. Perhaps this should say, "the derived P2WPKH bech32 address".
Nit: BECH is not an acronym AFAIK and so should not be all caps. A quick git grep shows that the convention in the repository is to treat it as a common noun, so all lowercase unless at the start of a sentence.
I don't really agree. Proving that you own an address is something that you do "now", not something you do and keep the proof around indefinitely. Losing an old proof should be perfectly fine, as the point is lost if you can't reproduce it. Of course, down the road Craig could claim that his broken proof actually worked, "but that core changed the mechanism so it can't be verified anymore" but I don't know if anybody cares about him enough to warrant keeping both kinds of verifiers (we probably do not want to keep the signmessage part).
Yes, BIP322 literally just says "the sighash (normally based on the transaction) is X; now sign/verify with this address" and uses exactly the same code that is used by the wallet to sign transactions, so anything goes. I'm actually confused why your example doesn't work out of the box. I'll look into that.
Actually, those should just be removed; they were meant for the proof of funds, which I am removing from BIP322. Thanks for the nudge.
Great! Thanks, will address. |
ef8fa5c
to
0048216
Compare
I think there are several points here:
I think the appropriate solution to all three issues is that these existing RPCs should continue to generate and verify legacy P2PKH signmessages when called with P2PKH addresses. They could automatically switch between legacy support and BIP322 support depending on address type or BIP322 could get new RPCs. (Something to consider would be amending BIP322 so that P2PKH addresses in BIP322 are special cased to use the old signmessage format; that way this PR can be made fully BIP322 compliant and yet backward compatible.) |
I see what you're saying. I think adding an optional 'format' which defaults to 'bip322' but can be set to 'legacy' for 'sign', and autodetection in 'verify' should be sufficient. What do you think? |
Sounds reasonable to me. I'd slightly prefer the default be legacy if the user provides a P2PKH address---but not enough to argue about it. Thanks! |
Thanks for picking this up again. I don't think we should gratuitously break compatibility with the existing signature scheme; that's generally not the approach we take with RPCs, and given that the scheme is implemented in other software too probably even more disruptive. I wouldn't be opposed to adding an exception in BIP322 that for P2PKH address the existing key recovery based scheme is used; in general I like avoiding ambiguity in these things. However I also see this may add additional complication to the spec, and perhaps not be very clear cut either (why just the P2PKH ones, and not the extensions to other single-key address types people have proposed and adopted in other software, for example?). |
I didn't expect we'd ever be able to get rid of the old format, even if the default would be a new format now. That'd indeed unfortunate, but that's life when you want compatibility. I don't think it's a big problem; it's not that much code. |
@harding @sipa I have updated the specification to include the special case, and I also added a description of the legacy format: https://github.com/bitcoin/bips/blob/e24e86b482e394e18803f14c7e2338aab9b7e1e2/bip-0322.mediawiki (from bitcoin/bips#808) Let me know if I got something wrong there. I wrote the legacy format spec based on the code. |
0048216
to
c390c53
Compare
I updated the code in this PR to do what was suggested (use legacy if p2pkh, otherwise use bip322), and I also updated bip322 to require this. |
c390c53
to
b96875a
Compare
Tested I skimmed this commit with the proposed changes to BIP322 and left a couple nits. As a specification, I think it could also use a description of |
FYI the bitcoin/bips PR was merged as you were reviewing it; I opened a fix-up here: bitcoin/bips#814 Thanks for testing!
Sure, since we are not getting rid of the legacy format, we might as well use it where it is usable. Edit: This has been updated. It uses the output type parsing from other places to allow legacy, p2sh-segwit, or bech32. I assume once taproot and such appear, they will be added to this and this should "just work [tm]". |
65bc98a
to
053afb2
Compare
src/interfaces/wallet.h
Outdated
@@ -255,6 +256,9 @@ class Wallet | |||
// Remove wallet. | |||
virtual void remove() = 0; | |||
|
|||
// Get a signing provider for the wallet. | |||
virtual SigningProvider& getSigningProvider() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to check with @achow101 that this doesn't conflict with the descriptor wallet changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't interfere with the descriptor wallet changes since a SigningProvider
was never actually needed in the GUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there won't be a singular signing provider for the whole wallet anymore, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think it would be better to add an interfaces::Wallet::signMessage()
method instead of adding an interfaces::Wallet::getSigningProvider()
method. Few reasons:
-
It would allow deduplicating more code for signing messages instead of having it written twice in qt and rpcwallet.
-
It would create a little less work for me with #10102, so I don't have to an add IPC wrapper around
SigningProvider
, or serializeSigningProvider
with private key information and send it from the wallet process to the qt process. -
Just in general I think
rpcwallet.cpp
is too monolothic and has too much important code mixed with UniValue boilerplate. So I'd love to see something likesrc/wallet/sign.h
/src/wallet/sign.cpp
files with abool SignMessage(CWallet& wallet, message, options...)
function and start organizing things better.
Anyway, I'm fine with this approach, but wanted to suggest an alternative.
EDIT: Changed Wallet&
to CWallet&
above (sorry for the confusing typo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanofsky That sounds good to me! I am not sure how to access the interfaces::Wallet
instance from rpcwallet.cpp
though. It seems like it only has access to CWallet
(while the QT side only has access to interfaces::Wallet
).
Is the idea to add two signmessage functions, where the interface one simply calls down into m_wallet->SignMessage(..)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanofsky That sounds good to me! I am not sure how to access the
interfaces::Wallet
instance fromrpcwallet.cpp
though. It seems like it only has access toCWallet
(while the QT side only has access tointerfaces::Wallet
).
rpcwallet.cpp shouldn't need to access interfaces::Wallet
here (and in general) because interfaces::Wallet
doesn't contain any real functionality, it's just a high level wrapper around selected wallet functions that GUI code uses to indirectly control wallets.
So the idea is just to implement a standalone function in wallet code:
bool SignMessage(CWallet& wallet, message, options...)
and to call this function both from wallet/rpcwallet.cpp
and interfaces/wallet.cpp
(just adding a one-line WalletImpl::signMessage()
wrapper method in the interface code).
Is the idea to add two signmessage functions, where the interface one simply calls down into
m_wallet->SignMessage(..)
?
Sorry! Just realized I had a typo in #16440 (comment), which is fixed now but was probably pretty confusing. I was trying to suggest not having a CWallet::SignMesage(...)
method, but instead defining a standalone SignMessage(CWallet&, ...)
function. Really either a method or a standalone function would be fine, but the function seemed better because we're gradually breaking CWallet up and pulling functionality out of it, so adding new functionality there without a reason seemed to be moving in wrong direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanofsky I ended up adding a couple of helper functions to proof.h
and adding a proof.cpp
file. The P2PKH-vs-others logic is now all contained inside the proof code, and is called from rpcwallet
/misc
/interfaces/wallet
:
Lines 237 to 250 in 6e9947e
/** | |
* Attempt to sign a message with the given destination. | |
*/ | |
void SignMessageWithSigningProvider(SigningProvider* sp, const std::string& message, const CTxDestination& destination, std::vector<uint8_t>& signature_out); | |
/** | |
* Attempt to sign a message with the given private key and address format. | |
*/ | |
void SignMessageWithPrivateKey(CKey& key, OutputType address_type, const std::string& message, std::vector<uint8_t>& signature_out); | |
/** | |
* Determine if a signature is valid for the given message. | |
*/ | |
Result VerifySignature(const std::string& message, const CTxDestination& destination, const std::vector<uint8_t>& signature); |
The interface side ends up like
bitcoin/src/interfaces/wallet.cpp
Lines 460 to 463 in 6e9947e
void signMessage(const std::string& message, const CTxDestination& destination, std::vector<uint8_t>& signature_out) override | |
{ | |
proof::SignMessageWithSigningProvider(m_wallet.get(), message, destination, signature_out); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side result of this, strMessageMagic
was moved out of validation.h/cpp into proof.cpp, and is no longer public, which is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The new SignMessageWithSigningProvider makes for fewer wallet changes than what I suggested, which is great!
e3a2396
to
7d9c869
Compare
b5248e4
to
51dea4b
Compare
The simple signature takes a sighash as argument and verifies a signature and pubkey against it. It is used in signet for verifying blocks and in BIP-322 implementation for verifying messages.
51dea4b
to
ebf4d23
Compare
- signmessagewithprivkey - verifymessage - signmessage
|
Will reopen (or re-create) after adapting simplifications in bitcoin/bips#903 |
This PR implements BIP-322, for the single proof (no multiple addresses simultaneously) single signer (no multisig addresses) case.
UI (CLI/QT) are restricted to the single proof case, but the underlying code (
script/proof.h
) supports multiple proofs.Recommend
?w=1
/-w
to avoid whitespace spam.There is a related PR #16653 that includes the sign/verify components of this and the signet PR (#16411).