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

Warn user if estimated tx fee is higher than 5% #367

Merged
merged 1 commit into from
Aug 18, 2019

Conversation

AlexCato
Copy link
Contributor

@AlexCato AlexCato commented Jul 8, 2019

Estimate tx fees for sendpayment or full tumbler run and warn the user if the tx fee estimation exceeds (bikesheddable) 5% of the funds to be coinjoined/tumbled.

Reasons:
We've recently had a user who accidently spent 20% of his whole coinjoin amount on tx fees.

This change warns a user in the command line tools tumbler.py (user must confirm) and sendpayment.py (the warning will be displayed right before the user enters his wallet password - no extra confirmation therefore).

@AlexCato
Copy link
Contributor Author

AlexCato commented Jul 8, 2019

Just noticed: i treated the tumbler's fee estimation by counting it as one big TX with the number of total makers according to schedule. That is not correct. I'll update this PR tomorrow.

Checked: the result does not change, as the tx fee estimation code just assumes a fixed sat per participant anyways, regardless how many TXs there actually are.

So this rough estimation is good enough to reach its goal, to warn users when the tx fee costs will be too high.

@AlexCato
Copy link
Contributor Author

AlexCato commented Jul 9, 2019

I did only test this on the command line, not the GUI.
My machine with JM/bitcoin core only can do command line.

@@ -115,6 +115,17 @@ def main():
options.txfee))
assert (options.txfee >= 0)

# From the estimated tx fees, check if the expected amount is a
# significant value compared the the cj amount
exp_tx_fees_ratio = ((1 + options.makercount) * options.txfee) / amount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly a reasonable idea, but are we now checking both percentage and absolute sanity checks for fees, with different code? I think so. I think this area needs significant rationalisation (look at the use of txfee_default in Taker.py ; I actually see one minor error there already, in the P2EP code, but leave that aside, there is still a need for rationalisation how the pre-tx-construction estimation works).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct it's different code checking for absolute and relative fees.

Though I'd argue that the absolute check is just the configurable value absurd_fee_per_kb from the joinmarket.cfg file. That's arguably just there to prevent excesses in the fee market or joinmarket misconfigurations. But it won't help at all against the issue we've seen sometimes, that takers use joinmarket for very small amounts and are shocked to find that a huge percentage of their funds went to fees (which were well below the absurd_fee_per_kb amount).

While we could argue that we can just get rid of the absurd_fee_per_kb setting if this PR is merged, I'd beg to differ: in the extreme case, e.g. when I wanted to mix 100 BTC, a misconfiguration might lead to set the tx fee to something like 500000 sat/kbyte which will not be caught by this PR (still too small, percentage-wise) but would be caught by the absurd fee setting.

Or did I maybe misunderstand your point? I'm happy to re-check any tx_fee safeguards and re-write the PR.

AlexCato pushed a commit to AlexCato/joinmarket-clientserver that referenced this pull request Aug 18, 2019
@AlexCato AlexCato force-pushed the warn_on_high_txfees branch from d802b02 to 0b43df0 Compare August 18, 2019 10:12
warn the user if the tx fee estimation exceeds 5% of
the funds to be coinjoined/tumbled.
@AlexCato AlexCato force-pushed the warn_on_high_txfees branch from 0b43df0 to af11116 Compare August 18, 2019 10:14
@AdamISZ
Copy link
Member

AdamISZ commented Aug 18, 2019

After some back and forth on IRC and some tweaks to the code above, tACK from me and I will merge.

My comments about concern about the scattered fee handling code still very much apply. That is mostly orthogonal to this PR, which does create a real improvement (probably necessary) for users, but the problem continues and so far has only gotten worse. The cognitive load on a reader of the code trying to understand fee handling is way too high and mistakes are basically inevitable until we rationalize it.

About the logic of the code used to make the percentage calculation in tumbler.py: it is slightly confusing, but in order to properly estimate the overall miner transaction fees on a relative basis, we must measure the contents of the wallet to be tumbled. Usually, the user will place all their funds in the starting mixdepth (which is almost always mixdepth 0, but may not be, see - m in help), but it may occasionally be the case that some of the funds to be tumbled are between options['mixdepthsrc'] and options['mixdepthsrc']+options['mixdepthcount'], so these must be included else the percentage calculation will be (way) off.

tACK means: tested, only on regtest, both tumbler and sendpayment, creating triggering and non-triggering scenarios for the warnings, and both accepting and rejecting after warning.

AdamISZ added a commit that referenced this pull request Aug 18, 2019
af11116 Estimate tx fees for sendpayment or full tumbler run and warn the user if the tx fee estimation exceeds 5% of the funds to be coinjoined/tumbled. (AlexCato)
@AdamISZ AdamISZ merged commit af11116 into JoinMarket-Org:master Aug 18, 2019
AdamISZ added a commit that referenced this pull request Nov 18, 2019
PR #367 and follow up edits were designed to give a sanity
check to users for fees, but require specifying a payment
amount, this could be generalised to custom schedules but
for now the simplest change is to remove this check for
schedules. Thanks to @roshii for flagging the error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants