refactor: deduplicate the message sign/verify code #17577

vasild · 2019-11-24T16:35:09Z

The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced MessageSign() and
MessageVerify() and add unit tests for them.

DrahtBot · 2019-11-24T17:11:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: consolidate sendmany and sendtoaddress code #18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
refactor: Abstract boost::variant out #17953 (refactor: Abstract boost::variant out by elichai)
Disallow automatic conversion between disparate hash types #17938 (Disallow automatic conversion between disparate hash types by Empact)
rpc: Auto-format RPCResult #17809 (rpc: Auto-format RPCResult by MarcoFalke)
UI external signer support (e.g. hardware wallet) #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
External signer support - Wallet Box edition #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
BIP-322: Generic signed message format #16440 (BIP-322: Generic signed message format by kallewoof)

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.

src/util/message.cpp

fanquake · 2019-11-24T18:18:49Z

Looks like this is going to overlap with #17557.

vasild · 2019-11-29T20:08:51Z

@jkczyz What do you think about this PR and its relation to #17557? Merge both in one PR?

jkczyz · 2019-12-04T16:29:22Z

@jkczyz What do you think about this PR and its relation to #17557? Merge both in one PR?

I have #17557 based on an outstanding PR (#17399), though I suppose it didn't have to be. The former resulted from a comment in the latter. I deliberately kept the change small so it could hopefully be merged quicker. Though I wanted to add a test to clarify why the "magic" string is needed.

That said, from a design perspective, I prefer your PR as it hides the complexity around signing a message.

vasild · 2019-12-07T20:03:21Z

@jkczyz thanks for the feedback! I did cherry-pick the last two commits from #17557 into this PR under e3d85bc - the hashing utility function and the unit test for it (to clarify why the "magic" string is needed).

vasild · 2019-12-07T20:14:05Z

Conflicts

Reviewers, this pull request conflicts with the following ones:

* [#17593](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17593.html) (test: move more utility functions into test utility library by mzumsande)

* [#17581](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17581.html) (refactor: Remove settings merge reverse precedence code by ryanofsky)

* [#17580](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17580.html) (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)

* [#17493](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17493.html) (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

* [#17473](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17473.html) (refactor: Settings code cleanups by ryanofsky)

* [#17399](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17399.html) (validation: Templatize ValidationState instead of subclassing by jkczyz)

* [#16545](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/16545.html) (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

All of the above conflicts are mechanical, small and easy to resolve.

instagibbs · 2019-12-19T16:18:20Z

concept ACK, though I'd probably be more pedantic in calling it "Bitcoin Signed Message Protocol" or something. Unfortunately there's no BIP to refer to.

vasild · 2019-12-24T11:23:59Z

@instagibbs Thanks for the review! Where would you put the "Bitcoin Signed Message Protocol" label?

We do message signing and verification now. What about encryption and decryption? Would that be useful feature or just a bloat that is rarely used? If encryption may be added in the future then maybe just "Bitcoin Message Protocol" would be a better name, or "Bitcoin Text Message Protocol".

laanwj · 2020-02-05T15:21:08Z

I don't think encryption / decryption in bitcoind is a good idea. It is best to avoid using keys for multiple purposes (e.g. use a signing key for encryption, and vice versa). Also it's scope creep: Bitcoin core is a validating full node. It should do that well, and not try to make a GPG-ish mess of it that tries to do everything with a mash-up of features but nothing particularly well…

Sjors · 2020-02-12T08:58:19Z

See also @achow101's #18115

vasild · 2020-02-12T10:45:43Z

See also @achow101's #18115

@Sjors thanks for the notice!

This PR (#17577) deduplicates message signing and message verification code.

I peeked at #18115 and among other things it deduplicates the message signing code from 2 of 3 places and does not deduplicate the message verification code. Assuming it also fixes the 3rd place where message signing code is repeated, then it will achieve half of this PR.

Should #18115 hit master before this PR, then I will trim this PR to only consolidate the message verification code.

vasild · 2020-02-12T20:08:25Z

I peeked at #18115 and among other things it deduplicates the message signing code from 2 of 3 places and does not deduplicate the message verification code. Assuming it also fixes the 3rd place...

That was fixed and now #18115 deduplicates all 3 message signing repetitions.

MessageSign() from #17577 is equivalent to SignMessage() from #18115. The former has unit tests added in #17577.

achow101

Couple of nits. otherwise ACK 1a00fb8

achow101 · 2020-02-12T19:59:23Z

src/qt/signverifymessagedialog.cpp

+
+    switch (MessageVerify(address, signature, message)) {
+        case MessageVerificationResult::ERR_INVALID_ADDRESS:
+            ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");


These ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");s could be moved out of the switch to further reduce duplication.

achow101 · 2020-02-12T20:09:43Z

src/util/message.cpp

+bool MessageSign(
+    const CKey& privkey,
+    const std::string& message,
+    std::string* signature)


This should be a reference (std::string&) instead of pointer.

achow101 · 2020-02-12T20:13:15Z

src/test/util_tests.cpp

+    const uint256 signature_hash = Hash(unsigned_tx.begin(), unsigned_tx.end());
+
+    const uint256 message_hash = MessageHash(unsigned_tx);
+    BOOST_CHECK_NE(message_hash, signature_hash);


I think you should have on additional check here with the full hashed message. i.e. also hash Bitcoin Signed Message:\n... and compare that with message_hash.

Notice: assuming CHashWriter hasher(SER_GETHASH, 0);, then:

hasher << "ab" << "cd";

would produce a different result than:

hasher << "abcd";

achow101 · 2020-02-12T20:14:11Z

src/util/message.cpp

@@ -14,7 +15,27 @@
 #include <string>
 #include <vector>

-const std::string strMessageMagic = "Bitcoin Signed Message:\n";
+static const std::string MESSAGE_MAGIC = "Bitcoin Signed Message:\n";


Renaming this should probably go in the next commit?

Sjors · 2020-02-13T14:05:10Z

tACK ad39c8f, thanks for adding tests as well

achow101 · 2020-02-13T21:48:04Z

I'm seeing a new compiler warning:

In file included from ./script/signingprovider.h:13,
                 from ./outputtype.h:10,
                 from rpc/misc.cpp:8:
rpc/misc.cpp: In function ‘UniValue verifymessage(const JSONRPCRequest&)’:
./sync.h:179:104: warning: control reaches end of non-void function [-Wreturn-type]
  179 | #define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
      |                                                                                                        ^
rpc/misc.cpp:273:5: note: in expansion of macro ‘LOCK’
  273 |     LOCK(cs_main);
      |     ^~~~

achow101 · 2020-02-13T21:48:53Z

src/rpc/misc.cpp

+        case MessageVerificationResult::ERR_NOT_SIGNED:
+            return false;
+        case MessageVerificationResult::OK:
+            return true;
    }


You need another return false statement after this switch.

Nice catch. I'm not getting that -Wreturn-type warning on macOS. Only one of the Travis machines noticed it: https://travis-ci.org/bitcoin/bitcoin/jobs/649943882?utm_medium=notification&utm_source=github_status

Added return false;, thanks!

The logic of verifying a message was duplicated in 2 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked() src/rpc/misc.cpp verifymessage() with the only difference being the result handling. Move the logic into a dedicated src/util/message.cpp MessageVerify() which returns a set of result codes, call it from the 2 places and just handle the results differently in the callers.

The logic of signing a message was duplicated in 3 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_signMessageButton_SM_clicked() src/rpc/misc.cpp signmessagewithprivkey() src/wallet/rpcwallet.cpp signmessage() Move the logic into src/util/message.cpp MessageSign() and call it from all the 3 places.

And add unit test for it. The purpose of using a preamble or "magic" text as part of signing and verifying a message was not given when the code was repeated in a few locations. Make a test showing how it is used to prevent inadvertently signing a transaction.

Sjors · 2020-02-14T12:00:17Z

re-ACK e193a84

achow101 · 2020-02-14T15:37:48Z

ACK e193a84

…Travis machines c98c26e ci: use --enable-werror on more hosts (Sjors Provoost) 6ba617d build: add Wreturn-type to Werror flags (Sjors Provoost) Pull request description: I overlooked a missing `return false` in #17577 (comment) and the warning only showed up on one Travis machine (`warning: control reaches end of non-void function [-Wreturn-type]`). This PR promotes `Wreturn-type` to an error when configured with `--enable-werror`. I also added `--enable-werror` to the Travis machine that happened to catch this particular instance. ACKs for top commit: vasild: ACK c98c26e. practicalswift: ACK c98c26e Tree-SHA512: 64e86c67fef2c5048aab201a8400b7e4a6f27b93d626159ba0b2807b5f119d2b0a83e3372db88f692cb4b0d059722d6a642d130c74a4f991a27f3a6b21780b5f

Sjors · 2020-02-19T12:11:23Z

@meshcollider some of this touches the wallet - and #18115 that builds on top of this definitely does - so maybe you can take a look?

instagibbs

utACK e193a84

instagibbs · 2020-02-21T15:34:21Z

src/util/message.h

+ * @param[in] signature The signature in base64 format.
+ * @param[in] message The message that was signed.
+ * @return result code */
+MessageVerificationResult MessageVerify(


nit: I'd prefer actual types used instead of strings for all args.

this is a non-blocking comment :) let's get this merged

meshcollider

utACK e193a84

Nice cleanup, thanks!

e193a84 Refactor message hashing into a utility function (Jeffrey Czyz) f8f0d98 Deduplicate the message signing code (Vasil Dimov) 2ce3447 Deduplicate the message verifying code (Vasil Dimov) Pull request description: The message signing and verifying logic was replicated in a few places in the code. Consolidate in a newly introduced `MessageSign()` and `MessageVerify()` and add unit tests for them. ACKs for top commit: Sjors: re-ACK e193a84 achow101: ACK e193a84 instagibbs: utACK bitcoin@e193a84 meshcollider: utACK e193a84 Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88

Summary: * Deduplicate the message verifying code The logic of verifying a message was duplicated in 2 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked() src/rpc/misc.cpp verifymessage() with the only difference being the result handling. Move the logic into a dedicated src/util/message.cpp MessageVerify() which returns a set of result codes, call it from the 2 places and just handle the results differently in the callers. * Deduplicate the message signing code The logic of signing a message was duplicated in 3 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_signMessageButton_SM_clicked() src/rpc/misc.cpp signmessagewithprivkey() src/wallet/rpcwallet.cpp signmessage() Move the logic into src/util/message.cpp MessageSign() and call it from all the 3 places. * Refactor message hashing into a utility function And add unit test for it. The purpose of using a preamble or "magic" text as part of signing and verifying a message was not given when the code was repeated in a few locations. Make a test showing how it is used to prevent inadvertently signing a transaction. Backport of Core [[bitcoin/bitcoin#17577 | PR17577]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D8098

e193a84 Refactor message hashing into a utility function (Jeffrey Czyz) f8f0d98 Deduplicate the message signing code (Vasil Dimov) 2ce3447 Deduplicate the message verifying code (Vasil Dimov) Pull request description: The message signing and verifying logic was replicated in a few places in the code. Consolidate in a newly introduced `MessageSign()` and `MessageVerify()` and add unit tests for them. ACKs for top commit: Sjors: re-ACK e193a84 achow101: ACK e193a84 instagibbs: utACK bitcoin@e193a84 meshcollider: utACK e193a84 Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88

fanquake added the Refactoring label Nov 24, 2019

vasild force-pushed the message-dedup branch from 3f17b4d to fcb1d27 Compare November 24, 2019 17:07

vasild commented Nov 24, 2019

View reviewed changes

src/util/message.cpp Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

vasild force-pushed the message-dedup branch from fcb1d27 to 38e24d7 Compare November 24, 2019 20:10

vasild force-pushed the message-dedup branch from 38e24d7 to e3d85bc Compare December 7, 2019 19:58

DrahtBot added the Needs rebase label Dec 16, 2019

vasild force-pushed the message-dedup branch from e3d85bc to 1a00fb8 Compare December 24, 2019 11:12

DrahtBot removed the Needs rebase label Dec 24, 2019

achow101 reviewed Feb 12, 2020

View reviewed changes

vasild force-pushed the message-dedup branch from 1a00fb8 to ad39c8f Compare February 13, 2020 13:04

achow101 reviewed Feb 13, 2020

View reviewed changes

Sjors mentioned this pull request Feb 14, 2020

build: add Wreturn-type to Werror flags, check on more Travis machines #18145

Merged

vasild and others added 3 commits February 14, 2020 10:45

vasild force-pushed the message-dedup branch from ad39c8f to e193a84 Compare February 14, 2020 10:46

DrahtBot mentioned this pull request Feb 17, 2020

util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value #18162

Merged

laanwj added this to Blockers in High-priority for review Feb 20, 2020

instagibbs reviewed Feb 21, 2020

View reviewed changes

This was referenced Feb 21, 2020

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

refactor: consolidate sendmany and sendtoaddress code #18202

Merged

meshcollider reviewed Feb 25, 2020

View reviewed changes

meshcollider merged commit 03f98b1 into bitcoin:master Feb 25, 2020

vasild deleted the message-dedup branch February 25, 2020 10:33

meshcollider removed this from Blockers in High-priority for review Feb 25, 2020

This was referenced Feb 28, 2020

util: Refactor message hashing into a utility function #17557

Closed

validation: Templatize ValidationState instead of subclassing #17399

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

refactor: deduplicate the message sign/verify code #17577

refactor: deduplicate the message sign/verify code #17577

Conversation

vasild commented Nov 24, 2019

DrahtBot commented Nov 24, 2019 • edited

Conflicts

fanquake commented Nov 24, 2019

This comment has been minimized.

vasild commented Nov 29, 2019

jkczyz commented Dec 4, 2019

vasild commented Dec 7, 2019

vasild commented Dec 7, 2019

Conflicts

instagibbs commented Dec 19, 2019

vasild commented Dec 24, 2019

laanwj commented Feb 5, 2020

Sjors commented Feb 12, 2020

vasild commented Feb 12, 2020 • edited

vasild commented Feb 12, 2020 • edited

achow101 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

Sjors commented Feb 13, 2020

achow101 commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Feb 14, 2020

achow101 commented Feb 14, 2020

Sjors commented Feb 19, 2020

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshcollider left a comment

Choose a reason for hiding this comment

DrahtBot commented Nov 24, 2019 •

edited

vasild commented Feb 12, 2020 •

edited

vasild commented Feb 12, 2020 •

edited