Announce and use fidelity bonds #872

chris-belcher · 2021-05-10T00:45:18Z

This pull request adds almost everything needed to implement fidelity bonds into JoinMarket.

Coded in separate commits for easier reviewing. The commit messages contain useful information for understanding the code so be sure to read them.

The only major feature missing is having yield generators announce fidelity bonds held in cold storage. This should be easy to add one day without needing to change the joinmarket protocol.

The actual changes to the joinmarket protocol are backward-compatible, meaning takers which don't understand fidelity bonds will just ignore the new fidelity bond messages and carry on as before.

I've also a section for the release notes. I reckon after this change is merged we should do a new release soon afterwards. For now I've called the release v0.9 in the docs (but it can be easily changed of course)

undeath

I've not done a functional review yet, only some high level nits.

I think the code would be a lot easier to read if fidelity bonds were wrapped into their own class. I'll try to refactor this part.

docs/fidelity-bonds.md

docs/release-notes/release-notes-fidelity-bonds.md

jmclient/jmclient/taker.py

jmclient/jmclient/wallet_utils.py

scripts/obwatch/ob-watcher.py

jmclient/test/test_wallet.py

jmbase/jmbase/commands.py

docs/fidelity-bonds.md

jmclient/jmclient/blockchaininterface.py

jmclient/jmclient/cli_options.py

jmclient/jmclient/wallet.py

jmclient/jmclient/yieldgenerator.py

jmdaemon/jmdaemon/message_channel.py

jmdaemon/jmdaemon/orderbookwatch.py

jmclient/jmclient/support.py

jmdaemon/jmdaemon/orderbookwatch.py

undeath · 2021-05-12T17:52:04Z

Pushed a commit to move fidelity bond proofs into their own class. I think this makes things a little easier to follow.

Also fixed some of the failing tests. Others are still failing but I don't seem to have broken any additional ones.

I didn't touch anything about the jmdaemon/jmbitcoin problem discussed above. If that is resolved the newly added jmbitcoin/fidelity_bonds.py should likely be moved to jmclient.

chris-belcher · 2021-05-13T23:29:21Z

Wrote a PR adding documentation for how the fidelity bond messages work, including why we use the intermediate certificate keypair JoinMarket-Org/JoinMarket-Docs#4

chris-belcher · 2021-05-23T21:05:11Z

Just to update you all. I'm currently working on the issue of removing the jmbitcoin dependency from the daemon side. I intend to solve it by moving all the crypto onto the client side. There will be two new messages in commands.py which are used by the daemon for requesting a signed proof. So thats the solution waxwing wrote in the first bullet point of "Either copy the current paradigm of ".

So I'd say don't do any more serious reviewing until I upload code and ping this thread again.

kristapsk · 2021-05-24T10:14:28Z

I'm currently working on the issue of removing the jmbitcoin dependency from the daemon side. I intend to solve it by moving all the crypto onto the client side.

Maybe do it as a separate PR, so that it would be more easy to review / test that part, as it will affect not only fidelity bond code?

undeath · 2021-05-24T10:27:30Z

as it will affect not only fidelity bond code?

No, the jmbitcoin dependency was only introduced in this PR.

chris-belcher · 2021-05-27T00:06:13Z

I've just pushed a bunch of commits which deal with the review comments.

For now my edits are in their own separate commits so that they can be reviewed separately from the previous commits, after the reviews they can be squished.

I've structured the commits so that commit message starting with a lowercase character I intend to later squish into earlier commits. In many cases in the commit message I write which exact commit they will be squished in.

jmclient/jmclient/fidelity_bond.py

jmclient/jmclient/support.py

jmclient/jmclient/taker.py

docs/fidelity-bonds.md

AdamISZ · 2021-06-01T15:31:06Z

Testing on regtest (will update):

✔️ wallet generation with fidelity bond active
✔️ wallet timelockaddress creation
✔️ fund wallet for yg running
✔️ start yg, check FB is announced correctly
✔️ start ob-watcher with 3 non-FB ygs and 1 FB yg, check displays info correctly in table
✔️ try sendpayment with N=3 and check it uses the FB as intended.

On the last one: I saw the expected solution (even if only probabilistic) here:

2021-06-01 22:06:49,435 [INFO]  Chose these orders: {'J52BTzp8E9yqrru8': {'cjfee': '0.000019',
                      'counterparty': 'J52BTzp8E9yqrru8',
                      'fidelity_bond_value': 1478433465244.456,
                      'maxsize': 294822053,
                      'minsize': 6575808,
                      'oid': 0,
                      'ordertype': 'sw0reloffer',
                      'txfee': 114},
 'J58EbHPgZB8a217w': {'cjfee': '0.00002',
                      'counterparty': 'J58EbHPgZB8a217w',
                      'fidelity_bond_value': 0,
                      'maxsize': 599972700,
                      'minsize': 7499999,
                      'oid': 0,
                      'ordertype': 'sw0reloffer',
                      'txfee': 100},
 'J5FEXeeH4NZLjzKO': {'cjfee': '0.00002',
                      'counterparty': 'J5FEXeeH4NZLjzKO',
                      'fidelity_bond_value': 0,
                      'maxsize': 599972700,
                      'minsize': 7499999,
                      'oid': 0,
                      'ordertype': 'sw0reloffer',
                      'txfee': 100}}

These were just some initial pass through checks. I'll test various more modes of operation shortly.

AdamISZ · 2021-06-01T15:36:30Z

showutxos needs an edit to account for timelockaddresses:

joinmarket-clientserver/jmclient/jmclient/wallet_utils.py

Lines 385 to 386 in 8295837

    
           key = wallet_service.get_key_from_addr(av['address']) 
        
           tries = podle.get_podle_tries(u, key, max_tries)

This crashes in generating the PoDLE key because the privkey is not a secp256k1.PrivateKey (or more simply, because the idea doesn't make sense), so some custom handling of this address type is needed here.

AdamISZ · 2021-06-03T14:06:50Z

I think it would make sense to have a test_fidelity_bond.py module in jmclient/test .. probably just a couple of test cases of creation, serialize/deserialize, and verification.

chris-belcher · 2021-06-04T12:44:29Z

There are some tests like that in test_orderbookwatch.py. More can be added or moved to test_fidelity_bond.py of course

https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/872/files#diff-8a47ea23d85bca58e6622c4f712ef1023c2678bfe2193d4cfbafe7ec1b3e3054

Previously fidelity bond wallets were always p2sh-p2wpkh wallet, but now joinmarket has moved over to native segwit.

Now that burning to OP_RETURN wont be used, if someone wants to burn coins they can just lock them up for a really long time.

Parse incoming and announce outgoing fidelity bond messages Fidelity bond proof messages will be checked and added to the internal database just like offers. Such messages are not announced in public but only directly to takers who ask for them, this is because the signature proofs must commmit to the maker's and taker's IRC nicknames in order to avoid replay attacks.

If a yield generator is run with a fidelity bond wallet then the most-valuable bond will be found and announced. The announcement includes a proof of a UTXO and its locktime. Also a proof that the maker's IRC nickname controls the UTXO. There is also an intermediate signature called the certificate signature which can later be used when holding fidelity bond UTXOs in cold storage without the protocol needing to change. Right now this feature is unused so certificates are generated dynamically on each request. The certificates have an expiry block height, which is defined as the number of 2016-block retargeting periods since the genesis block, so to check if the expiry was passed the taker will check `current_block_height > cert_expiry*2016`.

chris-belcher · 2021-07-16T14:09:01Z

Made the edits to create the one-pubkey-per-locktime structure, see 5178d05

That along with the calculating scripts only for mixdepth zero seems to have done a noticeable speedup in sync'ing.

AdamISZ · 2021-07-17T09:18:42Z

On d019241 :

✔️ test suite passing locally
✔️ generate FB wallet
✔️ create timelockaddress
✔️ fund timelockaddress and display funded
✔️ simple direct send from wallet
✔️ succesful single coinjoin with 3 makers, 2 FB (were chosen)
✔️ showutxos shows the timelocked utxos correctly including the lock time
✔️ showseed OK
✔️ summary OK (includes locked coins, seems fine)
✔️ displayall shows every timelock address so the output is very large. But again I think fine.
✔️ freeze seems to work fine, the earlier comments still apply of course
✔️ tumbler run with FB-enabled taker wallet (locked coins get ignored as intended). Makers as mentioned above, FB always chosen this time (this is statistical of course; there were only about 8 transactions).
✔️ dumpprivkey OK
✔️ importprivkey OK
✔️ BIP78 Payjoin CLI working with both sides FB wallets (and having coins locked, and using that mixdepth).
✔️ tumbler run with taker v0.8.3 - OK

Note: First sync on generating the wallet took around ~ 15 seconds. After that, pretty much immediate in all future runs of wallet-tool. So a big improvement on this point, here.

AdamISZ · 2021-07-20T11:03:18Z

I've reviewed 5178d05 specifically, in as much detail as I could. I see no problem worth mentioning.

This is now tACK from me again.

Add two new pages for the orderbook watch html server. One displays each fidelity bond individually, the other shows calculations about how expensive a successful sybil attack would be against the current fidelity bonds in the orderbook.

Verify the fidelity bond proof on the taker client side.

This is in the best interests of takers because it will incentivize more and greater fidelity bond usage. Takers are still asked what they want their maximum fee to be, this commit only changes the randomly generated suggestion.

Previously the showutxos method would crash when encountering a timelocked UTXO

Previously there would be a crash if the wallet receiving a payjoin had a timelocked UTXO.

Previously importing a private key would result in a crash

This commit changes how timelocked addresses are created from the seed and bip32 tree. A good thing to do would be to have each locktime (e.g. 1st jan 2020, 1st feb 2020, 1st march 2020, etc) actually use a different pubkey from the HD tree (i.e. ("m/84'/1'/0'/2/0" + 1st jan 2020) ("m/84'/1'/0'/2/1" + 1st feb 2020), etc). This now means that the sync code doesnt need to know what keys have been associated with a fidelity bond to scan for the next one. Previously when a user funded a single timelocked address, the wallet will generate _another_ pubkey and import _another_ ~960 addresses, so funding one address would actually mean watching and generating ~1920 addresses not ~960. This should help with the problem found by some people that fidelity bond wallets are slower to sync. Other optimizations are possible but the structure of fidelity bond wallets will probably be fixed for decades, so this change is worth doing now.

undeath reviewed May 10, 2021

View reviewed changes

ghost reviewed May 10, 2021

View reviewed changes

scripts/obwatch/ob-watcher.py Show resolved Hide resolved