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

feefilter: Compute the absolute fee rather than stored rate #16507

Merged
merged 3 commits into from Oct 4, 2019

Conversation

@instagibbs
Copy link
Member

commented Jul 31, 2019

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

Fixes #16499

Replacement PR for #16500

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

This can be tested by applying instagibbs@4538d6b which without this fix causes rpc_fundrawtransaction.py to stall out when feefilter starts rejecting wallet/mempool transactions.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

src/txmempool.h Show resolved Hide resolved
@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

utACK.
Seems like a reasonable simple fix for the existing problem.

Replacement PR for https://github.com/bitcoin/bitcoin/pull/165001

The link doesn't work.

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Oops, fixed link

@instagibbs instagibbs force-pushed the instagibbs:feefilter_match_mempool branch 2 times, most recently from 7a3af9e to 051fbd8 Aug 20, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@luke-jr fixed field type and made CFeeRate constructor explicit to avoid merge/backport errors.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

I can haz test? Looks like you managed to write a test for the issue? Should be included here, no? I'm confused if the changes to TxMempoolInfo are required - we're still converting to CFeeRate before the convserion.

if (filterrate) {
if (txinfo.feeRate.GetFeePerK() < filterrate)
if (filterrate > CFeeRate(0)) {
if (txinfo.fee < filterrate.GetFee(txinfo.vsize))

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 20, 2019

Author Member

@TheBlueMatt we're using it directly here, not converting to feerate.

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

And yes I can write a test.

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

@TheBlueMatt Pushed a test change that causes fee filter test to stall out when fix is removed.

@instagibbs instagibbs force-pushed the instagibbs:feefilter_match_mempool branch from 496221c to 3adf217 Sep 9, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

rebased

@DrahtBot DrahtBot removed the Needs rebase label Sep 9, 2019
@instagibbs instagibbs force-pushed the instagibbs:feefilter_match_mempool branch from 3adf217 to af5f7d2 Sep 9, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

kdiff ate some tab space or something, pushed fix

Copy link
Contributor

left a comment

ACK af5f7d2. Appears to be a good, low-complexity fix. Code review, build/tests pass (thanks for adding tests!), new tests fail without the changes and on current master cc1d7fd. Sorry for taking so long to review this. Feel free to ignore the nits below.

@@ -3881,10 +3882,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
for (std::set<uint256>::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) {
vInvTx.push_back(it);
}
CAmount filterrate = 0;
CFeeRate filterrate;

This comment has been minimized.

Copy link
@jonatack

jonatack Sep 18, 2019

Contributor

FWIW identical code at L3847 and L3885 (also the case before the changes touching this).

                    CFeeRate filterrate;
                    {
                        LOCK(pto->m_tx_relay->cs_feeFilter);
                        filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter);
                    }

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

In addition to removing code duplication, a helper would simplify this code, reduce dependencies on CNode internals, and prevent ever having an uninitialized fee rate. Consider (or similar):

const CFeeRate filter_rate = pto->GetMinRelayFee();

IMHO, continually making small improvements like this would go a long way in making the codebase more approachable. Speaking from experience, as it stands the codebase is quite daunting to new contributors.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 3, 2019

Author Member

The p2p code is the most daunting part. Refactoring has its own dangers. Complete ACK on working to make it easier to understand!

@@ -3911,7 +3912,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
if (!txinfo.tx) {
continue;
}
if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) {
if (filterrate > CFeeRate(0) &&

This comment has been minimized.

Copy link
@jonatack

jonatack Sep 18, 2019

Contributor
  1. Some code comments would be great, particularly before the conditional here and line 3859 just above.

  2. With these changes the 2 conditionals appear to be equivalent now. Making them the same or extracting them to a well-named bool function might be a nice touch.

                        if (filterrate > CFeeRate(0)) {
                            if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
                                continue;
                            }
                        }
                        if (filterrate > CFeeRate(0) &&
                            txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
                            continue;
                        }

This comment has been minimized.

Copy link
@instagibbs

instagibbs Sep 18, 2019

Author Member

Can you suggest some text? I'm not sure of the confusion.

Also I'm not sure what you mean by (2)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Sep 21, 2019

Author Member

ah I see now, I'm just mimicking current style. If I need to rebase or something I'll make them match.

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

Agreed regarding extracting into a well-named helper and/or commenting there. To me:

  1. It seems this code is to prevent sending transactions to a peers that won't relay it. Is that correct?
  2. It's not apparent why the check isn't made when the fee rate is zero. Is it as an optimization or some other reason?

Arguably someone scanning this code shouldn't need to parse that particular logic. Or if they do then they could dive into the helper.

That said, it's not clear to me why filterrate > CFeeRate(0) is needed. Won't GetFee return a zero amount when the rate is zero? Can this be eliminated or is it an optimization?

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

utACK 6a9ee89.

I would slightly prefer 3859 and 3915 to share the code-style (they do the same thing but in different fashion), but feel free to ignore.

Copy link
Member

left a comment

utACK with an ignorable nit

@@ -3856,9 +3856,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
const uint256& hash = txinfo.tx->GetHash();
CInv inv(MSG_TX, hash);
pto->m_tx_relay->setInventoryTxToSend.erase(hash);
if (filterrate) {
if (txinfo.feeRate.GetFeePerK() < filterrate)
if (filterrate > CFeeRate(0)) {

This comment has been minimized.

Copy link
@kallewoof

kallewoof Sep 27, 2019

Member

μ-Nit: I would slightly prefer a IsNull() { return nSatoshisPerK == 0; } in CFeeRate and using that instead of instantiating CFeeRate()s whenever you check if the filter rate is set.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Sep 30, 2019

Author Member

will do if forced to rebase or change commits for more substantial reasons

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 1, 2019

Member

maybe isZero instead of isNull? I always associate Null with pointers somehow, not with values, this could give confusion here.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 2, 2019

Member

Either works for me. We use IsNull elsewhere (e.g. uint256, FlatFilePos, BlockHeader, PSBTInput, CScriptWitness, etc etc.)

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

@kallewoof Those examples aren't really values in the sense of integral values that could support arithmetic operations. Rather they support invalid or null values via IsNull. For example, FlatFilePos::IsNull is not for the zero position but rather an invalid position object (i.e. nFile == -1). So IsZero seems more appropriate here since a zero fee rate represents a valid object.

I'm indifferent as to whether the helper is needed, though.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 3, 2019

Member

Same goes for uint256 I'd say, but I don't really have a strong opinion on the matter. (Consistency -> IsNull; intuitive -> IsZero.)

@achow101

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

ACK af5f7d2

Reviewed code and checked that the test fails without this change.

Copy link

left a comment

ACK af5f7d2

Also checked that test fails without the change.

My preference is to include some of the suggested refactorings as they would be beneficial in terms of improving code health and approachability.

@@ -3856,9 +3856,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
const uint256& hash = txinfo.tx->GetHash();
CInv inv(MSG_TX, hash);
pto->m_tx_relay->setInventoryTxToSend.erase(hash);
if (filterrate) {
if (txinfo.feeRate.GetFeePerK() < filterrate)
if (filterrate > CFeeRate(0)) {

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

@kallewoof Those examples aren't really values in the sense of integral values that could support arithmetic operations. Rather they support invalid or null values via IsNull. For example, FlatFilePos::IsNull is not for the zero position but rather an invalid position object (i.e. nFile == -1). So IsZero seems more appropriate here since a zero fee rate represents a valid object.

I'm indifferent as to whether the helper is needed, though.

@@ -3881,10 +3882,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
for (std::set<uint256>::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) {
vInvTx.push_back(it);
}
CAmount filterrate = 0;
CFeeRate filterrate;

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

In addition to removing code duplication, a helper would simplify this code, reduce dependencies on CNode internals, and prevent ever having an uninitialized fee rate. Consider (or similar):

const CFeeRate filter_rate = pto->GetMinRelayFee();

IMHO, continually making small improvements like this would go a long way in making the codebase more approachable. Speaking from experience, as it stands the codebase is quite daunting to new contributors.

@@ -3911,7 +3912,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
if (!txinfo.tx) {
continue;
}
if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) {
if (filterrate > CFeeRate(0) &&

This comment has been minimized.

Copy link
@jkczyz

jkczyz Oct 3, 2019

Agreed regarding extracting into a well-named helper and/or commenting there. To me:

  1. It seems this code is to prevent sending transactions to a peers that won't relay it. Is that correct?
  2. It's not apparent why the check isn't made when the fee rate is zero. Is it as an optimization or some other reason?

Arguably someone scanning this code shouldn't need to parse that particular logic. Or if they do then they could dive into the helper.

That said, it's not clear to me why filterrate > CFeeRate(0) is needed. Won't GetFee return a zero amount when the rate is zero? Can this be eliminated or is it an optimization?

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

@jkczyz your reading is correct. I was keeping the code as close as possible to the original logic, while fixing the bug. The previous logic asks if the fee is set(non-zero), so I mimicked that. p2p code is notoriously hard to get right and even harder to get reviewers for, so I just kept things as close as possible.

Follow-on cleanups are welcome of course.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I think this no longer compiles after #16727 got merged?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

If you rebase this, could you please do so on top of 8afa602, so that backports are simplified?

@MarcoFalke MarcoFalke closed this Oct 3, 2019
@MarcoFalke MarcoFalke reopened this Oct 3, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

I'll rebase it sure and fix some minimal nits.

@instagibbs instagibbs force-pushed the instagibbs:feefilter_match_mempool branch from af5f7d2 to eb7b781 Oct 3, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

rebased and removed the non-essential conditional that is spawing all the IsNull/IsZero debate.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

utACK af5f7d2. Code changes are simple enough and test fails if you revert the fix.

Copy link
Member

left a comment

ACK eb7b781.

In case you push again, commit "Disallow implicit conversion for CFeeRate constructor" could be the first commit.

@@ -25,7 +25,7 @@ class CFeeRate
/** Fee rate of 0 satoshis per kB */
CFeeRate() : nSatoshisPerK(0) { }
template<typename I>
CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {

This comment has been minimized.

Copy link
@promag

promag Oct 3, 2019

Member

nit, add space before :.

@kallewoof

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

rebased and removed the non-essential conditional that is spawing all the IsNull/IsZero debate.

Still see a bunch of >/== CFeeRate(0) in the changes.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

ACK eb7b781 code review only

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

@kallewoof I only removed the redundant calls in net_processing.cpp because it does reduce readability and fixing is a net gain imo.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

utACK eb7b781

@achow101

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

re ACK eb7b781

fanquake added a commit that referenced this pull request Oct 4, 2019
…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 #16499

  Replacement PR for #16500

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

Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
@fanquake fanquake merged commit eb7b781 into bitcoin:master Oct 4, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request 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
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

post-merge

ACK eb7b781

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK eb7b78165966f2c79da71b993c4c4d793e37297f
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgb3AwAh1Oi4PZcMIwCloxjHiSdLUKf/fEvb6wptxlDFYwLt9S8yI5uFKTVtYSg
K16HQxDHoHP/kAt8Ym3WFREQ+ib7BBiBrFG0xGKI2GukyThCrh2lcgGQGjCDUWlC
5V+n1K2xpar3UzD+ih7v515dQp6ve7TUXxybS3J1HcyIXVjIPgD1XGbEYi+CA/fp
rV52yAzVi0xqaHgKcjY4gqNW9F7hNGm6EFa8Xu3H8eaNFDbMA2fs+UY6kQdkMeaf
5e8YDUn58NM0yNR9X4rZ+baHWXqkF/r2y9SnW396kVvO5XiKWm2HIaYj7MzuF9Ur
HGOjeBPSO/zjsrOtpTwn+kV6pJ3rwkG7koMseSMB0zkwak1rNhDHU+Ryb5E5n7MG
8hBpIqtKJ+/3iEyjCqto21e79yN38Dub15bYjjRAb+doPbJJqDp9qxgWwudV8nxS
lOteMqZGpanPXfOp19bP6Nm7WnkPJ2358R0smPalrpNuJrolfziYVgFAIoq0zHTo
UUc9jGn0
=lpFy
-----END PGP SIGNATURE-----

Timestamp of file with hash 8169fd75811d5d99dfe89e4d9d3d55e4528f00d1db035d132d66806155ce78a3 -

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.