Page MenuHomePhabricator

feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic
ClosedPublic

Authored by PiRK on Oct 25 2020, 12:26.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC22362253b49e: feefilter: Compute the absolute fee rather than stored rate to match mempool…
Summary

Under certain circumstances the mempool will accept things, then not relay to any peers because the computed feerate is insufficient. Use the rounding-down behavior in GetFee to match both mempool acceptance and wallet logic, with minimal changes.

Description of the issue:

Currently fee filter and mempool/wallet will disagree on the sufficiency of minrelay fee transactions when the min relay rate is not % 1000 = 0. The wallet and mempool will allow a transaction to be created and entered into mempool, but then peers that have the same rate as a fee filter will not receive the transaction because of the undershooting that occurs in GetFee.

This is a backport of Core PR16507

It includes following commits:
https://github.com/bitcoin/bitcoin/pull/16507/commits/8e59af55aaf1b196575084bce2448af02d97d745
https://github.com/bitcoin/bitcoin/pull/16507/commits/eb7b78165966f2c79da71b993c4c4d793e37297f

The ignored commit is not relevant because the CFeeRate constructor is already explicit, in Bitcoin ABC. See D3313 for why this is the case. Thus the backport is completed.

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 25 2020, 12:26
PiRK requested review of this revision.Oct 25 2020, 12:26
deadalnix requested changes to this revision.Oct 25 2020, 15:21
deadalnix added a subscriber: deadalnix.

Please merge the test and the code change together instead of in two diffs.

This revision now requires changes to proceed.Oct 25 2020, 15:21

squash D8110 and D8111 (functional test)

deadalnix requested changes to this revision.Oct 25 2020, 17:01
This revision now requires changes to proceed.Oct 25 2020, 17:01
PiRK requested review of this revision.Oct 25 2020, 19:23

This is the commit we did not backport that explains the difference : https://github.com/bitcoin/bitcoin/pull/11303/commits
It was intentionally skipped in D3313 (backport of 11303).

deadalnix requested changes to this revision.Oct 25 2020, 22:33
In D8110#189913, @PiRK wrote:

CFeeRate is already explicit, in Bitcoin ABC:
https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/feerate.h$31
It is initialized with CFeeRate()
https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/wallet/rpcwallet.cpp$2737
which is initialized with Amount() : amount(0) {}

The commit description needs to explain this and describe the backport as being complete, then.

This revision now requires changes to proceed.Oct 25 2020, 22:33
PiRK requested review of this revision.Oct 26 2020, 07:36
PiRK edited the summary of this revision. (Show Details)
PiRK edited the summary of this revision. (Show Details)

updated commit message

This revision is now accepted and ready to land.Oct 26 2020, 13:09