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

pull glozow wants to merge 7 commits into bitcoin:master from glozow:2021-08-rbf changing 4 files +189 −131
  1. 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.
  2. DrahtBot added the label TX fees and policy on Aug 10, 2021
  3. DrahtBot added the label Validation on Aug 10, 2021
  4. glozow force-pushed on Aug 10, 2021
  5. glozow added the label Refactoring on Aug 10, 2021
  6. 0xB10C commented at 2:48 pm on August 10, 2021: member

    Concept ACK

    TIL Witness Replacement

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

  9. fanquake commented at 2:45 am on August 11, 2021: member
    Concept ACK
  10. fanquake requested review from darosior on Aug 11, 2021
  11. 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
  12. 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
  13. 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.
  14. 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.
  15. 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:
  16. 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
  17. 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!
  18. mjdietzx commented at 11:05 am on August 11, 2021: contributor
    Concept ACK. Very well done, I think this is a nice improvement
  19. glozow force-pushed on Aug 11, 2021
  20. mjdietzx commented at 1:24 am on August 12, 2021: contributor
    crACK 6a0a8ef791400d78fb4192a81a392131be78270b
  21. theStack commented at 12:11 pm on August 12, 2021: member
    Concept ACK
  22. mjdietzx commented at 2:49 pm on August 13, 2021: contributor
    Tested ACK 6a0a8ef791400d78fb4192a81a392131be78270b
  23. 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!
  24. 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
  25. 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.

  26. 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
  27. 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
  28. 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
  29. 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
  30. 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.
  31. glozow force-pushed on Aug 16, 2021
  32. 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

  33. 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
  34. theStack approved
  35. 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! 🚀

  36. 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
  37. 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
  38. 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
  39. 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
  40. 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?

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

  42. 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
    
  43. 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. */
    
  44. 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)
  45. 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.

  46. 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
  47. 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:
  48. 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>
    
  49. 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)
  50. 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
  51. 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
  52. 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:
    image

  53. 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()
  54. 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++) {
    
  55. 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
  56. 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)”
  57. 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
  58. 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:
  59. 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().
  60. 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.
  61. 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.
  62. 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.
  63. 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.

  64. glozow force-pushed on Aug 20, 2021
  65. 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
  66. 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
  67. 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.
  68. glozow force-pushed on Aug 24, 2021
  69. glozow commented at 3:14 pm on August 24, 2021: member
    Addressed @jnewbery comments
  70. 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.

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

  72. glozow commented at 1:31 pm on August 25, 2021: member
    I’ve split off the first few commits into #22796.
  73. fanquake referenced this in commit 81f4a3e84d on Aug 31, 2021
  74. glozow force-pushed on Sep 1, 2021
  75. glozow force-pushed on Sep 1, 2021
  76. 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
  77. 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.
  78. Make GetEntriesForConflicts return std::optional
    Avoids reusing err_string.
    f8ad2a57c6
  79. MOVEONLY: BIP125 Rule 2 to policy/rbf 7b60c02b7d
  80. 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
  81. MOVEONLY: check that fees > direct conflicts to policy/rbf 9c2f9f8984
  82. MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf ac761f0a23
  83. 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
  84. whitespace fixups after move and scripted-diff 32748da0f4
  85. glozow force-pushed on Sep 2, 2021
  86. 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)
  87. laanwj added this to the "Blockers" column in a project

  88. 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 ✅

  89. theStack approved
  90. 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?
  91. 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.

  92. MarcoFalke approved
  93. 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
    
  94. fanquake merged this on Sep 10, 2021
  95. fanquake closed this on Sep 10, 2021

  96. glozow deleted the branch on Sep 10, 2021
  97. 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!

  98. 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.
  99. ariard commented at 10:21 pm on September 10, 2021: member
    Post-merge Code Review ACK 32748da0
  100. fanquake removed this from the "Blockers" column in a project

  101. sidhujag referenced this in commit 4e41f85277 on Sep 11, 2021
  102. darosior commented at 1:25 pm on September 14, 2021: member
    late-to-the-party ACK 32748da0f47f7aa9fba78dfb29aa426b14f15624
  103. fanquake referenced this in commit 8bda5e0988 on Sep 23, 2021
  104. DrahtBot locked this on Oct 30, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 07:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me