JoinMarket-Org / joinmarket-clientserver

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

Proposal - minimum txfee rate for makers #721

Open kristapsk opened 4 years ago

kristapsk commented 4 years ago

I was thinking about this for some time already, but now, in a high fee environment, this becomes more critical.

Currently taker is the one who decides on Bitcoin transaction fees and maker has no saying there. This can lock up UTXO's of a makers for long time, which they might not want. I propose we add minimum txfee rate setting for a makers, so they don't participate in coinjoins with txfees too low.

This can be implemented in two steps:

  1. Minimum txfee rate setting is added to joinmarket.cfg and makers just refuse to participate in coinjoin transactions with lower fee. Downside here is that takers don't know about maker's settings, so it will result in a coinjoins with less privacy / counterparties.
  2. JoinMarket protocol is improved so that makers announce minimum txfee rates they are ready for, so that takers don't choose makers with incompatible txfee setting.
openoms commented 4 years ago

Very good idea to implement this in some form! I wonder if it would make sense to have the option to make the min_tx_fee setting dynamic, just like it is for the tx_fees: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/5f92b7915b0c1557990163f827964bb17c3ea443/jmclient/jmclient/configure.py#L206

The default could aim for a higher target like 30, so it is significantly lower than the Taker's default (tx_fees=3 ).

Current example:

$ bitcoin-cli estimatesmartfee 3
{
  "feerate": 0.00297104,
  "blocks": 3
}

$ bitcoin-cli estimatesmartfee 30
{
  "feerate": 0.00155404,
  "blocks": 30
}
AdamISZ commented 4 years ago

Yes this was identified as a possible improvement before. I agree with it.

The thing to do is to build a longer list of improvements to the protocol, because this is Joinmarket's "hard fork", so if we did it, we'd want to improve everything we possibly could at the same time. The old docs repo that has a description of the protocol might be a good place to start.

If you want to go forward with this @kristapsk i am happy to support. Obviously would be a longer term thing.

kristapsk commented 3 years ago

This seems to be a very much requested feature from people on Telegram in the current high fee environment.

chris-belcher commented 3 years ago

Such a protocol update could be backward compatible with non-updated takers (a "soft fork" if you want to use bitcoin consensus change terminology, but unlike in bitcoin there's no consensus requirement so no need for everyone to adopt the new protocol at the same time)

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/5eee9b3a823cd4304178a05ba61445f11aa3cfa4/jmdaemon/jmdaemon/message_channel.py#L768-L789

By my reading the code accesses the _chunks variable index-by-index, up to index 5. So if we coded makers to send another value in index 6 then old takers would just ignore it.

So the update could work in a combination of OP's options 1 and 2:

This minimum_tx_fee_rate value in the offers could be dynamically calculated with fee estimators if needed as openoms said. Thought its worth noting in adversarial conditions fee estimators stop working. (i.e. if miners are censoring and mining transactions based on some other criterion other than the transactionfee)

BitcoinWukong commented 3 years ago

So the update could work in a combination of OP's options 1 and 2:

* Write code so that makers send a new variable `minimum_tx_fee_rate` at index 6.

* At the same time, write code so that updated takers will read this new variable and take it into account when choosing makers.

* If makers receive a coinjoin with a fee rate too low, then they refuse to participate (OP's option 1 but this will only happen with non-updated takers who get unlucky)

I'll take a shot at this and try to implement it.

kristapsk commented 1 year ago

As we are again in situation where mempool hasn't cleared for some time, this could seem important for a lots of makers again.

GerdOpp commented 1 year ago

Hi, I wonder if are there any new developments for this issue? Without such feature I fear to run the YieldGenerator at all since I had a tx stuck when the ordinals thing started.

kristapsk commented 1 year ago

AFAIK, nobody is working on this currently.

There are two parts in this proposal, implementing (1) is simplest, but not optimal, would end up with coinjoins with less counterparties for low time preference takers, who want to have lower tx fees. Then again, if people start to be afraid to run maker bots currently, it's worth doing this first. I will look at it!

kristapsk commented 1 year ago

When working on (2) need to remember that each Bitcoin node may have different fee estimations for different confirmation targets. It should be configurable at maker side both as exact fee rate or confirmation target, but should be announced only as exact fee rate. Which means - should be changed after each new confirmed block. Changing offers after each block was already proposed in https://github.com/JoinMarket-Org/joinmarket-clientserver/pull/1042#issuecomment-945231395, but for different reasons (privacy).

kristapsk commented 1 year ago

This has become top priority again, people on Telegram are asking for this. Will try to finish something soon.

AdamISZ commented 1 year ago

Currently taker is the one who decides on Bitcoin transaction fees and maker has no saying there. This can lock up UTXO's of a makers for long time, which they might not want. I propose we add minimum txfee rate setting for a makers, so they don't participate in coinjoins with txfees too low.

It's worth considering that there are two complementary problems: the maker's problem is that he has no idea what feerate the taker chooses to construct the transaction (hence, this issue). The taker's problem is that he has no idea how many utxos each maker will contribute.

These problems interact. If we have takers announce a specific feerate at the start, they commit to it, and then the makers provide 100 utxos, the taker may be unable (and certainly, unwilling! to complete). - uh what was i writing there? takers aren't announcing feerates. sorry ignore that part. -

This certainly isn't an argument that you can't address only the first of the two problems, but it's something to consider. In an ideal world you'd address both together.

Re @chris-belcher 's suggestion in the above comment, agreed, this should work backwards-compatibly. so I guess 3 further code additions after that one: 1/ in maker, in Maker.on_tx_received, after successfully deserializing the CTransaction object, check its fee (you might need to use jm_single().bc_interface.get_transaction(), see bumpfee.py for how they do it there; not sure) and then just return if it's below threshold. Then, 2/ In jmclient.support.choose_orders you'd need to add another line or two of logic in the starting part, that just filters out any makers not matching a rule. You'd add a rule for this like "filter out any maker whose min_tx_fee setting is higher than my chosen fee rate" using the min_tx_fee in the maker data in the orderbook dict. 3/ would be to add the updating value of min_tx_fee to the offers we send out as maker, when there is a change in our state due to transaction seen, or confirmed, so, in answer to the following:

When working on (2) need to remember that each Bitcoin node may have different fee estimations for different confirmation targets. It should be configurable at maker side both as exact fee rate or confirmation target, but should be announced only as exact fee rate. Which means - should be changed after each new confirmed block.

... the thing to do is to change the offer that is automatically updated in YieldGenerator.on_tx_unconfirmed and YieldGenerator.on_tx_confirmed. Insert/update the field min_tx_fee in the offerlist dict which will then be sent via the client-daemon protocol to the backend for announcing. You'll also have to tidy that up by setting the first offer announcement in the same way: add the min_tx_fee field in YieldGeneratorBasic.create_my_orders().

This new extra field in the offer, should be serialized correctly over the wire automatically (see MessageChannelCollection.announce_orders; it doesn't care how many fields there are.

I think that's it. Maybe?

Those are some rough notes on how I see it. Are you planning to do this imminently @kristapsk ? I'd be happy for you to do it, but if you like, I can do it instead, probably in the next couple of days.

One thing I'm fairly sure of is, whoever does it, it is not going to be a perfect solution, and assuming it works well, it will lead to a higher failure rate and slower coinjoins for a while. But then, for most users, coinjoins aren't very practical most of the time today with the fees as-is.

kristapsk commented 1 year ago

@AdamISZ I have some WIP code already, currently struggling with some test failures, some code is still missing (for example, displaying minimum feerate in orderbook watcher) and haven't done any manual testing. But I could open draft PR.

AdamISZ commented 1 year ago

@AdamISZ I have some WIP code already, currently struggling with some test failures, some code is still missing (for example, displaying minimum feerate in orderbook watcher) and haven't done any manual testing. But I could open draft PR.

Yeah good idea, re: open draft.

I did think of the orderbookwatcher part (adding the min fee rates as another column and so on), but I guessed it would just ignore the extra field, apparently not :)