-
Notifications
You must be signed in to change notification settings - Fork 113
fix(UTXO): improve tx fee calculation and min relay fee handling #2316
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
Conversation
fix use gas_fee in build(); fix min_relay_tx_fee calc; refactor break build() into functions rename fn get_tx_fee to get_fee_per_kb; fix utxos tests for recalculated tx fee
fix refactored UtxoTxBuilder::build(): return only txfee (w/o gas fee) with tx as it used to be
|
Is this ready for review? |
I am doing final checks and will change the status for ready after that |
|
Thank you for this PR. Covers most of what I was working on here #2083. I will close mine when this is approved. |
mm2src/coins/utxo/utxo_common.rs
Outdated
| let total_fee = if tx.outputs.len() == outputs_count { | ||
| // take into account the change output | ||
| data.fee_amount + (dynamic_fee * P2PKH_OUTPUT_LEN) / KILO_BYTE | ||
| data.fee_amount + actual_tx_fee.get_tx_fee_for_change(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data.fee_amount + actual_tx_fee.get_tx_fee_for_change(None) | |
| data.fee_amount + actual_tx_fee.get_tx_fee_for_change(0) |
mm2src/coins/utxo/utxo_common.rs
Outdated
| data.fee_amount | ||
| } | ||
| // take into account the change output | ||
| data.fee_amount + fee_per_kb.get_tx_fee_for_change(Some(tx_bytes.len() as u64)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| data.fee_amount + fee_per_kb.get_tx_fee_for_change(Some(tx_bytes.len() as u64)) | |
| data.fee_amount + fee_per_kb.get_tx_fee_for_change(tx_bytes.len() as u64) |
mm2src/coins/utxo.rs
Outdated
| } | ||
|
|
||
| /// Return extra tx fee for the change output as p2pkh | ||
| fn get_tx_fee_for_change(&self, tx_size: Option<u64>) -> u64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that tx_size can be used as u64 (see the other suggestions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed Option from get_tx_fee_for_change in 3f709fc
mm2src/coins/utxo/utxo_common.rs
Outdated
| for utxo in self.required_inputs.clone() { | ||
| self.tx.inputs.push(UnsignedTransactionInput { | ||
| previous_output: utxo.outpoint, | ||
| prev_script: utxo.script, | ||
| sequence: SEQUENCE_FINAL, | ||
| amount: utxo.value, | ||
| }); | ||
| total += utxo.value; | ||
| } | ||
|
|
||
| for utxo in self.available_inputs.clone() { | ||
| if total >= amount { | ||
| break; | ||
| } | ||
| self.tx.inputs.push(UnsignedTransactionInput { | ||
| previous_output: utxo.outpoint, | ||
| prev_script: utxo.script, | ||
| sequence: SEQUENCE_FINAL, | ||
| amount: utxo.value, | ||
| }); | ||
| total += utxo.value; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be slightly cheaper:
| for utxo in self.required_inputs.clone() { | |
| self.tx.inputs.push(UnsignedTransactionInput { | |
| previous_output: utxo.outpoint, | |
| prev_script: utxo.script, | |
| sequence: SEQUENCE_FINAL, | |
| amount: utxo.value, | |
| }); | |
| total += utxo.value; | |
| } | |
| for utxo in self.available_inputs.clone() { | |
| if total >= amount { | |
| break; | |
| } | |
| self.tx.inputs.push(UnsignedTransactionInput { | |
| previous_output: utxo.outpoint, | |
| prev_script: utxo.script, | |
| sequence: SEQUENCE_FINAL, | |
| amount: utxo.value, | |
| }); | |
| total += utxo.value; | |
| } | |
| for utxo in &self.required_inputs { | |
| self.tx.inputs.push(UnsignedTransactionInput { | |
| previous_output: utxo.outpoint, | |
| prev_script: utxo.script.clone(), | |
| sequence: SEQUENCE_FINAL, | |
| amount: utxo.value, | |
| }); | |
| total += utxo.value; | |
| } | |
| for utxo in &self.available_inputs { | |
| if total >= amount { | |
| break; | |
| } | |
| self.tx.inputs.push(UnsignedTransactionInput { | |
| previous_output: utxo.outpoint, | |
| prev_script: utxo.script.clone(), | |
| sequence: SEQUENCE_FINAL, | |
| amount: utxo.value, | |
| }); | |
| total += utxo.value; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 50a38f8
| if total >= amount { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to add this check above the loop, so we don't start iterating it for no reason if it's already true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 50a38f8
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 1st iteration.
went over all the changes but skipped most of my comments on the tx builder stuff since i couldn't fully comprehend it and don't wanna cause confusion.
will need a couple more iters as this part of the code is a lil risky and confusing.
| /// fee amount per Kbyte received from coin RPC | ||
| Dynamic(u64), | ||
| /// Use specified amount per each 1 kb of transaction and also per each output less than amount. | ||
| /// Use specified fee amount per each 1 kb of transaction and also per each output less than the fee amount. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain more about this fee scheme. the per each output part isn't really clear (understood it as 1 fee_rate per 1 output, which i doubt is a correct understanding).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually I have the same question:
this fee calc is not fully clear to me. The comment refers to DOGE, I took a look at its daemon code but did not find where this logic is.
mm2src/coins/utxo.rs
Outdated
| impl ActualTxFee { | ||
| fn get_tx_fee(&self, tx_size: u64) -> u64 { | ||
| match self { | ||
| ActualTxFee::Dynamic(fee_per_kb) => (fee_per_kb * tx_size) / KILO_BYTE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't this wrongfully floor to zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could.
If it goes too low we would use min relay fee (if this is set though)
mm2src/coins/utxo/utxo_common.rs
Outdated
| if self.sum_inputs >= self.sum_outputs + self.total_tx_fee() { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be exactly equal (or equal + unused change)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe, not.
At this step tx_fee has been updated by the tx_size * fee_rate formula (not like difference of inputs - outputs). So if we have added sufficient inputs, we are finished
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the change will have been adjusted to inputs - outputs - fees, which brings the equation above to equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not necessarily so.
Within an iteration, we are getting next input set and change for the total outputs + previous-txfee.
But due to the input selection algo, we may get this input set with the length less than it was at the previous step (rare case, ofc). So the updated txfee may be less than the previous-txfee (but we want to quit now as we have added enough inputs to cover the txfee. As I can see, kmd works like that: it quits the loop when nFeeRet >= nFeeNeeded)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the updated txfee may be less than the previous-txfee (but we want to quit now as we have added enough inputs to cover the txfee. As I can see, kmd works like that: it quits the loop when nFeeRet >= nFeeNeeded)
I am not following here. Do you mean that the new tx fee is less than the previous iteration but we willingly and intentionally don't update/reduce it? why?
or we can make this argument simpler: if sum_inputs > sum_outputs + fees + unused_change where do these wasted sats (extra fees?) go? and what is the higher bound on the amount of these wasted sats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sets from previous loop iteration and we hope that the newly created inputs and outputs would be sufficient to cover the updated txfee
but this can:
- underpay fees -> we do another iteration.
- pay exact fees -> perfect, we should also have the equality holding in this case.
- overpay fees -> why overcharge? we should should subtract the overpaid fees and reward them to our change output (or the output paying the fees in general). (ref. https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/src/wallet/spend.cpp#L1268-L1276)
I am OK with this as long as we can have a check bounding the paid_fee - needed_fee to some maximum. Even though I think it's better to have a deterministic algo with a stricter/more-expected output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmk please if this statement is correct:
the algo here in it's core should terminate when the fees (after update at the end of the loop) at iteration x is the same as fees at iteration x+1. that is, the inputs will stay the same, the outputs will stay the same, whether there is a change or not will stay the same, but the fees changed a little bit to account for the true transaction size (that's the fee update at iteration x. iteration x+1 didn't need a fee update). in such case, we should have calculated the fees correctly already without overpaying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should should subtract the overpaid fees and reward them to our change output (or the output paying the fees in general). (ref.
bitcoin has additional fee optimisations indeed. I guess, this correction of the change in BTC (you refer to) works when the updated txfee becomes lower (due to the smaller reselected input set).
In our code we we need more inputs we add them in sequence (deterministically) so I think we won't have this case.
I suggest adding this as a TODO for now as well as implementation of an input selection algo like 'Branch and Bound'.
lmk please if this statement is correct: ... at iteration x is the same as fees at iteration x+1.
Almost. We break if next fee equals or less than the previous fee. It could be less if the change turns out to be dust (so the tx size becomes lower) so some overpay may appear but I guess this is okay.
we can have a check bounding the paid_fee - needed_fee to some maximum
We may add check for 'absurd fee' but this is not to prevent overpay
I think it's better to have a deterministic algo with a stricter/more-expected output.
Is this algo not deterministic enough? I believe most cli wallets use it and it fixes the existing bug.
It may produce slight overpay but I believe this is acceptable (and unavoidable)
I added a proptest https://github.com/dimxy/komodo-defi-framework/blob/69789daed7f204abdbea79bbb906853d54110f3f/mm2src/coins/utxo/utxo_tests.rs#L1228 with randomised inputs outputs. It catches when overpay occurs due to the change not added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin has additional fee optimisations indeed. I guess, this correction of the change in BTC (you refer to) works when the updated txfee becomes lower (due to the smaller reselected input set).
ah maybe. im not super sure tho since the thing isn't in a loop, so maybe not.
It could be less if the change turns out to be dust (so the tx size becomes lower) so some overpay may appear but I guess this is okay.
yup. that much overpay is perfectly justified for the dust case. and this is why i am saying there is a bound on how much we spend and such a bound is dust (actually a tiny bit greater due to how the change output mechanism bumps the fee).
Is this algo not deterministic enough? I believe most cli wallets use it and it fixes the existing bug.
the nondeterministic-ness here comes from the fact that we rely on the past state of the loop (tx fees computation) to solve-for/construct the tx in the current loop. it's not like a simple "these are the problem inputs and they give u this output/tx", but rather u need to reason about what was the last loop the tx solver gone through to know what the output tx would be.
tbh though, that's a play over a very small margin, it's not much of a big deal now since we don't have some sophisticated input selection algo. so not like one loop we will have a lot of small utxo inputs and next we have one large single input utxo (this case overpays in fees a lot).
i think i now have much confidence in this tx constructor algo :). will try to calculate the bound on the max overpay in fees though and discuss if we could add a check for it as that would be safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overpay may consist of dust and the fee for the removed change output.
(I improved the utxo builder test 9126c49: made inputs/outputs randomised and catch and print the overpay)
borngraced
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work! Some minor notes from my side
refactor (borngraced): add_change simplified, remove tx_fee mut, use checked_sub
onur-ozkan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the previous fixes.
It LGTM.
|
Does this PR require and docs updates? I've created a docs PR at GLEECBTC/komodo-docs-mdx#416 for the related KF PR #2083 please advise in the docs PR if anything else is needed. |
|
@dimxy pr started to have conflicts |
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed the latest commits. looks good. i like the new interest calculations. that made it much easier to understand what's going on.
single nit/optimization for now.
will do one last pass after discussions in the open threads are done.
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very neat test. thanks.
ran it many times and never failed :)
| const CHANGE_OUTPUT_SIZE: u64 = 1 + 25 + 8; | ||
| let max_overpay = dust + fee_rate * CHANGE_OUTPUT_SIZE / 1000; // could be slight overpay due to dust change removed from tx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
could we include this bound in the tx builder code as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this bound to the builder?
What if someone changes the input selection algo and the bound would become different?
And what to do if we hit it anyway, throw an error? (so user won't be able to send a tx).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if someone changes the input selection algo and the bound would become different?
this is a case where such a bound deems useful. when the input selection algo changes it should still respect such bound for whatever set of input selection.
And what to do if we hit it anyway, throw an error? (so user won't be able to send a tx).
we should never hit it (unless there is a bug; then it's justified to return an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a case where such a bound deems useful.
...
we should never hit it (unless there is a bug; then it's justified to return an error).
I would say for that tests should be used (and actually we have them)
…_verbose_transactions_from_cache_or_rpc
shamardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* dev: (26 commits) chore(deps): remove base58 and replace it completely with bs58 (GLEECBTC#2427) feat(tron): initial groundwork for full TRON integration (GLEECBTC#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (GLEECBTC#2316) deps(timed-map): bump to 1.3.1 (GLEECBTC#2413) improvement(tendermint): safer IBC channel handler (GLEECBTC#2298) chore(release): complete v2.4.0-beta changelogs (GLEECBTC#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (GLEECBTC#2431) improvement(watchers): re-write use-watchers handling (GLEECBTC#2430) fix(evm): make withdraw_nft work in HD mode (GLEECBTC#2424) feat(taproot): support parsing taproot output address types chore(RPC): use consistent param name for QTUM delegation (GLEECBTC#2419) fix(makerbot): add LiveCoinWatch price provider (GLEECBTC#2416) chore(release): add changelog entries for v2.4.0-beta (GLEECBTC#2415) fix(wallets): prevent path traversal in `wallet_file_path` and update file extension (GLEECBTC#2400) fix(nft): make `update_nft` work with hd wallets using the enabled address (GLEECBTC#2386) fix(wasm): unify error handling for mm2_main (GLEECBTC#2389) fix(tx-history): token information and query (GLEECBTC#2404) test(electrums): fix failing test_one_unavailable_electrum_proto_version (GLEECBTC#2412) improvement(network): remove static IPs from seed lists (GLEECBTC#2407) improvement(best-orders): return an rpc error when we can't find best orders (GLEECBTC#2318) ...
* dev: (29 commits) fix(p2pk): validate expected pubkey correctly for p2pk inputs (#2408) chore(docs): update old urls referencing atomicdex or old docs pages (#2428) improvement(p2p): remove hardcoded seeds (#2439) fix(evm-api): find enabled erc20 token using platform ticker (#2445) chore(docs): add DeepWiki badge to README (#2463) chore(core): organize deps using workspace.dependencies (#2449) feat(db-arch): more dbdir to address_dir replacements (#2398) chore(build-artifacts): remove duplicated mm2 build artifacts (#2448) feat(pubkey-banning): expirable bans (#2455) fix(eth-balance-events): serialize eth address using AddrToString (#2440) chore(deps): remove base58 and replace it completely with bs58 (#2427) feat(tron): initial groundwork for full TRON integration (#2425) fix(UTXO): improve tx fee calculation and min relay fee handling (#2316) deps(timed-map): bump to 1.3.1 (#2413) improvement(tendermint): safer IBC channel handler (#2298) chore(release): complete v2.4.0-beta changelogs (#2436) fix(event-streaming): initial addresses registration in utxo balance streaming (#2431) improvement(watchers): re-write use-watchers handling (#2430) fix(evm): make withdraw_nft work in HD mode (#2424) feat(taproot): support parsing taproot output address types ...
Fixes for utxo tx fee calculation in UtxoTxBuilder::build() fn:
UtxoTxBuilder::build() code was refactored to implement fixed txfee calc: made it similar to daemon code (with a loop), also broke it into functions.
Existing tests updated for the fixed txfee.
Fixes issue: #2313 (a test added to validate txfee from the issue).
Should also fix #1567 issue (in fact it did not fix it - with removal of custom high dust setting we may get errors like 'spend fee nnn is greater than transaction output' ).
TODO: fix input size for segwit
@cipig