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

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

Details

Reviewers

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

Branch

backport-pr16507

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 13663
Build 27352: Build Diff	build-without-wallet · build-diff · lint-circular-dependencies · build-debug · build-clang · build-clang-tidy
Build 27351: arc lint + arc unit

Event Timeline

PiRK created this revision.Oct 25 2020, 12:26

Owners added a reviewer: Restricted Owners Package.Oct 25 2020, 12:26

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 25 2020, 12:26

PiRK requested review of this revision.Oct 25 2020, 12:26

teamcity edited the summary of this revision. (Show Details)Oct 25 2020, 12:26

PiRK edited the summary of this revision. (Show Details)Oct 25 2020, 12:31

Harbormaster completed remote builds in B13663: Diff 25073.Oct 25 2020, 12:35

PiRK edited the summary of this revision. (Show Details)Oct 25 2020, 12:55

PiRK added a child revision: D8111: modify p2p_feefilter test to catch rounding error.Oct 25 2020, 12:57

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

deadalnix mentioned this in D8111: modify p2p_feefilter test to catch rounding error.Oct 25 2020, 15:21

squash D8110 and D8111 (functional test)

PiRK edited the summary of this revision. (Show Details)Oct 25 2020, 16:00

Harbormaster completed remote builds in B13669: Diff 25083.Oct 25 2020, 16:14

What about https://github.com/bitcoin/bitcoin/pull/16507/commits/6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 ?

deadalnix requested changes to this revision.Oct 25 2020, 17:01

This revision now requires changes to proceed.Oct 25 2020, 17:01

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) {}

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).

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) {}