Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement payjoin (p2ep) direct payment joins #279

Merged
merged 1 commit into from Jan 19, 2019

Conversation

Projects
None yet
3 participants
@AdamISZ
Copy link
Member

commented Jan 5, 2019

  • update mktx() to allow optional locktime setting (and sequence)
  • add a mk_shuffled_tx method to the wallet module
  • add a P2EPTaker and P2EPMaker class (inherit from Taker, Maker)
  • add a -T option to sendpayment script for doing payjoins
  • add a receive_payjoin script for receivers.
  • add payjoin tests in jmclient/test/test_payjoin.py
  • add a custom utxo selection method select_one_utxo to support.py
@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2019

This isn't really ready to merge, but it isn't far off. A few details still to address.

  • want to make the fee estimator a function of the wallet (e.g. you can see in the current test we try with each of 3 wallet types, but the txfee estimator is hard coded as p2sh-p2wpkh; obv to be fixed, the easy way is to make a little convenience function or attribute that converts from the wallet class to the fee type string (currently we have 'p2pkh', 'p2sh-p2wpkh' and I think p2sh multisig, we can edit this.

  • @undeath there's an ugly hack (although trivial/unimportant) to overwrite the utxo selection method here; please let me know how you think we should address that, because the selector is fixed at time of wallet construction as currently, prob we just need a setter method?

  • will need a bit more test cases - test the fallback transaction, and I'll have a think what non-manual testing I can add for the backend stuff.

  • A few more trivial changes are probably needed with the user interface on the command line. By the way I haven't tried to do any GUI work for this yet, although I kept it in mind.

For the reasoning and thinking, especially about how coins are selected, please be sure to read not just the gist but also the comments here

@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

* want to make the fee estimator a function of the wallet (e.g. you can see in the current test we try with each of 3 wallet types, but the txfee estimator is hard coded as p2sh-p2wpkh; obv to be fixed, the easy way is to make a little convenience function or attribute that converts from the wallet class to the fee type string (currently we have 'p2pkh', 'p2sh-p2wpkh' and I think p2sh multisig, we can edit this.

On this, I'm just being a dumbass, we already have BaseWallet.get_txtype() so obviously using that, I've just updated it now to include the p2wpkh case (should have done that in the BIP84 PR, but didn't notice).

@AdamISZ AdamISZ force-pushed the p2ep branch from 89cca97 to bb3d841 Jan 6, 2019

@undeath

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

the selector is fixed at time of wallet construction as currently, prob we just need a setter method?

looking at the code, I think passing an optional argument select_fn to wallet.select_utxos that overrides the default selection algorithm would be the cleanest way of handling this.

@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

the selector is fixed at time of wallet construction as currently, prob we just need a setter method?

looking at the code, I think passing an optional argument select_fn to wallet.select_utxos that overrides the default selection algorithm would be the cleanest way of handling this.

+1 , thanks.

@AdamISZ AdamISZ force-pushed the p2ep branch 2 times, most recently from 078e6f0 to c962239 Jan 15, 2019

@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2019

A note for prospective reviewers: the most security critical part of the code is, of course, the process of transaction construction and signing. This is located here for the "taker" (who is the sender of coins) and here for the "maker" (the receiver). It's a multi-step process in each case that's heavily commented.

You should also find useful to note the testing process as shown here, which includes detailed final checks on the final coinjoin transaction in terms of balances for the two parties. It also checks that the fallback non-coinjoin is valid with testmempoolaccept here.

Other important functionality of course is in the client_protocol (jmclient) and daemon_protocol (jmdaemon) code; this is not so easy to review if you're not already deeply familiar with that part of the code, though. Of course it would be great if anyone could though.
The changes to jmbitcoin (extending mktx function mainly) are pretty trivial.

@chris-belcher

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

I've reviewed c962239. It looks good to me.

@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2019

6cd60ef fixes what was actually a bug, that never mattered before: script_to_address argument is always a byte not an integer, but this only cropped up when checking the version byte for the hrp in bech32.

So fixed it for that (bech32/BIP84 payjoin is now tested working on mainnet, at least that's my report!).

320cc31 - this is forwards compatibility. Added two bytes to the end of the exchanged !pubkey messages (in line I think with an earlier comment); sender can offer (min,max) supported version and receiver can choose. Current version is crude - both send '00'*2 (hex) and sender checks exactly for 00,00 (i.e. it checks the receiver has chosen version 0), receiver checks that the first ('low' end of range) is 00. This was the minimal logic required so that a future code version could implement a protocol version 1, but still support old bots on version 0. (New bot as sender would send 00 01, and revert to old protocol if response is 00 00, and new bot as receiver would respect the 00 00 from version 0 bots, while upgrading to new version if it receives 00 01 (and would respond 01 01).

@AdamISZ

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2019

62d6d0d - this is obviously kludgy, but at least sanity checks to avoid the case where accidentally or not, the taker ends up paying a huge btc tx cost for receiver giving too many inputs.

Note how this is distinct from the already existing check that the fee per kilobyte chosen by the receiver is in an acceptable range.

A future improved version will probably share the fee cost in an equitable manner between the two parties based on their utxo contribution. Don't want to get into that for first version.

Implement payjoin (p2ep) direct payment joins
- update mktx() to allow optional locktime setting (and sequence)
- add a mk_shuffled_tx method to the wallet module
- add a P2EPTaker and P2EPMaker class (inherit from Taker, Maker)
- add a -T option to sendpayment script for doing payjoins
- add a receive_payjoin script for receivers.
- add payjoin tests in jmclient/test/test_payjoin.py
- add a custom utxo selection method select_one_utxo to support.py
- support bech32 wallets (SegwitWallet, p2wpkh) with native=true
  in config POLICY for PayJoin and direct send (not Joinmarket CJ)
- add a PayJoin.md usage guide in docs/
- include version bytes in pubkey message for forward compat
- taker pays fees but controls size (utxo number and fee/kB)
- add P2WPKH fee estimator
- Enforce INFO level logging in payjoin
- refactors regtest config settings into one place
- bugfix: script_to_address vbyte argument is bytes not integer

@AdamISZ AdamISZ force-pushed the p2ep branch from 62d6d0d to 28abddf Jan 19, 2019

@AdamISZ AdamISZ merged commit 28abddf into master Jan 19, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

AdamISZ added a commit that referenced this pull request Jan 19, 2019

Merge #279: Implement payjoin (p2ep) direct payment joins
28abddf Implement payjoin (p2ep) direct payment joins (AdamISZ)

@AdamISZ AdamISZ deleted the p2ep branch Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.