Privacy improvements for direct_send() #492
Conversation
|
Concept ACK. Further reading: JoinMarket-Org/joinmarket#755 Do we also need to set the nSequence numbers? See undeath's comment in that thread. |
| @@ -605,11 +605,7 @@ def on_tx_received(self, nick, txhex): | |||
| "value": new_change_amount}) | |||
| new_ins = [x[1] for x in utxo.values()] | |||
| new_ins.extend(my_utxos.keys()) | |||
| # set locktime for best anonset (Core, Electrum) - most recent block. | |||
| # this call should never fail so no catch here. | |||
| currentblock = jm_single().bc_interface.rpc( | |||
There was a problem hiding this comment.
As @chris-belcher pointed out in another recent PR thread, it's a failure of the abstraction to be referring to an rpc method here, it should be the business of the blockchaininterface (not even the walletservice, really - although currently there are rpc calls there, we should change that albeit it's not urgent) that rpc is being used, client shouldn't know about it. Could you maybe create a get_block_height() or similar function in walletservice which the get_default_locktime() uses, and is also called here?
Just for other readers, want to be clear: And that re: the locktime, our current code used latest block (except direct send again), but we should do better in matching the pattern to Core and Electrum as explained above. Thanks for this PR and the careful code reading :) concept ACK though see above architectural quibble. |
jmclient/jmclient/wallet.py
Outdated
| @@ -92,6 +93,17 @@ def estimate_tx_fee(ins, outs, txtype='p2pkh'): | |||
| raise NotImplementedError("Txtype: " + txtype + " not implemented.") | |||
|
|
|||
|
|
|||
| def get_default_locktime(): | |||
There was a problem hiding this comment.
Why is the function called get_default_locktime() ? What's default about it. The name doesn't make much sense IMO. Maybe get_anti_fee_snipe_locktime() or compute_tx_locktime() or get_tx_locktime().
* Shuffle order of tx inputs and outputs * Set locktime for best anonset (Core, Electrum)
|
Added |
|
utACK |
|
W.r.t. It's interesting to note that this has something in common with another function in the same file, This is kind of what i was trying to avoid with the WalletService architecture. But since I don't think that's critical at all, utACK to c23808f |
c23808f Privacy improvements for direct_send() (Kristaps Kaupe) Pull request description: * Shuffle order of tx inputs and outputs * Set locktime for best anonset (Core, Electrum) Also make P2EP use the same locktime code instead of copy/paste. Note that Core / Electrum does not always set current block height as locktime, at random cases it sets some older value. This code does the same. ACKs for top commit: AdamISZ: But since I don't think that's critical at all, utACK to c23808f Tree-SHA512: d0fb838b4fdee8be3ce8c1f38f110655697bf77c361a00b299128e7af5da8dc82aca7b7302b610b25a9037493a97496263a83d504d04cfa88070bc0032781885
Also make P2EP use the same locktime code instead of copy/paste. Note that Core / Electrum does not always set current block height as locktime, at random cases it sets some older value. This code does the same.