RPC-API: Implement message signing

[kristapsk]

Resolves #1533.

My first attempt at adding new RPC-API endpoint, there could be mistakes.

Didn't add tests in jmclient/test/test_wallet_rpc.py as message signing is currently supported for mainnet only, not regtest (but I tested HTTP 400 response with regtest).

[AdamISZ]

This does make sense, but, I find myself wondering - would it be better if we do the address -> hdpath conversion ourselves in joinmarketd? I don't think at the API client level (so, JAM), they necessarily have access to a conversion function between address and path, but we do (in wallet.py or cryptoengine.py).

[kristapsk]

I just mimicked behaviour of wallet-tool.py. Should we accept both path or address here in your opinion?

[AdamISZ]

I think the workflow probably starts with a user wanting to sign on a specific address. Of course nothing wrong with path also, though.

[AdamISZ]

Thanks, looks good.

Didn't add tests in jmclient/test/test_wallet_rpc.py as message signing is currently supported for mainnet only, not regtest (but I tested HTTP 400 response with regtest).

Good point, hmm, that is annoying.

[AdamISZ]

Thanks, looks good.

Didn't add tests in jmclient/test/test_wallet_rpc.py as message signing is currently supported for mainnet only, not regtest (but I tested HTTP 400 response with regtest).

Good point, hmm, that is annoying.

Maybe you can change the network on-the-fly in the test: use a call like seen in this code:

joinmarket-clientserver/jmclient/jmclient/configure.py

Lines 901 to 911 in fa17236

	if source == 'bitcoin-rpc': #pragma: no cover
	bc_interface = BitcoinCoreInterface(rpc, network,
	rpc_wallet_file)
	if testnet:
	btc.select_chain_params("bitcoin/testnet")
	else:
	btc.select_chain_params("bitcoin")
	elif source == 'regtest':
	bc_interface = RegtestBitcoinCoreInterface(rpc,
	rpc_wallet_file)
	btc.select_chain_params("bitcoin/regtest")

, remembering to change it back to whatever it was, after finishing the signmessage test.

[kristapsk]

Maybe you can change the network on-the-fly in the test

Or maybe it's simpler and more elegant to just allow message signing with regtest / signet / testnet too?

[AdamISZ]

Maybe you can change the network on-the-fly in the test

Or maybe it's simpler and more elegant to just allow message signing with regtest / signet / testnet too?

I didn't answer this last week because I couldn't remember why I/we only allowed this for mainnet. Now I look at the code it really doesn't make any sense; the function sign_message in wallet.py doesn't depend in any way on network; only the first element of the return tuple, the address, does, and that should be handled in cryptoengine.py. So I suppose only two comments: test if, with that restriction removed, it works correctly, and secondly have a quick thought about if there's any gotchas or risks for users, if we allow it.

[kristapsk]

Rebased.

There could be improvements with regtest / testnet / signet signing and tests, but I think this is already useful, that could addressed later.

@theborakompanioni WDYT?

[theborakompanioni]

Rebased.

There could be improvements with regtest / testnet / signet signing and tests, but I think this is already useful, that could addressed later.

@theborakompanioni WDYT?

I think it is nice. Great work. 💪
Will try to add this functionality to the next Jam milestone. 🙌
However, from what I understand, it does not work with other networks than mainnet, right? That is very unfortunate.. but not a deal-breaker of course.

[kristapsk]

However, from what I understand, it does not work with other networks than mainnet, right? That is very unfortunate.. but not a deal-breaker of course.

Yes, currently only mainnet. Can be added for other networks too, but that should be separate PR, has nothing to do with RPC API itself.

[kristapsk]

Rebased

[kristapsk]

Rebased

[kristapsk]

CI test failure is not related to these changes.

[kristapsk]

Rebased

[kristapsk]

Rebased

[kristapsk]

Rebased to run OpenAPI Diff CI action.

[amitx13]

Hey @kristapsk, I'm currently working on Jam issue #633, which is about signing a message. I noticed that the JM RPC-API signmessage requires us to send hd-path along with the message. However, in Jam, we only have access to a handful of addresses, not all of them. Would it be possible for the signmessage API to accept address instead of hd-path? Your assistance with this would be greatly appreciated. Thank you!

[kristapsk]

Hey @kristapsk, I'm currently working on Jam issue #633, which is about signing a message. I noticed that the JM RPC-API signmessage requires us to send hd-path along with the message. However, in Jam, we only have access to a handful of addresses, not all of them. Would it be possible for the signmessage API to accept address instead of hd-path? Your assistance with this would be greatly appreciated. Thank you!

Yes, it's possible, but needs some coding, will look at it. Signing by path was how it was originally implemented in wallet-tool.py, I just added support for that existing code to RPC API. But if there is use case, it's worth adding address support too. It was already suggested in review comment by @AdamISZ, see above.

[amitx13]

Hey @kristapsk, earlier in this conversation, you talked about allowing message signing with regtest / signet / testnet too. Can you please shed some light on that? How can someone achieve that? Like, I have added the (regtest", "testnet", "signet") networks under the sign_message function in cryptoengine.py. Here's the code:

@staticmethod
def sign_message(privkey, message):
    """
    Note: only (currently) used for manual
    signing of text messages by keys,
    *not* used in Joinmarket communication protocol.
    args:
        privkey: bytes
        message: bytes
    returns:
        base64-encoded signature
    """
    # supported in networks, i.e mainnet, regtest, testnet, and signet
    assert get_network() in ["mainnet", "regtest", "testnet", "signet"]
    k = btc.CBitcoinKey(BTCEngine.privkey_to_wif(privkey))
    return btc.SignMessage(k, btc.BitcoinMessage(message)).decode("ascii") 

Will this approach work, or is my approach wrong?

[amitx13]

I have also made some changes to the RPC-API to support the address. Please take a look at the code.
Filename : wallet_rpc.py
Original :

result = wallet_signmessage(self.services["wallet"], request_data["message"],out_str=False)

Changes_made :

hd_path = self.services["wallet"].addr_to_path(request_data["address"])
result = wallet_signmessage(self.services["wallet"],hd_path, request_data["message"],out_str=False)

Final-code:

        def signmessage(self, request, walletname):
            self.check_cookie(request)
            if not self.services["wallet"]:
                raise NoWalletFound()
            if not self.wallet_name == walletname:
                raise InvalidRequestFormat()

            request_data = self.get_POST_body(request, ["address", "message"])

            hd_path = self.services["wallet"].addr_to_path(request_data["address"])
            
            result = wallet_signmessage(self.services["wallet"],
                hd_path, request_data["message"],
                out_str=False)
            if type(result) == str:
                return make_jmwalletd_response(request, status=400,
                    message=result)
            else:
                return make_jmwalletd_response(request,
                    signature=result[0], message=result[1], address=result[2])

Filename : wallet_utils.py
Original :

 if not get_network() == "mainnet":
        return "Error: message signing is only supported on mainnet."

Changes_made :

if not get_network() in ["mainnet", "regtest", "testnet", "signet"]:
        return "Error: message signing is only supported on mainnet, regtest, testnet and signet."

Final-code:

def wallet_signmessage(wallet, hdpath: str, message: str,out_str: bool = True) -> Union[Tuple[str, str, str], str]:
    """ Given a wallet, a BIP32 HD path (as can be output
    from the display method) and a message string, returns
    a base64 encoded signature along with the corresponding
    address and message.
    If `out_str` is True, returns human readable representation,
    otherwise returns tuple of (signature, message, address).
    """
     if not get_network() in ["mainnet", "regtest", "testnet", "signet"]:
        return "Error: message signing is only supported on mainnet, regtest, testnet and signet."

    msg = message.encode('utf-8')

    if not hdpath:
        return "Error: no key path for signing specified"
    if not message:
        return "Error: no message specified"

    path = wallet.path_repr_to_path(hdpath)
    addr, sig = wallet.sign_message(msg, path)
    if not out_str:
        return (sig, message, addr)
    return ("Signature: {}\nMessage: {}\nAddress: {}\n"
    "To verify this in Electrum use Tools->Sign/verify "
    "message.".format(sig, message, addr))

Thank you for your time.

[kristapsk]

Rebased.

[kristapsk]

@amitx13 Will look at your comments later.

[kristapsk]

Merging this. It's not perfect, but already useful for Jam, so it's better than nothing. Can improve upon this later.