test: check that `verifymessage` RPC fails for non-P2PKH addresses #25782

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202208-test-rpc-check_that_verifymessage_only_works_with_p2pkh changing 1 files +20 −2

theStack commented at 4:54 PM on August 4, 2022: contributor

This PR adds missing test coverage for the verifymessage RPC, for the case that a non-P2PKH (but otherwise valid) address is passed: https://github.com/bitcoin/bitcoin/blob/e09ad284c762a79d59417389e9056c18e25d9770/src/util/message.cpp#L38-L40 https://github.com/bitcoin/bitcoin/blob/e09ad284c762a79d59417389e9056c18e25d9770/src/rpc/signmessage.cpp#L48-L49 The passed addresses to trigger the error are of the types nested segwit (P2SH-P2WPKH) and native segwit (P2WPKH) and are created with a helper function addresses_from_privkey using descriptors and the deriveaddresses RPC. At some point in the future, if we have BIP322 support, all those will likely succeed and can then be moved from error-throwing to the succedding assert loop.
fanquake added the label Tests on Aug 4, 2022
achow101 commented at 7:53 PM on August 4, 2022: member

It looks like this adds a bunch of boilerplate just to get the WIF key into an ECKey so that addresses can be derived from it. I think it would be easier to just put the key into a descriptor and use deriveaddresses to get the addresses.
test: check that `verifymessage` RPC fails for non-P2PKH addresses 68006c10ab
theStack force-pushed on Aug 5, 2022
theStack commented at 10:10 AM on August 5, 2022: contributor

It looks like this adds a bunch of boilerplate just to get the WIF key into an ECKey so that addresses can be derived from it. I think it would be easier to just put the key into a descriptor and use deriveaddresses to get the addresses.

Thanks, that is indeed much simpler, I wasn't even aware that using WIF private keys in descriptors is possible (though it makes sense that it is). Force-pushed and changed the PR description accordingly.
achow101 commented at 7:46 PM on August 5, 2022: member

ACK 68006c10abbfec0f16b90efa69b7880a5e17f186
in test/functional/rpc_signmessagewithprivkey.py:16 in 68006c10ab
```
  11 |  from test_framework.util import (
  12 |      assert_equal,
  13 |      assert_raises_rpc_error,
  14 |  )
  15 |  
  16 | +
```
w0xlt commented at 1:23 PM on August 8, 2022:

nit: this extra line can be removed.

theStack commented at 10:53 PM on August 8, 2022:

Note that the extra lines were introduced intentionally, to follow PEP8: Surround top-level function and class definitions with two blank lines.

in test/functional/rpc_signmessagewithprivkey.py:61 in 68006c10ab

  57 | @@ -41,5 +58,6 @@ def run_test(self):
  58 |          # malformed signature provided
  59 |          assert_raises_rpc_error(-3, "Malformed base64 encoding", self.nodes[0].verifymessage, 'mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB', "invalid_sig", message)
  60 |  
  61 | +

w0xlt commented at 1:23 PM on August 8, 2022:

nit: this extra line can be removed.

w0xlt approved
w0xlt commented at 1:24 PM on August 8, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25782/commits/68006c10abbfec0f16b90efa69b7880a5e17f186
achow101 merged this on Aug 8, 2022
achow101 closed this on Aug 8, 2022
theStack deleted the branch on Aug 8, 2022
sidhujag referenced this in commit 3841def7ee on Aug 9, 2022
bitcoin locked this on Aug 8, 2023

Contributors

Labels