JoinMarket-Org / joinmarket-clientserver

Bitcoin CoinJoin implementation with incentive structure to convince people to take part
GNU General Public License v3.0
733 stars 179 forks source link

Tumbler doesn't honor tx_fees setting #784

Closed tschaboo closed 3 years ago

tschaboo commented 3 years ago

I specified tx_fees = 40000 in joinmarket.cfg But the first transaction had a mining fee of >140 sat/vB. The second one was within range, but the third one was again >100 sat/vB. The fourth was within range again.

AdamISZ commented 3 years ago

I will investigate a bit now, but: if there are any other details you think might help, without revealing private info, do jot them down. Trivial things: joinmarket version, whether you use command line or gui, also any delta: have you run tumbler before without this issue, if so has anything else changed.

AdamISZ commented 3 years ago

Another point: for specifically those transactions where you see the excessive tx fee, are they sweeps? (It's easy to tell if you're not sure - it's the ones where the taker doesn't have a change output, so the number of outputs is odd). Note that the new algo as of last year has a sweep for each mixdepth already containing coins at the start of its process.

AdamISZ commented 3 years ago

Let me give a bit of context on the above question about sweeps. For reference, here is the place in the code where fee is being estimated for sweeps:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/a5e8879d119c8702476da32957d2cfecc3584c89/jmclient/jmclient/taker.py#L315-L328

That comment is partly stale, so best ignore it. Consider the sequence of events:

If you want to continue to trace the actual logic, you should look at Taker.receive_utxos where you will see how my_txfee is calculated from this self.total_txfee and how it is calculated differently for non-sweeps - because in that case we can wait to set the transaction fee value after we already have the correct number of inputs.

So this approach comes from the early days of the project. There wasn't felt to be a big problem with the fact that sweeps are a bit vague about the exact fee per byte, since it wasn't a huge deal then. Now, for all but the largest denomination users, the fee is certainly something you want to control.

Can we fix this? If we readjusted the network fee after we'd got the offers, and thus changed our coinjoin amount, we could try requesting to the same counterparties again with a new coinjoin amount. I think it's possible but messy, since you'd have to account for the fact that there is no guarantee a re-request to the same counterparties will result in the same amounts offered.

Another approach might be: (and I think this could be best, albeit again imperfect, but at least simple): if the prepared tx fee after this negotiation ends up x% higher than we originally intended, abort and try again.

Unless I'm being dumb and have forgotten something else (not uncommon!), this indicates to me something we should examine carefully. I would appreciate input from @chris-belcher @kristapsk and anyone else who follows Joinmarket's code.

tschaboo commented 3 years ago

@AdamISZ Excellent analysis, and I think you guessed right, the affected TXs were indeed sweeps, yes, and the unaffected ones were no sweeps.

I'm running version commit d90661580d786914749dfcb8f47058d7f627a121 from the command line. This is only the second run of the tumbler script and I didn't watch the first run closely and don't have the log anymore.

I don't know the protocol well enough to judge if this is possible or desirable, but what about the following approach: if the tx fee would end up being x% higher than intended the user has two options, set by a setting in the .cfg: a.) continue with the sweep, accepting the higher fee, or b.) leave the dust behind in the wallet. (In the current mixdepth, or maybe back to mixdepth 0?)

chris-belcher commented 3 years ago

Good analysis @AdamISZ. Another option is adding new protocol methods that have the taker say something like "i want to do a sweep, i only know the approximate coinjoin amount right now but send me your UTXOs". Then the taker calculates the exact coinjoin amount after the fact and tells the makers what it is. This would allow the taker to configure the exact precise miner fee they want. I think this approach fixes the problem fundamentally, though I appreciate extending the protocol involves a bit of hassle. Perhaps the protocol change could be as simple as adding an optional field to one of the taker-to-maker-messages called something like "new coinjoin amount" which just updates what the coinjoin amount is. It's perhaps worth doing this now because we can expect miner fees in bitcoin to become bigger over time, and cause this issue.

The option of aborting and try again with another estimate of the coinjoin amount also should work more-or-less, and is much easier to do. Although with it the taker will never be able to choose the miner fee exactly.

Leaving dust behind in the wallet probably isn't a good idea, because adding a dust output now adds more bytes to the transactions which changes the miner fee again. Also sweeps are great for privacy because they don't have a change address, so creating a change address undoes that.

AdamISZ commented 3 years ago

Another option is adding new protocol methods that have the taker say something like "i want to do a sweep, i only know the approximate coinjoin amount right now but send me your UTXOs".

Right it's clearly fixable with another round, but it's just too huge of a change to actually change the negotiation protocol here, although it could for sure be added to a list of changes if and when we do change the negotiation protocol.

For now I'll make a PR that adds an x% above and below limit, configurable in the config file, I think I'll go with 50% as a default. Realistically a user needs to conceptualize it as that they're choosing to pay for, say, 2-3 inputs per counterparty (for example) and then deal with the fact that the actual rate will be different/unpredictable for a sweep. (Note: "above and below" - it's always possible that it ends up being too low because of counterparties proposing way more than 2-3 inputs, too).