RPC signmessage

[nodech]

Builds on top #464, fixes issue #460

General updates on the signmessagewithprivkey, verifymessage and
signmessage :
These two calls are outdated and only work for old P2PKH addresses. It can
not be used with witness transactions or p2sh.
BitcoinCore thread about this: Bitcoin Core issue
Trezor implemeneted variation of signmessage, details in the issue.

There's new proposal for generic signed messages signing:
BIP 322: Generic Signed Message Format and BIP 322 - PR.

There's also an discussion about not adding new utility
RPC calls to the RPC that can be implemented as a library.

Other updates:

• organize utils tests
• add TODO for util tests to move to their respective repositories
  • encoding tests should go to bufio
  • base58 tests needs to go to bcrypto(previously bstring), after we update to bcrypto@4 (bcoin-org/bcrypto@216f918)
  • Validator tests need to go to bval and write more tests there.

---

NOTE: Bcrypto@4 changes signRecoverable: bcoin-org/bcrypto#22

[pinheadmz]

Got the tests to pass locally and also signed/verified messages between bcoin and bitcoind:

$ bwallet-cli rpc signmessage 1DxWmAH2BVSstDke3aeeGzuLfEoWaCTcsL 'Nodar did a great job with this PR!'
IL9qMrkeeQAMedg/J+Qb72mWSVVZmGrJopIPucHpm0cSX3VBgofOBBWeIJLneombiHiYSYmf7eJofTDlBL85ISQ=

$ bitcoin-cli verifymessage 1DxWmAH2BVSstDke3aeeGzuLfEoWaCTcsL IL9qMrkeeQAMedg/J+Qb72mWSVVZmGrJopIPucHpm0cSX3VBgofOBBWeIJLneombiHiYSYmf7eJofTDlBL85ISQ= 'Nodar did a great job with this PR!'
true

$ bitcoin-cli signmessage 167btagzs75tEWzRMKKwMm1d1K4zUbrpp8 'Tbilisi is the capital of Georgia'
H75pcpG2dfNjZxHY54KoBuE9qp+FEP8Vkoewl/evzSWVfoBBuxu7CDcqKgPLxJQp8w/R2bNOY4ZFzI6q2xt2ay4=

$ bcoin-cli rpc verifymessage 167btagzs75tEWzRMKKwMm1d1K4zUbrpp8 H75pcpG2dfNjZxHY54KoBuE9qp+FEP8Vkoewl/evzSWVfoBBuxu7CDcqKgPLxJQp8w/R2bNOY4ZFzI6q2xt2ay4= 'Tbilisi is the capital of Georgia'
1

It is perhaps a little weird that bcoin returns 1/0 for signature verification and not the words true/false

[nodech]

@pinheadmz Good catch, fixed it.

[braydonf]

Moving the RPC methods that don't require wallet, chain, network access sounds like a good plan. Regardless, message signing and verification should work.

[pinheadmz]

Looks like this needs a rebase on master. Would be helpful to have merged for this external package: https://github.com/pinheadmz/bid

[braydonf]

I believe the tests were organized in a prior commit in preparation for this, see #804.

[nodech]

Should I move to #810 ?
That PR Addressses and reorganizes more things and will be another conflict.

I can rebase this as well.

[pinheadmz]

@nodar-chkuaselidze I vote for rebase, this is ready to merge, its an important bug fix. #810 doesn't really fix anything like this does, that may be have to rebase after this is merged.

[nodech]

Rebased.

[nodech]

@braydonf Most of these are legacy, they should not be part of the bcoin or need to be modified to so they are relevant to the codebase. I just reformatted and put TODOs. I can clean them up in this PR though, but that was the reason for those modifications.

[braydonf]

Okay, yeah I think those changes are not needed in this PR (see braydonf@c71a6e2).

[nodech]

Okay, yeah I think those changes are not needed in this PR (see braydonf@c71a6e2).

rebased.

[pinheadmz]

LGTM

$ bitcoin-cli -regtest signmessage munNXtuPELrtxe244YRmxepQDJsFAdPafk "bcoin is a menace"
IPOzeEacPpH2jHZerAs4c6j2ePt12y2rfBk6A7xz7g5ZfoZ9ArLY4TMepg+TypCnM/CatYO0r01n5p2HKNZOrSc=

$ bcoin-cli --network=regtest rpc verifymessage munNXtuPELrtxe244YRmxepQDJsFAdPafk IPOzeEacPpH2jHZerAs4c6j2ePt12y2rfBk6A7xz7g5ZfoZ9ArLY4TMepg+TypCnM/CatYO0
r01n5p2HKNZOrSc= "bcoin is a menace"
true

$ bwallet-cli --network=regtest rpc signmessage mrVJ5n3uJ4TkZCJ9KrJWhWKVroTtsQFPtT "no, you are the menace"
ILgvQvdor+ojzDMOiPqPSYQrq2x2giKGJbfvbEaXDng6ay3WApEKUpLDFxlvqOKMRy4hWFVl6XFNvbb/roSEYOA=

$ bitcoin-cli -regtest verifymessage mrVJ5n3uJ4TkZCJ9KrJWhWKVroTtsQFPtT ILgvQvdor+ojzDMOiPqPSYQrq2x2giKGJbfvbEaXDng6ay3WApEKUpLDFxlvqOKMRy4hWFVl6XFNvbb/roSEYOA=  "no, you are the menace"
true

[pinheadmz]

I was, however, able to sign a message with a bech32 address. I think this is undefined behavior in bitcoind:

$ bwallet-cli rpc signmessage bcrt1qck3axmyswytk0wh4n5vetp796rgck3mzzxmda6 "hello"
H9iAkgICibShNrA64k83Xbkh5Y7kfCqDNxPM/fnTjJ0VE9hnjaai1OxdpgHrqlcqgwSNtoYWGghT9sRQshsBvLE=

$ bitcoin-cli signmessage bc1qpjm2uak8cj89dmww0wk2l98rpc34wpcws7mjuc  "bcoin is a menace"
error code: -3
error message:
Address does not refer to key

[pinheadmz]

Okay I have Electrum v3.3.4 installed locally, and was able to sign and verify messages back and forth between Electrum and bcoin with bech32 addresses. So even though Bitcoin Core doesn't sign or verify with bech32 yet, there is at least some community agreement that it is an ok thing to do.

Electrum reference: spesmilo/electrum@e299df7

[pinheadmz]

We should also track spesmilo/electrum#3861 and add our compatibility with Electrum if this PR gets merged as-is.

[braydonf]

I think there is general confusion around using a bitcoin address to verify signatures, as the address may not correspond with a private and public key pair. So I think the use case is somewhat discouraged because it can be confusing (and likely why bech32 isn't working across implementations). To support it in all cases I think is fairly complex and making the use case less useful.

We can fix signing/verifying, however there should probably be some skepticism in regards to new use cases that may use an address for the purpose of verifying and signing messages.