RBF move 2/3: extract RBF logic into policy/rbf

glozow commented at 12:49 pm on August 10, 2021: member

This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good:

Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn’t become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation.
We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea.
Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. PaysForRBF) and an edit to the relevant mempool entries.

DrahtBot added the label TX fees and policy on Aug 10, 2021

DrahtBot added the label Validation on Aug 10, 2021

glozow force-pushed on Aug 10, 2021

glozow added the label Refactoring on Aug 10, 2021

0xB10C commented at 2:48 pm on August 10, 2021: member

Concept ACK

TIL Witness Replacement

glozow commented at 4:33 pm on August 10, 2021: member

A new circular dependency policy/rbf -> txmempool -> validation -> policy/rbf is introduced here, so I’ve added it to EXPECTED_CIRCULAR_DEPENDENCIES to suppress the issue. I understand this is not ideal. The main problem, I think, is the txmempool -> validation part (which already exists), so I’ve taken a crack at it in #22677.

DrahtBot commented at 6:53 pm on August 10, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22539 (Re-include RBF replacement txs in fee estimation by darosior)

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.

fanquake commented at 2:45 am on August 11, 2021: member

Concept ACK

fanquake requested review from darosior on Aug 11, 2021

in src/policy/rbf.h:12 in 94655e96a4 outdated

 6@@ -7,6 +7,10 @@
 7 
 8 #include <txmempool.h>
 9 
10+/** Maximum number of transactions that can be replaced by BIP125 RBF (Rule #5). This includes all
11+ * mempool conflicts and their descendants. */
12+static constexpr uint32_t MAX_BIP125_REPLACEMENT_CANDIDATES{100};

mjdietzx commented at 9:45 am on August 11, 2021:

Any reason this shouldn’t go in policy/policy.h? It would get rid of the circular dependency as well

As precedent, we do have at least one BIP 125 specific constant there:

0/** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or BIP 125 replacement **/
1static const unsigned int DEFAULT_INCREMENTAL_RELAY_FEE = 1000;

glozow commented at 2:02 pm on August 11, 2021:

It wouldn’t get rid of the circular dependency unfortunately, because it’s caused by validation.cpp including policy/rbf.h

in src/validation.cpp:840 in 76354e503c outdated

834@@ -835,33 +835,36 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
835                             newFeeRate.ToString(),
836                             oldFeeRate.ToString()));
837             }
838+        }
839+
840+        for (const auto& mi : setIterConflicting) {

mjdietzx commented at 9:52 am on August 11, 2021:

re 76354e503c039ae9c7e0d0f93191b211d699478c, nice 👍 I had the same thought when reading this code that it’d be more efficient to do something like this

in src/policy/rbf.cpp:61 in b003c8da75 outdated

60+        {
61+            setConflictsParents.insert(txin.prevout.hash);
62+        }
63+
64+        nConflictingCount += mi->GetCountWithDescendants();
65+        // This potentially overestimates the number of actual descendants

mjdietzx commented at 10:41 am on August 11, 2021:

I guess the reason it may overestimate here is due to the same descendant being counted multiple times when multiple conflicting parents share that same descendant?

glozow commented at 2:02 pm on August 11, 2021:

Yes, that’s exactly right.

in src/policy/rbf.cpp:50 in aa30c6f7d6 outdated

46@@ -47,6 +47,29 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
47     return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
48 }
49 
50+bool IsRBFOptOut(const CTransaction& txConflicting)

mjdietzx commented at 10:43 am on August 11, 2021:

Is this necessary? Can’t we just use !SignalsOptInRBF from util/rbf.h?

Note: I realize this is a move only PR, but I’m just wondering, maybe it’s a minor refactor later

mjdietzx commented at 10:51 am on August 11, 2021:

nit: I’m sure if util/rbf.h existence is justified in the first place, but if we go with the current convention, IsRBFOptOut would seem to belong there

glozow commented at 1:59 pm on August 11, 2021:

Hm, I think everything in util/rbf should be moved to policy/rbf actually :thinking: There’s no reason to have two places for RBF (I think) and organization-wise, all RBF logic and code is policy.

darosior commented at 2:25 pm on August 11, 2021:

Not sure, it would make all users of policy/rbf depend on txmempool. Eg not sure the standalone binaries wallet-util and friends depend on it although they use util/rbf.

glozow commented at 3:00 pm on August 11, 2021:

Ahh good point @darosior. I was wrong. I think it makes sense for policy/* to depend on mempool, but I see how util/rbf is needed now.

in src/policy/rbf.cpp:81 in 520d3efda0 outdated

76+                      const CTxMemPool::setEntries setIterConflicting,
77+                      TxValidationState& state, CTxMemPool::setEntries& allConflicting)
78+{
79+    AssertLockHeld(m_pool.cs);
80+    const uint256 hash = tx.GetHash();
81+    std::set<uint256> setConflictsParents;

mjdietzx commented at 10:57 am on August 11, 2021:

We are populating setConflictsParents in this method, but we never use it

glozow commented at 3:08 pm on August 11, 2021:

Oopsie good catch! did a few different attempts at moving BIP125, this is leftover from a previous one :sweat_smile:

in src/policy/rbf.cpp:87 in 520d3efda0 outdated

114+    for (const auto& mi : setIterConflicting) {
115+        for (const CTxIn &txin : mi->GetTx().vin)
116+        {
117+            setConflictsParents.insert(txin.prevout.hash);
118+        }
119+    }

mjdietzx commented at 10:58 am on August 11, 2021:

Now we are populating setConflictsParents again, the same way we did in GetEntriesForRBF, but we use it here

in src/policy/rbf.cpp:195 in 520d3efda0 outdated

190+        }
191+    }
192+    return true;
193+}
194+
195+bool PaysForRBF(CAmount nConflictingFees, size_t nConflictingSize,

mjdietzx commented at 11:03 am on August 11, 2021:

Note: nConflictingSize is an unused variable

glozow commented at 3:09 pm on August 11, 2021:

Removed - thanks!

mjdietzx commented at 11:05 am on August 11, 2021: contributor

Concept ACK. Very well done, I think this is a nice improvement

glozow force-pushed on Aug 11, 2021

mjdietzx commented at 1:24 am on August 12, 2021: contributor

crACK 6a0a8ef791400d78fb4192a81a392131be78270b

theStack commented at 12:11 pm on August 12, 2021: member

Concept ACK

mjdietzx commented at 2:49 pm on August 13, 2021: contributor

Tested ACK 6a0a8ef791400d78fb4192a81a392131be78270b

in src/policy/rbf.cpp:51 in 474258f429 outdated

46@@ -43,3 +47,31 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
47     return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
48 }
49 
50+bool GetEntriesForRBF(const CTransaction& tx, CTxMemPool& m_pool,
51+                      const CTxMemPool::setEntries setIterConflicting,

theStack commented at 5:12 pm on August 13, 2021:

I guess for performance reasons, we should pass setIterConflicting by reference, rather than by value?

0                      const CTxMemPool::setEntries& setIterConflicting,

glozow commented at 9:48 am on August 16, 2021:

Good point, done!

in src/policy/rbf.cpp:10 in 474258f429 outdated

 1@@ -2,9 +2,13 @@
 2 // Distributed under the MIT software license, see the accompanying
 3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 4 
 5+#include <consensus/validation.h>
 6+#include <logging.h>
 7 #include <policy/rbf.h>
 8 #include <util/rbf.h>
 9 
10+#include <string>

theStack commented at 5:18 pm on August 13, 2021:

This header doesn’t seem to be directly needed in this compilation unit (strings are used as return values/parameters for uint256.ToString() and strprintf, but it’s the job of the headers offering those functions to already also include <string>).

glozow commented at 9:48 am on August 16, 2021:

Done

theStack commented at 5:30 pm on August 13, 2021: member

Reviewed up to 474258f4296878a3740c8e19c9358ba4134cd48d (5/9 commits) so far, LGTM, nice cleanups. To be extra-cautious, I also compiled and ran unit tests + the functional test feature_rbf.py on every commit.

Left two minor nits. Planning to review the remaining 4 commits tomorrow.

in src/policy/rbf.cpp:79 in 7558a4f0c1 outdated

74@@ -75,3 +75,40 @@ bool GetEntriesForRBF(const CTransaction& tx, CTxMemPool& m_pool,
75     return true;
76 }
77 
78+bool HasNoNewUnconfirmed(const CTransaction& tx, CTxMemPool& m_pool,
79+                         const CTxMemPool::setEntries setIterConflicting, TxValidationState& state)

theStack commented at 6:04 pm on August 14, 2021:

0bool HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& m_pool,
1                         const CTxMemPool::setEntries& setIterConflicting, TxValidationState& state)

glozow commented at 9:48 am on August 16, 2021:

Done

in src/policy/rbf.cpp:116 in 85feb62fbf outdated

111@@ -112,3 +112,20 @@ bool HasNoNewUnconfirmed(const CTransaction& tx, CTxMemPool& m_pool,
112     }
113     return true;
114 }
115+
116+bool SpendsAndConflictsDisjoint(CTxMemPool::setEntries& setAncestors, std::set<uint256> setConflicts,

theStack commented at 6:15 pm on August 14, 2021:

0bool SpendsAndConflictsDisjoint(const CTxMemPool::setEntries& setAncestors, const std::set<uint256>& setConflicts,

glozow commented at 9:49 am on August 16, 2021:

Good point, done

in src/policy/rbf.h:58 in 85feb62fbf outdated

50@@ -51,4 +51,12 @@ bool GetEntriesForRBF(const CTransaction& tx, CTxMemPool& m_pool,
51 bool HasNoNewUnconfirmed(const CTransaction& tx, CTxMemPool& m_pool,
52                          const CTxMemPool::setEntries setIterConflicting,
53                          TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(m_pool.cs);
54+
55+/** Check the intersection between original mempool transactions (candidates for being replaced) and
56+ * the ancestors of replacement transactions.
57+ * @param[in]   hash    Transaction ID, included in the error message if violation occurs.
58+ * returns false if the intersection is empty, true if otherwise.

theStack commented at 6:17 pm on August 14, 2021:

0 * returns true if the intersection is empty, false if otherwise.

glozow commented at 9:49 am on August 16, 2021:

Fixed

in src/validation.cpp:882 in a30b0270d0 outdated

803@@ -828,7 +804,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
804             return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
805                     strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
806                         hash.ToString(),
807-                        FormatMoney(nDeltaFees),

theStack commented at 6:28 pm on August 14, 2021:

This line removal must be unintentional (three placeholders, but only two arguments)? Quite a pity that the compiler doesn’t warn us in this case. At least the tinyformat library seems to be nice enough to notice us at runtime :)

 0$ ./test/functional/feature_rbf.py
 1.....
 2.....
 32021-08-14T18:32:49.823000Z TestFramework (INFO): Running test replacement relay fee...
 42021-08-14T18:32:49.834000Z TestFramework (ERROR): Assertion failed
 5Traceback (most recent call last):                                                                                                                                                             File "/home/honey/bitcoin_prrev/test/functional/test_framework/util.py", line 132, in try_rpc
 6    fun(*args, **kwds)
 7  File "/home/honey/bitcoin_prrev/test/functional/test_framework/coverage.py", line 49, in __call__
 8    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 9  File "/home/honey/bitcoin_prrev/test/functional/test_framework/authproxy.py", line 146, in __call__
10    raise JSONRPCException(response['error'], status)
11test_framework.authproxy.JSONRPCException: tinyformat: Too many conversion specifiers in format string (-1)

glozow commented at 8:42 am on August 16, 2021:

woah good catch O.o

glozow commented at 9:56 am on August 16, 2021:

Fixed

theStack commented at 6:43 pm on August 14, 2021: member

Left some more suggestions considering const-correctness and passing by reference rather than by value. On the second-to-last commit a30b0270d0129f16573827d45250aa42912803d3 the test feature_rbf.py currently fails, see comment below. In the last commit this is fixed, as the deleted strprintf() argument line is re-introduced in the helper function.

glozow force-pushed on Aug 16, 2021

glozow commented at 9:58 am on August 16, 2021: member

To be extra-cautious, I also compiled and ran unit tests + the functional test feature_rbf.py on every commit. Left some more suggestions considering const-correctness and passing by reference rather than by value. On the second-to-last commit a30b027 the test feature_rbf.py currently fails, see comment below. In the last commit this is fixed, as the deleted strprintf() argument line is re-introduced in the helper function.

Thanks @theStack and good catch! Fixed. Good idea for reviewers to run git rebase --exec "make check; test/functional/feature_rbf.py" -i HEAD~11 in general

Also added a small scripted diff style cleanup, should be simple to review

in test/lint/lint-circular-dependencies.sh:18 in 192193c7c4 outdated

14@@ -15,6 +15,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
15     "index/base -> validation -> index/blockfilterindex -> index/base"
16     "index/coinstatsindex -> node/coinstats -> index/coinstatsindex"
17     "policy/fees -> txmempool -> policy/fees"
18+    "policy/rbf -> txmempool -> validation -> policy/rbf"

laanwj commented at 2:38 pm on August 16, 2021:

Concept ACK, but I think we should avoid adding a circular dependency here. Edit: oh, I see you already have a PR, #22677, for this. I guess it’s fine under the condition that it’s temporarily (to keep this one move-only).

glozow commented at 2:02 pm on August 19, 2021:

Yep! I’ve rebased #22677 on top of this PR so it’s more clear that it would fix this problem.

jnewbery commented at 10:39 am on August 20, 2021:

Perhaps add a note to the commit log that this circular dependency is due to the txmempool->validation dependency, and that #22677 resolves it.

glozow commented at 5:05 pm on August 20, 2021:

Okie done

theStack approved

theStack commented at 4:06 pm on August 17, 2021: member

Code-review ACK 192193c7c420ebbbac6bb0a60778e6a352b3d67c

Verified via git range-diff 6a0a8ef7...192193c7 that since my last review (https://github.com/bitcoin/bitcoin/pull/22675#pullrequestreview-729819432 and #22675#pullrequestreview-730123757) all suggestions have been tackled and that the newly added commits 7d6571ffed165d681c8b016b965aa94f41cfadb0 (renames) and 192193c7c420ebbbac6bb0a60778e6a352b3d67c (style cleanups) are okay. Kudos for the new variable names, I think the code is much more comprehensible now 👌

Good idea for reviewers to run git rebase --exec "make check; test/functional/feature_rbf.py" -i HEAD~11 in general

Oh nice, I didn’t know about git rebase --exec before (I always did review/compile/run test manually hopping from commit to commit), that is a huge time-saver! 🚀

in src/util/rbf.h:20 in 192193c7c4 outdated

14@@ -15,4 +15,7 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
15 // opt-in to replace-by-fee, according to BIP 125
16 bool SignalsOptInRBF(const CTransaction &tx);
17 
18+/** Determine whether a mempool transaction is opting out of RBF. Mempool entries do not inherit
19+ * signaling from their parents in this implementation. */
20+bool IsRBFOptOut(const CTransaction& txConflicting);

ariard commented at 11:32 pm on August 18, 2021:

I think src/util/rbf.h, did already incorporate bip125 policy value such as MAX_BIP125_RBF_SEQUENCE and if we have a newer src/policy/rbf.h encapsulating the RBF-logic better to gather in the same place to avoid bugs where our components (wallet/mempool/etc) misinterpret policy/consensus rules. E.g the wallet applying the pre-bip113 semantic for finality see #17443.

jnewbery commented at 1:16 pm on August 20, 2021:

Since you’re changing the parameter name already, consider:

0bool IsRBFOptOut(const CTransaction& tx);

glozow commented at 5:06 pm on August 20, 2021:

Deleted this, ended up just calling SignalsOptInRBF instead

in src/policy/rbf.h:52 in 192193c7c4 outdated

47+ */
48+bool GetEntriesForRBF(const CTransaction& tx, CTxMemPool& pool,
49+                      const CTxMemPool::setEntries& iters_conflicting, TxValidationState& state,
50+                      CTxMemPool::setEntries& all_conflicts) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
51+
52+/** Enforce BIP125 Rule 2: a replacement transaction must not add any new unconfirmed inputs. */

ariard commented at 0:26 am on August 19, 2021:

If you’re pointing to the exact standard rule, I think that’s better to either textually quote it to minimize risks of inconsistency OR propose an amendment to the spec with your better wording.

Note, exact standard quoting is a development practice heavily done by CL. E.g : https://github.com/ElementsProject/lightning/blob/4514d2a18098c9136d1b26b5028298f79028012e/lightningd/onchain_control.c#L31

Personally, I think that make easier to audit a codebase from the outside, as you can just grep the refs without knowing in-details the code architecture.

glozow commented at 12:28 pm on August 23, 2021:

Makes sense, quoted BIP125

in src/policy/rbf.h:68 in 192193c7c4 outdated

60+ * @param[in]   direct_conflicts    Set of mempool entries corresponding to the mempool conflicts
61+ *                                  (candidates to be replaced).
62+ * @param[in]   hash                Transaction ID, included in the error message if violation occurs.
63+ * returns true if the two sets are disjoint (i.e. intersection is empty), false if otherwise.
64+ */
65+bool SpendsAndConflictsDisjoint(const CTxMemPool::setEntries& ancestors,

ariard commented at 0:40 am on August 19, 2021:

I think a more accurate name would be “VerifyAncestorCandidateConflictsDisjunction”.

Also hash could be txid to dissociate clearly from wtxid. And (candidates to be replaced) is a bit unclear imho as a the direct conflicts aren’t mempool candidate anymore, they’re are already in until the replacement succeeds, if it does.

Also maybe, this function could be made a helper in txmempool, and if the disjunction boolean is false, the policy error is qualified.

jnewbery commented at 12:38 pm on August 20, 2021:

I agree that txid would be better than hash here (and for PaysMoreThanConflicts() below).

glozow commented at 12:27 pm on August 23, 2021:

I didn’t end up renaming hash to txid because I want to be able to pass in either package id or txid in the future :thinking: let me know if it’s too weird though, and I can change it

jnewbery commented at 3:04 pm on August 23, 2021:

I’d personally prefer fewer surprise tools that help us later, since it’s easy enough to update comments/rename parameters when you actually need that functionality. No big deal either way though!

glozow commented at 3:11 pm on August 24, 2021:

renamed to txid

ariard commented at 1:02 am on August 19, 2021: member

Concept ACK, I like the direction to drain PreChecks in isolated logical components and adding clearer documentation in the process.

I think the changes proposed by this PR are sizeable enough to be splitted off in different PRs. Especially about the replace-by-fee check rules, beyond the better-feerate-than-directly-replaced transaction present in the code but not in the spec, iirc they’re still minor discrepancies worthy to be documented.

One PRs-split-offs could be :

IsRBFOptOut() : the replacement signaling, introducing src/policy/rbf.h
SpendsAndConflictsDisjoint : the replacement candidate sanitization
PaysMoreThanConflicts/… : the replacement check rules

darosior commented at 10:04 am on August 20, 2021: member

Concept ACK nice cleanup.

I was not sure about the approach at first, since we don’t currently need txmempool in policy/rbf for IsRBFOptIn (#22665 removes it). But i agree it’s cleaner to rather have txmempool not depend on validation as you do in your followup. That said, why not proceed the other way around to avoid introducing the circular dependency at all?

glozow commented at 10:20 am on August 20, 2021: member

That said, why not proceed the other way around to avoid introducing the circular dependency at all?

True, I just did it this way because this PR is a bit further along (other one is still looking for approach feedback), and I have some branches depending on this refactor.

in src/util/rbf.h:18 in 192193c7c4 outdated

14@@ -15,4 +15,7 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
15 // opt-in to replace-by-fee, according to BIP 125
16 bool SignalsOptInRBF(const CTransaction &tx);
17 
18+/** Determine whether a mempool transaction is opting out of RBF. Mempool entries do not inherit

jnewbery commented at 10:34 am on August 20, 2021:

I don’t think this needs to mention mempool:

0/** Determine whether a transaction is opting out of RBF. Mempool entries do not inherit

in src/util/rbf.h:19 in 192193c7c4 outdated

14@@ -15,4 +15,7 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
15 // opt-in to replace-by-fee, according to BIP 125
16 bool SignalsOptInRBF(const CTransaction &tx);
17 
18+/** Determine whether a mempool transaction is opting out of RBF. Mempool entries do not inherit
19+ * signaling from their parents in this implementation. */

jnewbery commented at 10:34 am on August 20, 2021:

0 * signaling from their ancestors in this implementation. */

in src/util/rbf.cpp:37 in 192193c7c4 outdated

32+    // from BIP125's inherited signaling description (see CVE-2021-31876).
33+    // Applications relying on first-seen mempool behavior should
34+    // check all unconfirmed ancestors; otherwise an opt-in ancestor
35+    // might be replaced, causing removal of this descendant.
36+    for (const CTxIn &_txin : txConflicting.vin) {
37+        if (_txin.nSequence <= MAX_BIP125_RBF_SEQUENCE) return false;

jnewbery commented at 10:36 am on August 20, 2021:

In the final fixup commit, you could fix this to use current style:

0    for (const CTxIn& txin : txConflicting.vin) {
1        if (txin.nSequence <= MAX_BIP125_RBF_SEQUENCE) return false;

glozow commented at 5:07 pm on August 20, 2021:

(Doesn’t apply anymore because I deleted the function)

in src/policy/rbf.cpp:49 in eb3bd36a8b outdated

41@@ -42,3 +42,4 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
42     // If we don't have a local mempool we can only check the transaction itself.
43     return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;
44 }
45+

jnewbery commented at 10:37 am on August 20, 2021:

In eb3bd36a8b MOVEONLY: signal checking into policy/rbf

Don’t add this new blank line.

in src/validation.cpp:802 in 192193c7c4 outdated

775+    if (!SpendsAndConflictsDisjoint(setAncestors, setConflicts, state, hash)) return false;
776 
777-    // Check if it's economically rational to mine this transaction rather
778-    // than the ones it replaces.
779-    nConflictingFees = 0;
780-    nConflictingSize = 0;

jnewbery commented at 12:02 pm on August 20, 2021:

This is potentially a behavior change. Previously, these would get set to zero unconditionally. Now, they only get set if fReplacementTransaction is true.

I think it’d be safer to default initialize m_conflicting_fees and m_conflicting_size to 0 in the struct member declarations, and remove these assignments.

I might even go further and remove the nConflictingFees and nConflictingSize references. I think they harm readability more than help.

glozow commented at 5:07 pm on August 20, 2021:

Done

in src/policy/rbf.h:9 in 192193c7c4 outdated

5@@ -6,6 +6,11 @@
6 #define BITCOIN_POLICY_RBF_H
7 
8 #include <txmempool.h>
9+#include <consensus/validation.h>

jnewbery commented at 12:03 pm on August 20, 2021:

sort includes :)

glozow commented at 5:08 pm on August 20, 2021:

Actually decided to remove this dependency because I realized it’s not the right place to be deciding what TxValidationResult to return (should be in validation.cpp) But yes, sorted includes :salute:

in src/policy/rbf.cpp:5 in 192193c7c4 outdated

1@@ -2,14 +2,18 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include <consensus/validation.h>

jnewbery commented at 12:06 pm on August 20, 2021:

First include this file’s header, then other headers in the project:

0#include <policy/rbf.h>
1
2#include <consensus/validation.h>
3#include <logging.h>
4#include <policy/settings.h>
5#include <util/moneystr.h>
6#include <util/rbf.h>

in src/policy/rbf.cpp:57 in 192193c7c4 outdated

51+                      const CTxMemPool::setEntries& iters_conflicting,
52+                      TxValidationState& state, CTxMemPool::setEntries& all_conflicts)
53+{
54+    AssertLockHeld(pool.cs);
55+    const uint256 hash = tx.GetHash();
56+    uint64_t nConflictingCount = 0;

jnewbery commented at 12:12 pm on August 20, 2021:

Consider updating this to current style in the fixup commit:

0    uint64_t number_conflicting_tx{0};

or similar

jnewbery commented at 3:10 pm on August 23, 2021:

(not addressed)

glozow commented at 3:13 pm on August 24, 2021:

(will fix in a later PR when scripted-diffing the validation.cpp file)

in src/validation.cpp:778 in 192193c7c4 outdated

860-                    strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
861-                        hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)));
862-        }
863+        // Calculate all conflicting entries and enforce Rules 2 and 5.
864+        if (!GetEntriesForRBF(tx, m_pool, setIterConflicting, state, allConflicting)) return false;
865+        if (!HasNoNewUnconfirmed(tx, m_pool, setIterConflicting, state)) return false;

jnewbery commented at 12:19 pm on August 20, 2021:

Split this comment up:

0        // Calculate all conflicting entries and enforce BIP125 rule 5.
1        if (!GetEntriesForRBF(tx, m_pool, setIterConflicting, state, allConflicting)) return false;
2        // Enforce BIP125 rule 2.
3        if (!HasNoNewUnconfirmed(tx, m_pool, setIterConflicting, state)) return false;

(The comment is added in 28e7da0363 MOVEONLY: getting mempool conflicts to policy/rbf, which is a bit confusing since it implies that GetEntriesForRBF() enforces rule 2)

glozow commented at 5:08 pm on August 20, 2021:

Done, good point

in src/policy/rbf.cpp:6 in 192193c7c4 outdated

1@@ -2,14 +2,18 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include <consensus/validation.h>
6+#include <logging.h>

jnewbery commented at 12:24 pm on August 20, 2021:

You don’t actually need logging.h. You could just include tinyformat.h.

glozow commented at 5:09 pm on August 20, 2021:

Switched to tinyformat

in src/policy/rbf.h:45 in 192193c7c4 outdated

40+ * possible. There cannot be more than MAX_BIP125_REPLACEMENT_CANDIDATES potential entries.
41+ * @param[in]   iters_conflicting   The set of iterators to mempool entries.
42+ * @param[out]  state               Used to return errors, if any.
43+ * @param[out]  all_conflicts       Populated with all the mempool entries that would be replaced,
44+ *                                  which includes descendants of iters_conflicting. Not cleared at
45+ *                                  the start; any existing mempool entries will remain in the set.

jnewbery commented at 12:27 pm on August 20, 2021:

This seems like an odd detail to specify in the interface, when this function is only ever called with an empty all_conflicts set.

glozow commented at 2:28 pm on August 20, 2021:

glozow commented at 2:31 pm on August 20, 2021:

https://github.com/bitcoin/bitcoin/pull/22290/commits/6c04bf5a60e2af86c326c4e65aba777d8bf768bb#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R970-R972

in src/policy/rbf.h:48 in 192193c7c4 outdated

43+ * @param[out]  all_conflicts       Populated with all the mempool entries that would be replaced,
44+ *                                  which includes descendants of iters_conflicting. Not cleared at
45+ *                                  the start; any existing mempool entries will remain in the set.
46+ * @returns false if Rule 5 is broken.
47+ */
48+bool GetEntriesForRBF(const CTransaction& tx, CTxMemPool& pool,

jnewbery commented at 12:31 pm on August 20, 2021:

Perhaps this should be named GetEntriesForRBFConflicts() or similar?

glozow commented at 5:15 pm on August 20, 2021:

I named it GetEntriesForConflicts()

in src/policy/rbf.cpp:94 in 192193c7c4 outdated

85+            parents_of_conflicts.insert(txin.prevout.hash);
86+        }
87+    }
88+
89+    for (unsigned int j = 0; j < tx.vin.size(); j++)
90+    {

jnewbery commented at 12:33 pm on August 20, 2021:

In fixup commit:

0    for (unsigned int j = 0; j < tx.vin.size(); j++) {

in src/policy/rbf.h:65 in 192193c7c4 outdated

56+
57+/** Check the intersection between two sets of mempool entries to make sure they are disjoint.
58+ * @param[in]   ancestors           Set of mempool entries corresponding to ancestors of the
59+ *                                  replacement transactions.
60+ * @param[in]   direct_conflicts    Set of mempool entries corresponding to the mempool conflicts
61+ *                                  (candidates to be replaced).

jnewbery commented at 12:37 pm on August 20, 2021:

These are txids, not mempool entries:

0 * [@param](/bitcoin-bitcoin/contributor/param/)[in]   direct_conflicts    Set of txids for entries corresponding to the mempool conflicts
1 *                                  (candidates to be replaced).

This comment uses the wrong argument name in commit 4f656bb23c MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (fixed in later commit)

glozow commented at 5:15 pm on August 20, 2021:

Done and fixed the commit jumping

in src/policy/rbf.h:60 in 192193c7c4 outdated

52+/** Enforce BIP125 Rule 2: a replacement transaction must not add any new unconfirmed inputs. */
53+bool HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
54+                         const CTxMemPool::setEntries& iters_conflicting,
55+                         TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
56+
57+/** Check the intersection between two sets of mempool entries to make sure they are disjoint.

jnewbery commented at 12:39 pm on August 20, 2021:

As below, the second argument isn’t mempool entries.

jnewbery commented at 3:12 pm on August 23, 2021:

(not addressed - this comment still refers to “two sets of mempool entries”)

glozow commented at 3:13 pm on August 24, 2021:

now calling it “two sets of transactions (a set of mempool entries and a set of txids)”

in src/policy/rbf.cpp:119 in 192193c7c4 outdated

114+                                TxValidationState& state, const uint256& hash)
115+{
116+    for (CTxMemPool::txiter ancestorIt : ancestors) {
117+        const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
118+        if (direct_conflicts.count(hashAncestor)) {
119+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx",

jnewbery commented at 12:43 pm on August 20, 2021:

Maybe worth adding a comment here that this is a consensus rule violation since the transaction is self-inconsistent, and can never be valid.

glozow commented at 5:14 pm on August 20, 2021:

good eyes. added a comment

in src/policy/rbf.cpp:157 in 192193c7c4 outdated

152+}
153+
154+bool PaysForRBF(CAmount original_fees, CAmount replacement_fees, size_t replacement_vsize,
155+                TxValidationState& state, const uint256& hash)
156+{
157+    // The replacement must pay greater fees than the transactions it replaces - if we did the

jnewbery commented at 12:58 pm on August 20, 2021:

Maybe explicitly mention BIP125 rule 3 in this comment.

glozow commented at 5:14 pm on August 20, 2021:

Marked all of them with their corresponding BIP125 rule numbers :+1:

in src/policy/rbf.cpp:167 in 192193c7c4 outdated

162+                      hash.ToString(),
163+                      FormatMoney(replacement_fees),
164+                      FormatMoney(original_fees)));
165+    }
166+
167+    // Finally in addition to paying more fees than the conflicts the new transaction must pay for

jnewbery commented at 12:59 pm on August 20, 2021:

Maybe explicitly mention BIP125 rule 4 in this comment. Also remove “Finally” - I guess that was there before because this was the final check in PreChecks().

in src/policy/rbf.cpp:92 in 192193c7c4 outdated

88+
89+    for (unsigned int j = 0; j < tx.vin.size(); j++)
90+    {
91+        // We don't want to accept replacements that require low feerate junk to be mined first.
92+        // Ideally we'd keep track of the ancestor feerates and make the decision based on that, but
93+        // for now requiring all new inputs to be confirmed works.

jnewbery commented at 1:02 pm on August 20, 2021:

Maybe explicitly mention that this is BIP125 rule 2.

in src/util/rbf.cpp:22 in 192193c7c4 outdated

14@@ -15,3 +15,26 @@ bool SignalsOptInRBF(const CTransaction &tx)
15     }
16     return false;
17 }
18+
19+bool IsRBFOptOut(const CTransaction& txConflicting)
20+{
21+    // Allow opt-out of transaction replacement by setting
22+    // nSequence > MAX_BIP125_RBF_SEQUENCE (SEQUENCE_FINAL-2) on all inputs.

jnewbery commented at 1:03 pm on August 20, 2021:

Maybe explicitly comment that this is BIP125 rule 1.

in src/policy/rbf.cpp:62 in 192193c7c4 outdated

57+    for (const auto& mi : iters_conflicting) {
58+        nConflictingCount += mi->GetCountWithDescendants();
59+        // This potentially overestimates the number of actual descendants but we just want to be
60+        // conservative to avoid doing too much work.
61+        if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) {
62+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements",

jnewbery commented at 1:05 pm on August 20, 2021:

Maybe explicitly comment that this is BIP125 rule 5.

jnewbery commented at 1:17 pm on August 20, 2021: member

Looks good!

The logs for some of the commits could use a little more detail, e.g. e535422365 [validation] quit earlier and separate loops could have a better summary (Quit what earlier? Separate which loops? At the very least this should mention ATMP or RBF). The same is true for some of the other commit logs as well.

glozow force-pushed on Aug 20, 2021

in src/policy/rbf.cpp:10 in 7d9fc8153c outdated

 2@@ -3,13 +3,17 @@
 3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 4 
 5 #include <policy/rbf.h>
 6+
 7+#include <policy/settings.h>
 8+#include <util/moneystr.h>
 9 #include <util/rbf.h>
10+#include <tinyformat.h>

jnewbery commented at 2:24 pm on August 23, 2021:

sort plz

glozow commented at 3:12 pm on August 24, 2021:

Fixt

in src/validation.cpp:797 in 7d9fc8153c outdated

885-                                hash.ToString(), j));
886-                }
887-            }
888+        // Calculate all conflicting entries and enforce Rule #5.
889+        if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) {
890+            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, err_string);

jnewbery commented at 2:51 pm on August 23, 2021:

You lost the m_reject_reason:

0            return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many replacement transactions", err_string);

(this is why default arguments are evil, especially when you have multiple default arguments of the same type or of types that can be implicitly cast to each other)

glozow commented at 3:12 pm on August 24, 2021:

:pleading_face: Added it back

jnewbery commented at 3:07 pm on August 23, 2021: member

In commit 76828aa9c1 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf, you update the doxygen comment for HasNoNewUnconfirmed(). That change should have been squashed into commit 526b490e81 MOVEONLY: BIP125 Rule 2 to policy/rbf.

glozow force-pushed on Aug 24, 2021

glozow commented at 3:14 pm on August 24, 2021: member

Addressed @jnewbery comments

theStack commented at 11:44 am on August 25, 2021: member

I tend to agree with @ariard that splitting this PR into different ones would make a lot of sense here:

I think the changes proposed by this PR are sizeable enough to be splitted off in different PRs. Especially about the replace-by-fee check rules, beyond the better-feerate-than-directly-replaced transaction present in the code but not in the spec, iirc they’re still minor discrepancies worthy to be documented.

One PRs-split-offs could be :

IsRBFOptOut() : the replacement signaling, introducing src/policy/rbf.h

SpendsAndConflictsDisjoint : the replacement candidate sanitization

PaysMoreThanConflicts/… : the replacement check rules

Maybe it’s my lousy git skills, but I just tried to re-review the changes since my last ACK via git range-diff and the results are rather long (> 800 LOC) and confusing (e.g. some commits have been completely removed), to a point that a complete new review is probably quicker.

jnewbery commented at 12:19 pm on August 25, 2021: member

Code review ACK a33fdd0b981965982754b39586eedb7ae456ba57

I’m very happy to review any new PRs if you split this up.

glozow commented at 1:31 pm on August 25, 2021: member

I’ve split off the first few commits into #22796.

fanquake referenced this in commit 81f4a3e84d on Aug 31, 2021

glozow force-pushed on Sep 1, 2021

glozow renamed this:
~~MOVEONLY policy: extract RBF logic into policy/rbf~~
RBF move 2/3: extract RBF logic into policy/rbf
on Sep 1, 2021

glozow commented at 9:08 am on September 1, 2021: member

Rebased and split off the documentation changes. Ready for review. Please note that this is move-only, and I’m happy to add any doc suggestions to #22855.

Make GetEntriesForConflicts return std::optional

Avoids reusing err_string.

f8ad2a57c6

MOVEONLY: BIP125 Rule 2 to policy/rbf 7b60c02b7d

MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf

This checks that a transaction isn't trying to replace something it
supposedly depends on.

3f033f01a6

MOVEONLY: check that fees > direct conflicts to policy/rbf 9c2f9f8984

MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf ac761f0a23

scripted-diff: rename variables in policy/rbf

"Fee Delta" is already a term used for prioritizing transactions:
modified = base fees + delta

Here, delta also means the difference between original and modified replacement fees:
nDeltaFees = (original_base + original_delta) - (replacement_base + replacement_delta)

This is insanely confusing. Also, since mempool is no longer a member of a
class (MemPoolAccept.m_pool), the "m" prefix is unnecessary. The rest are
clarity/style-focused changes to already-touched lines.

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/\<$1\>/$2/g" src/policy/rbf* ; }

ren nDeltaFees additional_fees
ren m_pool pool

ren nSize replacement_vsize
ren nModifiedFees replacement_fees
ren nConflictingFees original_fees
ren oldFeeRate original_feerate
ren newFeeRate replacement_feerate

ren setAncestors ancestors
ren setIterConflicting iters_conflicting
ren setConflictsParents parents_of_conflicts
ren setConflicts direct_conflicts
ren allConflicting all_conflicts

sed -i "s/ hash\b/ txid/g" src/policy/rbf*
-END VERIFY SCRIPT-

fa47622e8d

whitespace fixups after move and scripted-diff 32748da0f4

glozow force-pushed on Sep 2, 2021

glozow commented at 3:42 pm on September 2, 2021: member

Changed the functions to return std::optional<std::string> instead of bool in response to #22796 (review)

laanwj added this to the "Blockers" column in a project

mjdietzx commented at 1:46 am on September 3, 2021: contributor

ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624

I tested this again by running some additional test coverage around BIP125 (this PR #22867) on this branch. All good ✅

theStack approved

theStack commented at 6:49 pm on September 5, 2021: member

Code-review ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624 📐

Built and ran unit tests and functional test ./test/functional/feature_rbf.py at each commit, verified by review that there is no change in behaviour. Here are some naming/documentation improvement nits:

to increase clarity, you could explicitely refer to BIP125 when mentioning rules in the comments in MemPoolAccept::PreChecks(), e.g. s/Enforce Rule [#2](/bitcoin-bitcoin/2/)/Enforce BIP125 Rule [#2](/bitcoin-bitcoin/2/)/
though I totally understand the motivation behind the new interface, returning std::optional<std::string> seems a bit confusing for functions that previously returned bool. The function name is like a predicate, and the predicate (e.g. PaysForRBF) is now unexpectedly true if nothing (std::nullopt) is returned, leading to an inverted logic for the reader of the if-clause where it is called. Maybe prefix the function names with something like Verify or Check?

in src/policy/rbf.cpp:181 in ac761f0a23 outdated

176+    }
177+
178+    // Finally in addition to paying more fees than the conflicts the
179+    // new transaction must pay for its own bandwidth.
180+    CAmount nDeltaFees = nModifiedFees - nConflictingFees;
181+    if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))

MarcoFalke commented at 9:09 am on September 6, 2021:

ac761f0a23c9c469fa00885edf3d5c9ae7c6a2b3: Would be nice to not use a global here. Testability is one of the motivations in OP, but using globals makes testing actually harder.

glozow commented at 11:15 am on September 6, 2021:

Yes definitely agree - I can do this in the followup PR.

glozow commented at 8:57 am on September 8, 2021:

Done in https://github.com/bitcoin/bitcoin/pull/22855/commits/7ccaac6c5332b8a93c9b5c7103f846854ee595d5

MarcoFalke approved

MarcoFalke commented at 9:15 am on September 6, 2021: member

review ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624 🦇

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624 🦇
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjoaQwAggcrCYBYlCDfIy4FYpWzVg9dKVuvdwpkRXNo146y0z4w2cNoJa+08akW
 8UfBbdsu2YlwbPKlgODU6LARsjq5PE8nj+vkj1S3Z9MnvhoXh/UwLlRci2bDDshn1
 9YyiTdtiFfN1UYFQ4ShhunipzGK6ZNALWZUDFXCIWbtVQUYS0shmWls/Dchah94lf
102hmK9vGXLbtpAuR/9zxYguUL9ixcsBtALCI3GG2DspZIHON3aJBZDP59HJXXINo+
11aQoFNw77QfQKa+GMKLvCW8/5eQDVue5xAJyckB+EyJjXE2ztcskn2riA+VP+I0Zu
12PfVcqgkfyy5Ci/8BUjvtWAIYGa+WUeJjbtd/DVw80E9CpqILj3TPvrpTeA2q3bHC
13PAJQMrmZbEGQgMir9Zj/c9Bh54+9MnEhKwo5JqktBI110ohJsIkFwuT75hXrKRjn
1453jrMg7LR9kzA2Hmg625hgxJTNthv1QMZrerkhRjA1ad/s1bE/hGnN2Lk5yYy/8k
15FATeV8qZ
16=rsJ0
17-----END PGP SIGNATURE-----

Timestamp of file with hash 23997a2a7eb742c9c9782006676c24ca77f830db8c1bb6251e751a60a1f59bc5 -

The scripted diff might also modify out-of-tree files. You can fix that by passing the glob through git grep -l:

0sed ... $(git grep -l "$1" ./src/policy/rbf*)

Also in the last commit, if you fixup the file so much, might as well go the extra step and fixup the whole file’s whitespace:

 0diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp
 1index 15527afb8a..d524b95b89 100644
 2--- a/src/policy/rbf.cpp
 3+++ b/src/policy/rbf.cpp
 4@@ -48,9 +48,9 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
 5 }
 6 
 7 std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx,
 8-                            CTxMemPool& pool,
 9-                            const CTxMemPool::setEntries& iters_conflicting,
10-                            CTxMemPool::setEntries& all_conflicts)
11+                                                  CTxMemPool& pool,
12+                                                  const CTxMemPool::setEntries& iters_conflicting,
13+                                                  CTxMemPool::setEntries& all_conflicts)
14 {
15     AssertLockHeld(pool.cs);
16     const uint256 txid = tx.GetHash();
17@@ -81,7 +81,7 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
18     AssertLockHeld(pool.cs);
19     std::set<uint256> parents_of_conflicts;
20     for (const auto& mi : iters_conflicting) {
21-        for (const CTxIn &txin : mi->GetTx().vin) {
22+        for (const CTxIn& txin : mi->GetTx().vin) {
23             parents_of_conflicts.insert(txin.prevout.hash);
24         }
25     }
26@@ -111,7 +111,7 @@ std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries&
27                                                    const uint256& txid)
28 {
29     for (CTxMemPool::txiter ancestorIt : ancestors) {
30-        const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
31+        const uint256& hashAncestor = ancestorIt->GetTx().GetHash();
32         if (direct_conflicts.count(hashAncestor)) {
33             return strprintf("%s spends conflicting transaction %s",
34                              txid.ToString(),
35diff --git a/src/policy/rbf.h b/src/policy/rbf.h
36index 56468a09b2..3f37bb09bf 100644
37--- a/src/policy/rbf.h
38+++ b/src/policy/rbf.h
39@@ -48,14 +48,14 @@ RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx);
40 std::optional<std::string> GetEntriesForConflicts(const CTransaction& tx, CTxMemPool& pool,
41                                                   const CTxMemPool::setEntries& iters_conflicting,
42                                                   CTxMemPool::setEntries& all_conflicts)
43-                                                  EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
44+    EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
45 
46 /** BIP125 Rule [#2](/bitcoin-bitcoin/2/): "The replacement transaction may only include an unconfirmed input if that input
47  * was included in one of the original transactions."
48  * [@returns](/bitcoin-bitcoin/contributor/returns/) error message if Rule [#2](/bitcoin-bitcoin/2/) is broken, otherwise std::nullopt. */
49 std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTxMemPool& pool,
50                                                const CTxMemPool::setEntries& iters_conflicting)
51-                                               EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
52+    EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
53 
54 /** Check the intersection between two sets of transactions (a set of mempool entries and a set of
55  * txids) to make sure they are disjoint.
56diff --git a/src/util/rbf.h b/src/util/rbf.h
57index 6d44a2cb83..db9549e3ed 100644
58--- a/src/util/rbf.h
59+++ b/src/util/rbf.h
60@@ -18,6 +18,6 @@ static const uint32_t MAX_BIP125_RBF_SEQUENCE = 0xfffffffd;
61 * SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by non-replaceable transactions. All
62 * inputs rather than just one is for the sake of multi-party protocols, where we don't want a single
63 * party to be able to disable replacement. */
64-bool SignalsOptInRBF(const CTransaction &tx);
65+bool SignalsOptInRBF(const CTransaction& tx);
66 
67 #endif // BITCOIN_UTIL_RBF_H

fanquake merged this on Sep 10, 2021

fanquake closed this on Sep 10, 2021

glozow deleted the branch on Sep 10, 2021

in src/policy/rbf.h:84 in 32748da0f4

89+                                                 CFeeRate replacement_feerate, const uint256& txid);
90+
91+/** Enforce BIP125 Rule #3 "The replacement transaction pays an absolute fee of at least the sum
92+ * paid by the original transactions." Enforce BIP125 Rule #4 "The replacement transaction must also
93+ * pay for its own bandwidth at or above the rate set by the node's minimum relay fee setting."
94+ * @param[in]   original_fees       Total modified fees of original transaction(s).

ariard commented at 10:19 pm on September 10, 2021:

Note, as previously mentioned, I think this is where we do have additional divergences w.r.t to bip125.

Rule 3 says “The replacement transaction pays an absolute fee of at least the sum paid by the original transactions”.

I think original transactions as to be interpreted as the set of directly conflicted transactions, i.e sharing inputs with the replacement transaction. At least Rule 5 explicitly dissociates “original transactions to be replaced and their descendant transactions”.

However our RBF implementation computes the original_fees from iterating on allConflicting which is built from CalculateDescendants. So to-be-replaced descendants fees are also added to the sum paid checked.

This means our implementation is more conservative than BIP125, as the replacement threshold is higher. I think this is preferable, at least without deeper thinking.

AFAIK, this is mental model shared by most developers when they reason about our RBF logic (e.g in pinning discussions). Though it might create surprises if a user submits replacement transactions to Core and other full-node like bcoin, btcd (I don’t know their behaviors), they might be refused by Core but accepted in other implems.

If I’m correct, good to make this point more known.

darosior commented at 1:24 pm on September 14, 2021:

You are correct with regard to the implementation. However BIP125 says:

The replacement transaction pays an absolute fee of at least the sum paid by the original transactions

Which is plural, so i think the implementation is in line with this rule. Of course, i agree that it would be a nice place to look for bugs in re-implementations :)

glozow commented at 10:41 am on September 15, 2021:

At least wrt fees, it wouldn’t make much sense to not consider the descendants of direct conflicts. You can very easily end up replacing higher-fee packages with lower-fee ones. As such, I don’t expect that other implementations are interpreting the BIP wording as excluding descendants. Good shout, though, since that would be a pretty juicy bug! :bug:

wrt wording used in these comments, I have been trying to use “direct conflicts” as the mempool transactions that a transaction conflicts with, and “original transactions” as all of the mempool transactions that would need to be evicted, i.e. direct conflicts and their descendants. I hope that’s clear enough - welcome any feedback on that

ariard commented at 9:20 pm on September 26, 2021:

Which is plural, so i think the implementation is in line with this rule.

Yes “original transactions” is plural though Rule 5 explicitly dissociates “original transactions to be replaced and their descendant transactions”. Like the directly-replaced transactions and their descendants as two different things :) ?

I still think this is confusing and our implementation is not in line with this rule. Though I think this is the best behavior for the reason underscored by Gloria.

I’ll let someone else checking other full-node implementations if they have bugs around that!

in src/policy/rbf.cpp:94 in 32748da0f4

108+    for (unsigned int j = 0; j < tx.vin.size(); j++) {
109+        // We don't want to accept replacements that require low feerate junk to be mined first.
110+        // Ideally we'd keep track of the ancestor feerates and make the decision based on that, but
111+        // for now requiring all new inputs to be confirmed works.
112+        //
113+        // Note that if you relax this to make RBF a little more useful, this may break the

ariard commented at 10:20 pm on September 10, 2021:

In case of code relocation like this one, I think it could ease understanding if a reference to PreChecks is added, where the calls to CalculateMempoolAncestors happen.

ariard commented at 10:21 pm on September 10, 2021: member

Post-merge Code Review ACK 32748da0

fanquake removed this from the "Blockers" column in a project

sidhujag referenced this in commit 4e41f85277 on Sep 11, 2021

darosior commented at 1:25 pm on September 14, 2021: member

late-to-the-party ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624

fanquake referenced this in commit 8bda5e0988 on Sep 23, 2021

DrahtBot locked this on Oct 30, 2022

RBF move 2/3: extract RBF logic into policy/rbf #22675

Conflicts