test: test RBF rule 3 #25461

Somewhat surprisingly, RBF rule 3 (absolute fees of replacement transaction must meet or exceed original) is untested.

In other words, applying the following patch:

diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
index e25f5c7c5b..e4f2d9d728 100644
--- a/src/policy/rbf.cpp
+++ b/src/policy/rbf.cpp
@@ -162,14 +162,6 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
                                       CFeeRate relay_fee,
                                       const uint256& txid)
 {
-    // BIP125 Rule [#3](/bitcoin-bitcoin/3/): The replacement fees must be greater than or equal to fees of the
-    // transactions it replaces, otherwise the bandwidth used by those conflicting transactions
-    // would not be paid for.
-    if (replacement_fees < original_fees) {
-        return strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
-                         txid.ToString(), FormatMoney(replacement_fees), FormatMoney(original_fees));
-    }
-
     // BIP125 Rule [#4](/bitcoin-bitcoin/4/): The new transaction must pay for its own bandwidth. Otherwise, we have a DoS
     // vector where attackers can cause a transaction to be replaced (and relayed) repeatedly by
     // increasing the fee by tiny amounts.

then recompiling and running the full test suite causes no failures.

This is sort of a moot point, because the check following the removed check (rule 4 following rule 3) implicitly requires the absolute fee of the replacement to exceed the original. So effectively, you can't violate rule 3 even if its code is removed.

But still, it seems like we should probably explicitly test for the enforcement of rule 3. This test creates a failure when the above patch is applied.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25522 (test: Remove -acceptnonstdtxn=1 from feature_rbf.py by MarcoFalke)
• #25353 (Add a -mempoolfullrbf node setting by ariard)
• #22867 (test: Extend test coverage of BIP125 and document confusing/inconsistent behavior by mjdietzx)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK

Rule 4 implies Rule 3, so isn't this just testing the error string?

Rule 4 implies Rule 3, so isn't this just testing the error string?

I think we should either remove the "dead" code and fold rule 3 into rule 4 or add this test. Having untested code in RBF policy code does not seem like a good state.

I think we should either remove the "dead" code and fold rule 3 into rule 4 or add this test.

I think it depends on what we value more:

1. To maintain the completeness of the code by explicitly writing conditions for all the rules. Or,
2. To keep the code clean, minimal, and free of redundancies.

If there is no predetermined consensus on which option is better, then, in my opinion, I prefer the second option. And so, I favor removing the "dead" code.

I think we should either remove the "dead" code and fold rule 3 into rule 4 or add this test. Having untested code in RBF policy code does not seem like a good state.

I wouldn't call this code "dead" since it is executed, I interpret it as testing debug messages rather than adding coverage of policy code. ~I am in favor of folding Rule 3 and 4 together since one is encapsulated in the other.~ (edit: apologies for scope creep)

I have never worked on a project before where adding a simple test that provides novel coverage has been conceptually nit-picked to this degree.

An "error string" is part of the public interface of this software. We don't know which users are relying on this particular error string. Its continued existence should be verified with a test if practical, which luckily in this case it is.

If you want to change the public interface, in principle the right way to do that is with some kind of deprecation schedule. Until then we should not be in a state where we can remove a central chunk of policy code and not have any tests fail.

No opinion on what to do here. I think anything is fine if reviewers like it.

The error string seems to be hit 4 times already in the tests (probably in non-rbf related ones): https://marcofalke.github.io/btc_cov/total.coverage/src/policy/rbf.cpp.gcov.html#169

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

I'm in favor of testing what we have, and removing what we don't need. The former is generally easier than the latter.

ACK https://github.com/bitcoin/bitcoin/pull/25461/commits/6de5e666855be3bd7aba0c9bbdb10f6ddb3a1df7

Are you still working on this? Is this still relevant after test/rbf_tests.cpp?

I still think that testing the interface is important. Will rebase if it's not too much trouble otherwise will close.