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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
324+ {
325+ {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key to sign the message with."},
326+ {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message to create a signature of."},
327+ },
328+ RPCResult{
329+ "\"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"
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 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!
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).
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).
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.
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.
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.
Actually, those should just be removed; they were meant for the proof of funds, which I am removing from BIP322. Thanks for the nudge.
Finally, I skimmed the code and left one documentation-related note. Thanks for working on this!
Great! Thanks, will address.
I don’t like that this changes existing RPCs in way that’s not backwards compatible […]
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. […]
I think there are several points here:
This change breaks backwards compatibility between different versions of Bitcoin Core. I think it’s unfortunate if someone running version x can’t create or verify proofs when talking to someone running version y. Sure, it’d be nice if everyone upgraded to the latest version, but that takes time (sometimes years in the case of cold wallets).
This change breaks compatibility with almost every other wallet that has implemented signmessage support. Again, it’d be great if they all have BIP322 support ready at the same time this code gets into a Bitcoin Core release, but that seems extremely unlikely to me—I expect it to take years to get widespread BIP322 support. Then each of those wallets has their own user upgrade cycle which may take years, so we’re talking years + years until everyone is using BIP322 (if it is generally adopted at all). Until near-universal adoption happens, it’d be nice if Bitcion Core is compatible with other wallets.
As you mention, this breaks old signmessages. While I don’t feel strongly about that and I’m at least somewhat sympathetic with your arguments about people just generating new proofs, I think unnecessary breakage is something we should try to avoid.
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?
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?).
@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 https://github.com/bitcoin/bips/pull/808)
Let me know if I got something wrong there. I wrote the legacy format spec based on the code.
Tested signmessage and verifymessage both ways between 0.18 and this branch, looks good, thanks! You may want to consider making signmessagewithprivkey default to using P2PKH for backwards compatibility and add a third argument to that RPC that allows selecting P2WPKH (I’d suggest the argument select between P2PKH and P2WPKH (rather than legacy vs bip322) so that it can be extended to take a P2TR argument or other address format argument in the future).
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 CKey::SignCompact() and CKey::RecoverCompact() such as how they serialize which derived pubkey to use and whether or not it’s compressed or uncompressed. An alternative to putting this information in BIP322 would be to create a separate BIP for the established legacy signmessage format (which was created before we had BIPs) and then simply have BIP322 reference that BIP. I can help with documentation if you’d like.
FYI the bitcoin/bips PR was merged as you were reviewing it; I opened a fix-up here: https://github.com/bitcoin/bips/pull/814
Thanks for testing!
You may want to consider making
signmessagewithprivkeydefault to using P2PKH for backwards compatibility and add a third argument to that RPC that allows selecting P2WPKH (I’d suggest the argument select between P2PKH and P2WPKH (rather than legacy vs bip322) so that it can be extended to take a P2TR argument or other address format argument in the future).
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]”.
255@@ -255,6 +256,9 @@ class Wallet
256 // Remove wallet.
257 virtual void remove() = 0;
258
259+ // Get a signing provider for the wallet.
260+ virtual SigningProvider& getSigningProvider() = 0;
SigningProvider was never actually needed in the GUI.
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 serialize SigningProvider 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 like src/wallet/sign.h / src/wallet/sign.cpp files with a bool 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)
@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(..)?
@ryanofsky That sounds good to me! I am not sure how to access the
interfaces::Walletinstance fromrpcwallet.cppthough. 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:
0bool 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 (review), 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.
@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:
The interface side ends up like
strMessageMagic was moved out of validation.h/cpp into proof.cpp, and is no longer public, which is nice.
Concept ACK based on light testing. The API changes default to backwards compatibility with legacy signmessage and BIP322 is an excellent extension of the signmessage mechanism.
Needs fix and testing for this case:
0$ _gna() { src/bitcoin-cli getnewaddress '' bech32 ; } ; export -f _gna
1$ bitcoin-cli addmultisigaddress 2 '["'$(_gna)'", "'$(_gna)'", "'$(_gna)'"]' '' bech32
2{
3 "address": "bc1qxg4wrq0yc2vngavudljkjdvwcah6j06pc2pyygy3kevdqqlf707se96d7s",
4 "redeemScript": "522103da30e969126296db3db0b95030b8813557f031b3ce8aaa43215f77d62f6f506721021d856b20af7e5fc6f0f0e873e6bdbc1030a3ca63a19e8e71b2ed806ca5ec4ef521032f278e5fc10fa304151b6939dc545189905e492dbe8cb393fb8f2e976fb5440e53ae"
5}
6$ bitcoin-cli signmessage bc1qxg4wrq0yc2vngavudljkjdvwcah6j06pc2pyygy3kevdqqlf707se96d7s Test
7error code: -1
8error message:
9ScriptPubKey does not refer to a key
(Same error message also returned for legacy and p2sh-segwit.)
Abstract, tentative concept NACK:
I’m not convinced BIP322 is itself a good idea as-is. Its multiple-proofs concept seems ripe to encourage misuse of signatures as a false “proof” of spend-ability rather than simply proving a receiver (as current signatures do).
If BIP322 is intended for proving spend-ability, then it confuses the matter of addresses (which don’t spend), and also conflicts with the historical use case of signmessage and verifymessage which is never for spend-ability purposes.
I’m not convinced BIP322 is itself a good idea as-is. Its multiple-proofs concept seems ripe to encourage misuse of signatures as a false “proof” of spend-ability rather than simply proving a receiver (as current signatures do).
The argument for multiple proofs is that it’s not trivial to verify that many independent proofs are not overlapping. This becomes more serious if/when a “proof of funds” purpose is added, where I may convince you that I have more money than I actually do. Even now, though, I can make you think I have the private keys to more pubkeys than I do (and whatever locked funds associated with those).
The BIP explicitly describes how multiple proofs should be handled (e.g. immediately abort if the same proof appears twice), but if there are attack vectors I didn’t think of, I’d of course like to address them.
If BIP322 is intended for proving spend-ability, then it confuses the matter of addresses (which don’t spend), and also conflicts with the historical use case of signmessage and verifymessage which is never for spend-ability purposes.
BIP322 is intended to be a general purpose “prove/verify” system for script-style “stuff” in Bitcoin.
If there is a way to clarify the above, I’m all ears.
@kallewoof Existing signatures do not prove you have access to any private keys nor funds, only that you are the recipient to future transactions paying an address (and therefore the right person to be signing contractual terms for such payments). Because it proves nothing about existing funds or keys, there is no such thing as “overlapping” proofs to worry about.
If you intend to cover more proof cases than that, you probably need a new RPC so it can’t be confused with the purpose of the existing ones.
I know I’m late to the party but I would like to give some conceptual feedback. But I have some more serious issues understanding the BIP.
On a high level, it seems to me that the purpose of the BIP is not entirely clear. At the moment, it is only for signing messages but includes the entire scripting system. As I understand it, the reason is future extensions to proving ownership of funds. If this is right, I’m not convinced by this approach: If we want a BIP for signing and verifying messages, this can much simpler (if restricted to addresses). If we want a BIP for proving ownership of funds, we should actually write one. My concerns are:
(edit:) Maybe it helps to think about actual applications: When do we need a signed message? I think this is related to @luke-jr’s comments.
On a more technical level and related to the previous point: Does the BIP implicitly assume that a scriptPubKey contains exactly one pubkey? I’m not sure after reading it. Signing seems to fail if I can’t derive a private key from a scriptPubKey (but this could happen because there is not a single pubkey in the scriptPubKey OR because we don’t have the private key). Verification always uses some sighash, but what if there is no pubkey (or more than one pubkey)? This illustrates what I meant when I said the purposes are different: If you want to verify a signed message, you need a pubkey (and better exactly one). If you want to prove ownership, we shouldn’t give the user a verifymessage function that in fact won’t verify a signature in some cases.
I guess I have some more comments but it’s probably better to clarify this first. I’m happy to move this discussion to the mailing list if this is more appropriate for these conceptual issues.
On a high level, it seems to me that the purpose of the BIP is not entirely clear. At the moment, it is only for signing messages but includes the entire scripting system. As I understand it, the reason is future extensions to proving ownership of funds. If this is right, I’m not convinced by this approach: If we want a BIP for signing and verifying messages, this can much simpler (if restricted to addresses). If we want a BIP for proving ownership of funds, we should actually write one. My concerns are:
No, this is not the reason. The reason why the script system is used is unrelated to a potential future funds proving tool, and is purely done this way to make future upgrades (e.g. taproot, schnorr, …) seamless. New address formats and such will work out of the box assuming the underlying node(s) actually understand them.
- These two purposes of signing messages and proving ownership are very different, and maybe they deserve different approaches/BIPs/RPC.
Yes, I specifically removed any proof-of-funds related stuff from the BIP for this reason. It should be considered a separate thing, albeit with some shared functionality.
- The current BIP seems to be in the middle and it could very well happen that some of decisions taken now turn out to be not great when there is an actual extension to proving ownership.
Aside from the fact it is extensible to add another purpose (for the reason stated above), I’m not sure it’s overly middle-groundy as it stands. What could be trimmed out to make it less so without burning too many bridges in a potential future expansion to add a POF purpose?
On a more technical level and related to the previous point: Does the BIP implicitly assume that a scriptPubKey contains exactly one pubkey?
The BIP accepts any kind of script. In practice, if you feed it the appropriate scriptSig to fulfill the scriptPubKey associated with the address in question, it should be able to work with any scriptPubKeys.
I’m not sure after reading it. Signing seems to fail if I can’t derive a private key from a scriptPubKey (but this could happen because there is not a single pubkey in the scriptPubKey OR because we don’t have the private key).
Right now, it will attempt to derive a P2PKH key. If it succeeds, it uses the legacy system, otherwise it tries to produce a signature given that scriptPubKey as the input (just like it does when signing transactions). There is some practical bits missing to do e.g. multisig right now; the conclusion was basically that PSBT should be used, and was out of scope of this PR.
1423+
1424+ // Hash type is one byte tacked on to the end of the signature
1425+ std::vector<unsigned char> vchSig(vchSigIn);
1426+ if (vchSig.empty()) return false;
1427+ // int nHashType = vchSig.back();
1428+ vchSig.pop_back();
I don’t think it’s a problem to have malleable signature in this case, but perhaps this should be bolted down to always say SIGHASH_ALL.
We do not need the flag, in all honesty, but removing it means special-casing stuff in various places, so it felt simpler to just keep it in there.
12+Result SignMessage::Prepare(const std::vector<SignMessage>& entries, const std::string& message, std::set<CScript>& inputs_out, uint256& sighash_out, CScript& spk_out) const {
13+ Result rv = Purpose::Prepare(m_scriptpubkey, inputs_out);
14+ if (rv != RESULT_VALID) return rv;
15+ CHashWriter hw(SER_DISK, 0);
16+ std::string s = strMessageMagic + message;
17+ hw << m_scriptpubkey << LimitedString<65536>(s);
Unserialize method, see https://github.com/bitcoin/bitcoin/blob/master/src/serialize.h#L503
s. I used the limited string thing simply because I didn’t realize strings were allowed as is.
55+ assert_equal("INVALID", self.nodes[0].verifymessage(other_address, signature, message, 1))
56+ assert_equal("INVALID", self.nodes[0].verifymessage(address, other_signature, message, 1))
57+ assert_equal("INVALID", self.nodes[0].verifymessage(legacy_address, signature, message, 1))
58+ # We get an error here, because the verifier believes the proof should be a BIP-322 proof, due to
59+ # the address not being P2PKH, but the proof is a legacy proof
60+ assert_equal("ERROR", self.nodes[0].verifymessage(address, legacy_signature, message, 1))
I think if the (attacker-provided) input is indeed invalid, this should INVALID instead of ERROR. I think it’s cleaner to use ERROR only for real processing errors when we can’t reach a result, e.g., out-of-memory (okay, maybe a bad example here). Here, we have a result: The proof is invalid because a legacy proof for P2PKH is not allowed.
The background is that crypto APIs typically work like this and are easier to get wrong for the user. If someone gives me a 3 byte string and claims it’s an ECDSA signature for some message and some key, then it’s an invalid signature. That’s all I care about if I call a RPC, I don’t care (or shouldn’t care) why it’s invalid and I don’t need to handle different errors differently.
Hm, this has two layers: What other code (ours or not ours) is doing with the return value and what the user is doing with the return value.
INVALID, which leads to different behavior in the end.
I’m not sure I agree that users should not be concerned with debugging information. Oftentimes people are teaching each other how to do these things. If the other person says “it tells me ‘invalid’” because they missed one character when copy-pasting the proof (or something), that just adds unnecessary ambiguity to the situation.
Perhaps ‘ERROR’ is the wrong word, though. ‘MALFORMED’ might be better..
If I understand the code correctly, then any signature will verify for an anyone-can-spend output or other outputs that don’t require a signature.
This is somewhat weird for the user: He calls verifymessage, which returns VALID but actually no message has been verified. Maybe he thinks that VALID confirms that the address is indeed protected by a signature but it isn’t.
No, this is not the reason. The reason why the script system is used is unrelated to a potential future funds proving tool, and is purely done this way to make future upgrades (e.g. taproot, schnorr, …) seamless. New address formats and such will work out of the box assuming the underlying node(s) actually understand them.
Okay, I see. This was not clear at all the me from reading the BIP because the BIP talks about verifying messages based on the “Bitcoin Script Format”. (Yes, it’s kind of clear if you look at the RPC…) But the purpose of the BIP is rather signing and verifying messages with respect to address (and the script format is only a technical tool.) I think this can be made clearer in the BIP.
Aside from the fact it is extensible to add another purpose (for the reason stated above), I’m not sure it’s overly middle-groundy as it stands. What could be trimmed out to make it less so without burning too many bridges in a potential future expansion to add a POF purpose?
Hm yeah, good question. I think @luke-jr is right and we don’t need multiple proofs for simple signatures of messages. And we should think about disallowing proofs that don’t require any signatures.
Also the terminology is somehow unclear, both in the BIP and in the code. What I mean is that don’t understand the difference between a “proof” and a “signature” (or maybe call it “message signature” or “signed message” to make clear it’s not a signature of a transaction)?
If I read the code correctly, the basic idea is
To be honest, this was only clear to me after reading the code and not at all from the BIP.
Another concern that I have is that there are a lot of verification result codes (related to what I said above for ERROR/INVALID). I guess we INCOMPLETE for PSBT? But this should concern signers only. Wouldn’t INVALID be enough for a verifier?
Also, I see the point of INCONCLUSIVE but this reminds me a lot of certificate warnings, which are a UX mess. What should I as a user when I get an inconclusive signature? (I’m not saying that I have a better idea to implement this.)
@real-or-random Thanks a lot for all your feedback. Sorry it took some days to get back to you.
Would you mind opening a PR to https://github.com/bitcoin/bips to make the BIP clearer?
Edit: to answer your question, if you get an inconclusive proof from someone, it means your node is probably old. If it isn’t old, the prover is probably using broken software to generate said proof. This feature is here to deal with the case where new consensus rules were activated (e.g. Segwit Script Version 1). Old nodes would accept those in blocks (not INVALID) but would not be able to check them beyond that (thus INCONCLUSIVE).
23+
24+/**
25+ * Result codes ordered numerically by severity, so that A is reported, if A <= B and A and B are
26+ * two results for a verification attempt.
27+ */
28+enum Result {
enum class?
33+ RESULT_ERROR = -4, //!< An error was encountered
34+};
35+
36+inline std::string ResultString(const Result r) {
37+ static const char *strings[] = {"ERROR", "INVALID", "INCOMPLETE", "INCONCLUSIVE", "VALID"};
38+ return r < -4 || r > 0 ? "???" : strings[r + 4];
r < -4 || r > 0 is known to be false at compile-time and the "???" path is thus unreachable. Perhaps use switch instead?
I’m not sure what you mean. No guarantees that r is actually a valid Result enum value.
0#include <cstdio>
1
2enum class X {
3 one = 1,
4 two = 2,
5};
6
7int main(int argc, char** argv)
8{
9 X z = (X)3;
10 printf("z = %d\n", z); // outputs 3
11}
221+ auto d = GetDestinationForKey(key.GetPubKey(), address_type);
222+ auto a = GetScriptForDestination(d);
223+ privkeys[d] = key;
224+ m_challenge.emplace_back(a);
225+ }
226+ void GenerateSingleProof(const SignMessage& challenge, SigningProvider* sp, const uint256& sighash, const CScript& scriptPubKey, const std::string& message) override {
challenge and message not used?
229+ if (!ProduceSignature(*sp, sc, scriptPubKey, sigdata)) {
230+ throw signing_error("Failed to produce a signature");
231+ }
232+ m_proof.m_items.emplace_back(sigdata);
233+ }
234+ Result VerifySingleProof(unsigned int flags, const SignMessage& challenge, const SignatureProof& proof, const std::string& message, const uint256& sighash, const CScript& scriptPubKey) override {
challenge and message not used?
7+const std::string strMessageMagic = "Bitcoin Signed Message:\n";
8+
9+namespace proof
10+{
11+
12+Result SignMessage::Prepare(const std::vector<SignMessage>& entries, const std::string& message, std::set<CScript>& inputs_out, uint256& sighash_out, CScript& spk_out) const {
entries not used?
134+ virtual Result VerifySingleProof(unsigned int flags, const T& challenge, const SignatureProof& proof, const std::string& message, const uint256& sighash, const CScript& scriptPubKey) = 0;
135+
136+ void Prove(const std::string& message, SigningProvider* signingProvider = nullptr) {
137+ m_proof.m_items.clear();
138+ m_proof.m_version = BIP322_FORMAT_VERSION;
139+ m_proof.m_entries = m_challenge.size();
m_challenge.size() <= 255 guaranteed here (m_entries is uint8_t)? If so, perhaps assert(...) that to make the tacit assumption explicit? :-)
93+ } catch (const std::runtime_error&) {
94+ return Result::RESULT_ERROR;
95+ }
96+}
97+
98+}
// namespace proof :-)
Would you mind opening a PR to https://github.com/bitcoin/bips to make the BIP clearer?
I’ll be gone for two weeks but I’ll be able to open a PR afterwards. I think the main points are
If I read the code correctly, the basic idea is
* checking that a provided script matches the address and * verifying the script but replacing every signature verification operation by a signature verification operation on the provided message instead of the transaction.
Edit: to answer your question, if you get an inconclusive proof from someone, it means your node is probably old. If it isn’t old, the prover is probably using broken software to generate said proof. This feature is here to deal with the case where new consensus rules were activated (e.g. Segwit Script Version 1). Old nodes would accept those in blocks (not INVALID) but would not be able to check them beyond that (thus INCONCLUSIVE).
Yep, I see the point, and it makes a lot of sense. And yep, I don’t have a better idea either. Maybe we can find a different word at least? “Inconclusive” does not say much (in fact that’s the original of this word). For example INVALID_TRY_UPGRADING_YOUR_NODE is not beautiful either but has some more meaning. I’m sure there are better suggestions. Also the docs should say that upgrading can help.
I wouldn’t be opposed to adding an exception in BIP322 that for P2PKH address the existing key recovery based scheme is used;
I think it would be nicer if it were just a separate RPC so that the old format (esp verifying) could be eventually sunset.
On a high level, it seems to me that the purpose of the BIP is not entirely clear. At the moment, it is only for signing messages but includes the entire scripting system. […] Does the BIP implicitly assume that a scriptPubKey contains exactly one pubkey?
I think this isn’t the best way to think about this facility. Bitcoin transactions are not signed with ECDSA. They are signed with a digital signature system called “Bitcoin Script”. Bitcoin Script happens to have ECDSA embedded in it– like ECDSA has field multiplication embedded in it. A bitcoin message signing thing can be seen as just a sign-stuff-using-script.
Unfortunately this is complicated by the fact that script itself is expected to be embedded in a consensus system, so stuff like softforks are awkward to handle.
Why would someone want to sign messages using script? In practice usually for dumb reasons for proving control or tying metadata to addresses that used to control funds, currently currently control funds, or will control funds in the future. For these latter three things, nothing less than Bitcoin Script would work– no amount of “sniff scripts yank keys out” would actually get the right behaviour.
For the “currently control funds” case it would be very useful if the system actually can check the existence of the funds (e.g. specifying txouts and checking them). For the “previously controlled funds” it would be very useful if the system could check that (SPV proofs for the outputs? kinda big :( ).
But for the “will control funds in the future” case there is basically no special behaviour needed/possible except signing with the same digital signature system which will eventually be used.
So I think this would mostly be useful for this last case. But that last case might better by serve by dry-signing an actual transaction with a dummy input/locktime, since doing that actually verifies your ability to sign for transaction like things.
For CSV, CLTV… probably the right way to do that is have them go in as non-locked (max sequence) by default but allow specifying other values for these fields as some kind of argument tagged onto the end of the address.
On a high level, it seems to me that the purpose of the BIP is not entirely clear. At the moment, it is only for signing messages but includes the entire scripting system. […] Does the BIP implicitly assume that a scriptPubKey contains exactly one pubkey?
I think this isn’t the best way to think about this facility. Bitcoin transactions are not signed with ECDSA. They are signed with a digital signature system called “Bitcoin Script”. Bitcoin Script happens to have ECDSA embedded in it– like ECDSA has field multiplication embedded in it. A bitcoin message signing thing can be seen as just a sign-stuff-using-script.
To be clear, I agree with that and I think what you describe is indeed the right the way to think about it. The question you quoted was at a time when I really didn’t understand what was going on in the BIP.
The one point where my crypto heart hurts is when you call this a signature scheme. That’s just because I have a different defintion in mind. Let’s say script is a scheme for authorizing transactions.
But this is one of the open points here: We need to think about the terminology: I think “signed message” is misleading given how expressive scripts are: If you know about cryptography, you think of a signature scheme. But this really breaks down when it comes to, e.g., anyonecanspend or hash preimages, where ECDSA is not involved. If you have no idea of cryptographic definitions, a “signed message” should still be something that only someone with a secret can produce and that cannot by modified by someone without the secret. Again, this intuition breaks down with unusual scripts.
Proof of “control”? This comes with a sound of “I’m the only one in control” and is again strange for anyonecanspend.
Maybe something that means “proof that you can fulfill the script” but with nicer words?
Sorry, I’m not really constructive here but I don’t have a better suggestion at the moment.
Apologies about lack of activity. My work pulled me away for a couple of months. I’m getting back to things now, though. ><
No worries. :+1:
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.
- signmessagewithprivkey
- verifymessage
- signmessage🐙 This pull request conflicts with the target branch and needs rebase.