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

Fee(rate) mismatch between min relay and fee filter #16499

Closed
instagibbs opened this issue Jul 30, 2019 · 2 comments · Fixed by #16507

Comments

@instagibbs
Copy link
Member

commented Jul 30, 2019

I was messing around with min relay policies and found that under certain circumstances the mempool will accept things, then not relay to any peers.

Example made using wallet:
Transaction of size 6195, fee of 619 satoshis, minrelay of 100 satoshis per kvB. Mempool logic allows it:

        // No transactions are allowed below minRelayTxFee except from disconnected blocks
        if (!bypass_limits && nModifiedFees < ::minRelayTxFee.GetFee(nSize)) {
            return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", false, strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize)));
        }

and lets it into mempool

but in net_processing.cpp:

                    if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) {
                        continue;
                    }

this gets hit because the computed feerate is insufficient(printing I added):

0.00000099 BTC/kB

I'm not super at arithmetic so I'm opening this up in case someone has an easy fix.

let me also get a reproducible test failure with this...

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Reproduced here instagibbs@4538d6b if you run rpc_fundrawtransaction.py

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Ok it's pretty straight-forward:

Whenever you have a feerate in satoshis per kvB that's not divisible by 1000 exactly, GetFee may undershoot its intended results when it's checked for fee filter. See:

instagibbs@c480e4d

Applying this fix means that previously-accepted things into mempool are no longer accepted, but these transactions won't propagate anyways, so I think it's fairly conservative.

@fanquake fanquake closed this in 94d6a18 Oct 4, 2019
sidhujag added a commit to syscoin/syscoin that referenced this issue Oct 4, 2019
…stored rate

eb7b781 modify p2p_feefilter test to catch rounding error (Gregory Sanders)
6a51f79 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
8e59af5 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)

Pull request description:

  This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes.

  Fixes bitcoin#16499

  Replacement PR for bitcoin#16500

ACKs for top commit:
  ajtowns:
    ACK eb7b781 code review only
  naumenkogs:
    utACK eb7b781
  achow101:
    re ACK eb7b781
  promag:
    ACK eb7b781.

Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.