RPC-API: Implement message signing #1536

kristapsk · 2023-08-22T13:34:42Z

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 · 2023-08-22T14:34:11Z

docs/api/wallet-rpc.yaml

+      properties:
+        hd_path:
+          type: string
+          example: m/84'/0'/0'/0/0


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).

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

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 · 2023-08-22T14:34:20Z

docs/api/wallet-rpc.yaml

+        - message
+        - address
+      properties:
+        signatture:


Fixed with b262d18.

AdamISZ · 2023-08-22T14:48:57Z

jmclient/jmclient/wallet_rpc.py

+            request_data = self.get_POST_body(request, ["hd_path", "message"])
+            result = wallet_signmessage(self.services["wallet"],
+                request_data["hd_path"], request_data["message"])
+            if type(result) == str:


If you set out_str to False in the call above, then you will always get the tuple returned.

The thing is, wallet_signmessage returns a str either if there is an error, or if out_str is False, so that it can output the full info string (it's for the command line, so that's why it's default). Bad design really, though not very important. The errors are only raised if the input hd path is invalid or the message is missing. For this RPC use case that doesn't apply, we input those programmatically so there's no human error. Errors just get caught by the exception raises.

So TLDR I don't think you need this if/else, but you do need to set out_str to False.

Ohh, my mistake, somehow didn't notice that default is True not False. With False it does not always return tuple, it returns tuple with successful signing, string on error. So if/else is needed.

Yes, design here isn't the best, but, OTOH, it's not the most critical functionality, so maybe not worth spending too much time redesigning it.

Addressed in 59ee058.

AdamISZ · 2023-08-22T14:50:00Z

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 · 2023-08-22T18:03:49Z

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 · 2023-08-23T10:08:16Z

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 · 2023-08-30T19:58:21Z

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 · 2023-09-20T04:08:22Z

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 · 2023-09-20T09:15:56Z

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 · 2023-09-20T13:39:06Z

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 · 2023-09-21T20:50:55Z

Rebased

kristapsk · 2023-10-06T16:12:43Z

Rebased

kristapsk · 2023-10-06T16:50:21Z

CI test failure is not related to these changes.

kristapsk · 2023-11-22T19:17:13Z

Rebased

kristapsk · 2024-01-04T22:21:19Z

Rebased

kristapsk · 2024-01-25T14:19:36Z

Rebased to run OpenAPI Diff CI action.

amitx13 · 2024-04-21T11:40:10Z

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 · 2024-04-24T12:54:33Z

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 · 2024-04-28T10:54:14Z

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 · 2024-04-28T12:53:47Z

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 · 2024-05-19T15:06:29Z

Rebased.

kristapsk · 2024-05-19T15:06:56Z

@amitx13 Will look at your comments later.

kristapsk added the RPC-API label Aug 22, 2023

AdamISZ reviewed Aug 22, 2023

View reviewed changes

kristapsk force-pushed the rpc-signmessage branch from 59ee058 to b3dfa4f Compare September 20, 2023 04:06

kristapsk force-pushed the rpc-signmessage branch from b3dfa4f to 63df0ca Compare September 21, 2023 20:50

kristapsk force-pushed the rpc-signmessage branch from 63df0ca to 8cddf24 Compare October 6, 2023 16:12

kristapsk force-pushed the rpc-signmessage branch from 8cddf24 to b5de1a1 Compare November 22, 2023 19:17

kristapsk force-pushed the rpc-signmessage branch from b5de1a1 to 0b3c464 Compare January 4, 2024 22:20

theborakompanioni mentioned this pull request Jan 8, 2024

Need an easy way to sign a message joinmarket-webui/jam#633

Open

kristapsk force-pushed the rpc-signmessage branch from 0b3c464 to 15acd02 Compare January 25, 2024 14:19

kristapsk added 2 commits May 19, 2024 18:05

RPC-API: Implement message signing

60c038d

Fix typo

4e082b3

wallet_signmessage() outstr argument must be False

58d19f8

kristapsk force-pushed the rpc-signmessage branch from 15acd02 to 58d19f8 Compare May 19, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC-API: Implement message signing #1536

RPC-API: Implement message signing #1536

kristapsk commented Aug 22, 2023 •

edited

Loading

AdamISZ Aug 22, 2023

kristapsk Aug 23, 2023

AdamISZ Aug 30, 2023

AdamISZ Aug 22, 2023

kristapsk Aug 23, 2023

AdamISZ Aug 22, 2023

kristapsk Aug 23, 2023

AdamISZ commented Aug 22, 2023

AdamISZ commented Aug 22, 2023

kristapsk commented Aug 23, 2023

AdamISZ commented Aug 30, 2023

kristapsk commented Sep 20, 2023

theborakompanioni commented Sep 20, 2023

kristapsk commented Sep 20, 2023

kristapsk commented Sep 21, 2023

kristapsk commented Oct 6, 2023

kristapsk commented Oct 6, 2023

kristapsk commented Nov 22, 2023

kristapsk commented Jan 4, 2024

kristapsk commented Jan 25, 2024

amitx13 commented Apr 21, 2024

kristapsk commented Apr 24, 2024

amitx13 commented Apr 28, 2024 •

edited

Loading

amitx13 commented Apr 28, 2024 •

edited

Loading

kristapsk commented May 19, 2024

kristapsk commented May 19, 2024

RPC-API: Implement message signing #1536

Are you sure you want to change the base?

RPC-API: Implement message signing #1536

Conversation

kristapsk commented Aug 22, 2023 • edited Loading

AdamISZ Aug 22, 2023

Choose a reason for hiding this comment

kristapsk Aug 23, 2023

Choose a reason for hiding this comment

AdamISZ Aug 30, 2023

Choose a reason for hiding this comment

AdamISZ Aug 22, 2023

Choose a reason for hiding this comment

kristapsk Aug 23, 2023

Choose a reason for hiding this comment

AdamISZ Aug 22, 2023

Choose a reason for hiding this comment

kristapsk Aug 23, 2023

Choose a reason for hiding this comment

AdamISZ commented Aug 22, 2023

AdamISZ commented Aug 22, 2023

kristapsk commented Aug 23, 2023

AdamISZ commented Aug 30, 2023

kristapsk commented Sep 20, 2023

theborakompanioni commented Sep 20, 2023

kristapsk commented Sep 20, 2023

kristapsk commented Sep 21, 2023

kristapsk commented Oct 6, 2023

kristapsk commented Oct 6, 2023

kristapsk commented Nov 22, 2023

kristapsk commented Jan 4, 2024

kristapsk commented Jan 25, 2024

amitx13 commented Apr 21, 2024

kristapsk commented Apr 24, 2024

amitx13 commented Apr 28, 2024 • edited Loading

amitx13 commented Apr 28, 2024 • edited Loading

kristapsk commented May 19, 2024

kristapsk commented May 19, 2024

kristapsk commented Aug 22, 2023 •

edited

Loading

amitx13 commented Apr 28, 2024 •

edited

Loading

amitx13 commented Apr 28, 2024 •

edited

Loading