Add support for BIP78 payjoins to .onion receivers

[AdamISZ]

In this commit, the jmclient.payjoin module now supports
sending payments to BIP21 URIs where the pj= parameter is
to a hidden service address.
Additionally, the test/payjoinclient and test/payjoinserver
modules are edited to support optionally testing payments to
an ephemeral hidden service.

[AdamISZ]

Rebased on the bip78fallback branch as of 61c10f7

[AdamISZ]

Rebased on master as of bdd85b9 (no code change just base branch change) a couple days back.

Is anyone planning on testing this?

[decentclock]

Is anyone planning on testing this?

I'd like to test this, just not sure what's the easiest way to set up a test receiver for this situation. Maybe BTCPay server? I took a quick look at Wasabi, they don't seem to support BIP78 receive.

[kristapsk]

I could look at testing this. @AdamISZ , you don't like the idea of renaming socks5_* to onion_socks5_* at the end?

[AdamISZ]

I could look at testing this. @AdamISZ , you don't like the idea of renaming socks5_* to onion_socks5_* at the end?

Oh thanks for the reminder, I just forgot. I'll do it yeah.

Re: btcpayserver I asked in their IRC 'cos I was thinking of trying that myself. Does Wasabi not have this available yet? (onion receiver).

[decentclock]

Re: btcpayserver I asked in their IRC 'cos I was thinking of trying that myself. Does Wasabi not have this available yet? (onion receiver).

I took a quick look at the Wasabi docs, and I don't think so.

Cross checked with this link below as well.
https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#implementations

I'm currently working on setting up BTCPay server to run the test.

I'm following the testnet instructions here: https://docs.btcpayserver.org/TryItOut/

[AdamISZ]

Ah OK nice @Jules23 . Btw I am not testing it on testnet, testnet is too painful. Either regtest or mainnet for me :) Kukks tells me he has such a thing and will link it later.

[AdamISZ]

@kristapsk changed config var name as of 1983e08

[decentclock]

Btw I am not testing it on testnet, testnet is too painful.

Where's the pain point? I'm progressing quite nicely, I am now moving to set up joinmarket on testnet.

I have created an invoice with this URI, let me know if this corresponds to what we are testing.

bitcoin:tb1qkq53u32tpxk5d2k7dklsgzqcahs8uj6tane533?amount=0.00109348&pj=http://yibzxq3jdqyfjzwgatd3fqihswtrdunjdnlbmb6v6yii5g2ghntbfkad.onion/BTC/pj

[AdamISZ]

Large reorgs, blocks vary between painfully slow and every few seconds due to the weird algo, much too large (both block height and size of blockchain, kind of ridiculous how big it is, rescanning is not fun), awkward getting hold of coins sometimes, but the biggest one isn't relevant any more I guess: for a long time before and after segwit activation, there were many testnet miners (people with ASICs messing around) who weren't mining segwit transactions.

It's just not worth it until they reset it imo.

[kristapsk]

I set up local BTCPay Server testnet instance with P2SH-SegWit wallet and created Tor hidden service for it (BTCPay Server automatically creates .onion pj= URLs if site itself is opened via .onion link, no extra configuration there necessary).

First payjoin test didn't succeed, ended up with a non-payjoin payment.

...
2020-07-17 23:08:42,054 [DEBUG]  Initial nonpayjoin transaction feerate is: 1.0060240963855422                                                                                                 
2020-07-17 23:08:42,054 [DEBUG]  From which we calculated a max additional fee contribution of: 109                                                                                            
2020-07-17 23:08:42,055 [INFO]  Starting transaction monitor in walletservice                                                                                                                  
2020-07-17 23:08:46,274 [DEBUG]  Response headers:                                                                                                                                             
2020-07-17 23:08:46,275 [DEBUG]  [(b'Date', [b'Fri, 17 Jul 2020 20:08:45 GMT']),                                                                                                               
 (b'Content-Type', [b'text/plain; charset=utf-8']),                                                                                                                                            
 (b'Server', [b'Kestrel']),                                                                                                                                                                    
 (b'Referrer-Policy', [b'same-origin']),                                                                                                                                                       
 (b'X-XSS-Protection', [b'1; mode=block']),                                                                                                                                                    
 (b'X-Content-Type-Options', [b'nosniff']),                                                                                                                                                    
 (b'X-Frame-Options', [b'DENY'])]                                                                                                                                                              
2020-07-17 23:08:46,292 [DEBUG]  Receiver sent us this PSBT:                                                                                                                                   
2020-07-17 23:08:46,297 [DEBUG]  {                                                                                                                                                             
...
2020-07-17 23:08:46,323 [DEBUG]  proposed fee rate: 1.0078125
2020-07-17 23:08:46,324 [ERROR]  receiver proposed transaction has too low feerate: 1.0078125
2020-07-17 23:08:46,324 [WARNING]  Payjoin did not succeed, falling back to non-payjoin payment.
2020-07-17 23:09:42,054 [WARNING]  Payjoin did not succeed, falling back to non-payjoin payment.
2020-07-17 23:09:42,055 [WARNING]  Error message was: timeout
2020-07-17 23:09:42,076 [DEBUG]  rpc: sendrawtransaction ['...']
2020-07-17 23:09:42,080 [INFO]  We paid without coinjoin. Transaction: 
...
done
Unhandled error in Deferred:

Traceback (most recent call last):
  File "/home/neonz/git4/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/web/_newclient.py", line 1306, in _bodyDataFinished_CONNECTED
    self._bodyProtocol.connectionLost(reason)
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/web/client.py", line 2259, in connectionLost
    self.deferred.callback(b''.join(self.dataBuffer))
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 460, in callback
    self._startRunCallbacks(result)
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 568, in _startRunCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/home/user/git/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 654, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File "/home/user/git/joinmarket-clientserver/jmclient/jmclient/payjoin.py", line 603, in process_payjoin_proposal_from_server
    fallback_nonpayjoin_broadcast(manager, err="Receiver PSBT checks failed.")
  File "/home/user/git/joinmarket-clientserver/jmclient/jmclient/payjoin.py", line 545, in fallback_nonpayjoin_broadcast
    log.warn("Error message was: " + err.decode("utf-8"))
builtins.AttributeError: 'str' object has no attribute 'decode'

[kristapsk]

Hmm, but I forgot to add onion_socks5_host/onion_socks5_port to joinmarket.cfg, wondering how it got response at all...

[decentclock]

I am currently getting the error below when setting up a testnet instance, and using both wrapped and native segwit joinmarket wallets.

[ERROR] Connection was refused by other side: 61: Connection refused.

Also, in certain cases, I end up with this. The process doesn't seem to make any progress after the last line. It does surprise me because I have ~0.01 BTC, and Im sending ~0.0005 BTC. I did check that I am using the corrext mixdepth.

WARNING: Expected bitcoin network miner fees for this coinjoin amount are roughly 5.9%
You might want to modify your tx_fee settings in joinmarket.cfg. Still continue? (y/n):y
2020-07-17 16:23:37,788 [INFO]  Using tx fee: 10000 sat/vkB (10.0 sat/vB)
Unhandled Error
Traceback (most recent call last):
  File "/Users/barbac/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/base.py", line 434, in fireEvent
    DeferredList(beforeResults).addCallback(self._continueFiring)
  File "/Users/barbac/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 322, in addCallback
    callbackKeywords=kw)
  File "/Users/barbac/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 311, in addCallbacks
    self._runCallbacks()
  File "/Users/barbac/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/defer.py", line 654, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
--- <exception caught here> ---
  File "/Users/barbac/joinmarket-clientserver/jmvenv/lib/python3.7/site-packages/twisted/internet/base.py", line 447, in _continueFiring
    callable(*args, **kwargs)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/payjoin.py", line 446, in send_payjoin
    with_final_psbt=True)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/taker_utils.py", line 115, in direct_send
    utxos = wallet_service.select_utxos(mixdepth, amount + initial_fee_est)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/wallet_service.py", line 812, in select_utxos
    includeaddr=includeaddr)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/wallet.py", line 693, in select_utxos
    mixdepth, amount, utxo_filter, select_fn, maxheight=maxheight)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/wallet.py", line 259, in select_utxos
    selected = selector(available, amount)
  File "/Users/barbac/joinmarket-clientserver/jmclient/jmclient/support.py", line 81, in select
    raise Exception("Not enough funds")
builtins.Exception: Not enough funds

2020-07-17 16:23:37,810 [INFO]  Starting transaction monitor in walletservice

[AdamISZ]

Initial nonpayjoin transaction feerate is: 1.0060240963855422

You set a fee of 1 sat/byte which is right at the relay limit. That's why it didn't work, because we insist iirc on a slight bump above that (1.2).

log.warn("Error message was: " + err.decode("utf-8")) builtins.AttributeError: 'str' object has no attribute 'decode'

This is a bug in the error message code, I think someone already told me about. Thanks for the heads up I will check it out shortly.

Hmm, but I forgot to add onion_socks5_host/onion_socks5_port to joinmarket.cfg, wondering how it got response at all...

People often don't realise this, but the defaults are picked up from jmclient/jmclient/configure.py always, and only overwritten if you have something in joinmarket.cfg

[AdamISZ]

builtins.Exception: Not enough funds

Are the coins confirmed? It is possible to spend unconfirmed coins with listunspent_args in the config, but we try to have only confirmed coins spent by default. (Even if I'm right about that, we should catch a NotEnoughFunds exception really).

[decentclock]

builtins.Exception: Not enough funds

Are the coins confirmed? It is possible to spend unconfirmed coins with listunspent_args in the config, but we try to have only confirmed coins spent by default. (Even if I'm right about that, we should catch a NotEnoughFunds exception really).

If confirmed means at least 1 confirmation, then yes, the coins are confirmed.

[AdamISZ]

OK. Are they frozen? Check with freeze method. It could happen if you sent coins to the same address.
Third option is wrong mixdepth, but you already said you're sure about that.

[AdamISZ]

Re: Connection refused, it can be a configuration issue, also a question is whether the onion is serving SSL. I was discussing this a bit on IRC, I'm hoping that nobody will be serving using ssl on ephemeral onions, it doesn't really make much sense.

[AdamISZ]

So you generated the wallet (native or p2sh?), with testnet true, and then you wallet-tooled it and then you deposited, you waited for confirmation, and you see the coins in wallet-tool in mixdepth 0 (my guess), and then you get this direct-send error? Hmm. Is your joinmarket.cfg in your home dir set to testnet? Do you know you can do --datadir=. to use a joinmarket.cfg in the current directory, not interfering with your main instance? Just a few thoughts. I don't have the full information here, you've only shown a stack trace.

[kristapsk]

People often don't realise this, but the defaults are picked up from jmclient/jmclient/configure.py always, and only overwritten if you have something in joinmarket.cfg

Ah, great! Missed (or have forgotten) that.

You set a fee of 1 sat/byte which is right at the relay limit. That's why it didn't work, because we insist iirc on a slight bump above that (1.2).

Yes, did sendpayment with --txfee=2000 and testnet p2sh-segwit payjoin to BTCPay Server .onion succeeded.

[decentclock]

So you generated the wallet (native or p2sh?), with testnet true, and then you wallet-tooled it and then you deposited, you waited for confirmation, and you see the coins in wallet-tool in mixdepth 0 (my guess), and then you get this direct-send error?

Yes. I am getting this error with both native and p2sh.

Is your joinmarket.cfg in your home dir set to testnet?

Yes. network = testnet

Do you know you can do --datadir=. to use a joinmarket.cfg in the current directory, not interfering with your main instance?

I did not know, but I am not running another instance on my machine.

[AdamISZ]

@Jules23 maybe try a normal direct send payment (-N0 ) to another address in the wallet to see if you get the same not enough funds?
Edit: also see if there's a diff between this PR and master for this problem. I'm guessing maybe not.

[decentclock]

@Jules23 maybe try a normal direct send payment (-N0 ) to another address in the wallet to see if you get the same not enough funds?

I'm also getting this error here. I should have noted that at the beginning sorry.

Edit: Let me figure this problem out further, I'll let you know once I have a better understanding.

[AdamISZ]

Here are Kukks' donation endpoints (edit: sorry not literally the endpoints; this is the donation server which will generate the bip21 uri for you, of course) if anyone's interested in trying them:

<btcpay_bot> [mattermost - Kukks]@waxwing
<btcpay_bot> [mattermost - Kukks]mainnet: http://lyn4lzmpo2u3eurdbzt4w5xjg35shz3ek2pshbmrvud56ktfut6d5mad.onion/apps/Pv5B2TZkjcxFEaqntQ8zdoLT7LE/pos
<btcpay_bot> [mattermost - Kukks]
<btcpay_bot> [mattermost - Kukks]testnet:
<btcpay_bot> [mattermost - Kukks]http://m33g5t4itjerxhf2vuyrxahstcuhshfxzl2qy5mcuhti3m5erzh5y2yd.onion/apps/eFtHvBGQAkbyc7GQAN3hQBEnRQe/po

[AdamISZ]

The above worked first time for me on mainnet, see tx 0902979fcc2476d64056537116e2c2b5e1903daca3eda095a457cda336bf91bb

Note that they're v3 onions, as expected we can send to them also, fine.

[decentclock]

1. [ERROR] Connection was refused by other side: 61: Connection refused.

This was due to me not running tor when attempting a payjoin to a .onion. Can we remind users to run Tor in this case in PAYJOIN.md? A second suggestion on this point: for the vast majority of cases, the socks5_port = 9050 will be correct, except in the case where the user is running the full Tor browser, in which case socks5_port = 9150 would be correct. See here: https://2019.www.torproject.org/docs/faq.html.en#TBBSocksPort.

2. I am still puzzled on this Not enough funds error. As an example, I ran sendpayment.py to send to another testnet addres twice in quick succession. The first run failed with the error, the second did not. I will investigate this further, but it doesn't seem to me that this is an issue related to this PR.

3. In the case that the Payjoin fails, I think it would be nice to prompt the user on whether they want to abort the transaction, or carry on with a non-payjoin one. Although maybe this is for another PR.

Besides these points, I've been able to execute a payjoin with a remote testnet BTCPay server onion instance, with both wrapped and native segwit.
native:
51a5a2c04e76d10e36b79b16e6ded1959a3135dd8b035df2eb8ee9a8796d8f8e
48a106dd809625bfb17e050bda54d1f42eff9a3376469687552d42ab9b507b0e
wrapped:
0339c3a303252b285df33b221bd6cd1e64f5a63aa8fff93afc6adaaf7de66a02

[AdamISZ]

In the case that the Payjoin fails, I think it would be nice to prompt the user on whether they want to abort the transaction, or carry on with a non-payjoin one. Although maybe this is for another PR.

On balance I think not. The BIP specifies this behaviour. I agree it is a valid question/point of debate, but for now we're just implementing the BIP.

This was due to me not running tor when attempting a payjoin to a .onion. Can we remind users to run Tor in this case in PAYJOIN.md?

Agreed. I'll add something in there on this, and on the port, but of note: users have always had the same configuration requirement connecting to IRC servers. A very large fraction of our users go over tor for that in the same way (socks5 proxy, to onion), but this is distinct in as much as it may be required for a given payment URI and the user may not even know it (at least, in theory).

[kristapsk]

A second suggestion on this point: for the vast majority of cases, the socks5_port = 9050 will be correct, except in the case where the user is running the full Tor browser, in which case socks5_port = 9150 would be correct.

We could probably use some smart probing by default here (try 9050, then 9150). Timeout when connecting to localhost could be really small, I think, something like 1 second?

[AdamISZ]

Rebased on master in 91a9116

Edit: I have no idea why it's showing 3 commits (the first two are already in master). Appears to be just a glitch.

[AdamISZ]

Fixed problem in #638 (comment) with 1ee8a42

This is probably mergeable after squash now as it's passed several tests.

@Jules23 did you eventually figure out what was wrong with your testnet set up?

[AdamISZ]

Addressed the first point in #638 (comment) with 0071948

[AdamISZ]

Rebased on master again with 660aca3 - sorry for all this messing around but I can't figure out why the PR shows all the recent commits after git rebase master and git push -f origin hiddenservice-bip78. It appears to be a display glitch as the history clearly shows that only the last 3 commits are add-ons to master.

[decentclock]

did you eventually figure out what was wrong with your testnet set up?

@AdamISZ Yes! The whole time, I was trying to spend an unconfirmed coin ( the change coin from the Payjoin transaction ).

Are the coins confirmed? It is possible to spend unconfirmed coins with listunspent_args in the config, but we try to have only confirmed coins spent by default. (Even if I'm right about that, we should catch a NotEnoughFunds exception really)

When you said this, my brain was only thinking of possible unconfirmed coins coming from outside the wallet.

I wasn't realizing that when I execute a payjoin, all my balance again becomes unconfirmed as well. O well! Sorry for sowing confusion again.

Question: instead of a NotEnoughFunds error, should we tell users that some of their balance is not confirmed ?

[AdamISZ]

I wasn't realizing that when I execute a payjoin, all my balance again becomes unconfirmed as well. O well! Sorry for sowing confusion again.

No worries, it's a normal enough error. Thanks for following up.

Question: instead of a NotEnoughFunds error, should we tell users that some of their balance is not confirmed ?

A lot of us long-time runners just set listunspent_args but this is not at all obvious to new users, moreover, probably the biggest weakness of Joinmarket as a wallet is that it does very little to inform users about confirmation status. This should be fixed (both documentation, somewhat but especially code for CLI and Qt) and I don't think it is very difficult since some of the work on the wallet some months ago (starting with #359 but also some follow ups from that) means we now have confirmations data attached to all utxos.

[AdamISZ]

Attempting merge now and hoping that the above display of all the previous commits is an irrelevant display glitch.

[AdamISZ]

Nope, it's borked. Replaced this with #648 - exactly the same code, so now merging that.