RBF move (1/3): extract BIP125 Rule 5 into policy/rbf #22796

glozow · 2021-08-25T13:30:02Z

See #22675 for motivation, this is one chunk of it. It extracts some BIP125 logic into policy/rbf:

Defines a constant for specifying the maximum number of mempool entries we'd consider replacing by RBF
Calls the available SignalsOptInRBF function instead of manually iterating through inputs
Moves the logic for getting the list of conflicting mempool entries to a helper function
Also does a bit of preparation for future moves - moving declarations around, etc
Also see cut the validation <-> txmempool circular dependency 2/2 #22677 for addressing the circular dependency.

A circular dependency is added because policy now depends on txmempool and txmempool depends on validation. It is natural for [mempool] policy to rely on mempool; the problem is caused by txmempool depending on validation. bitcoin#22677 will resolve this.

This should have no effect in practice, since we only ever call PreChecks once per transaction.

No behavior change. While we're looking through the descendants and calculating how many transactions we might replace, quit early, as soon as we hit 100. Since we're failing faster, we can also separate the loops - yes, we loop through more times, but this helps us detangle the different BIP125 rules later.

jnewbery · 2021-08-25T13:47:26Z

Code review ACK f293c68

theStack

Thanks for splitting up, this way the changes are easier to digest and should also potentially attract more reviewers.

Code-review ACK f293c68 📔

theStack · 2021-08-25T19:45:57Z

src/policy/rbf.cpp

+
+#include <policy/settings.h>
+#include <tinyformat.h>
+#include <util/moneystr.h>


nit: I think this include is not needed in this compilation unit (neither FormatMoney nor ParseMoney is used anywhere here)

Suggested change

#include <util/moneystr.h>

ah true, I guess not needed until 781e100

will update if i need to repush

DrahtBot · 2021-08-25T23:49:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22698 (Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status by mjdietzx)
#22677 ([RFC] cut the validation <-> txmempool circular dependency by glozow)
#22665 (policy/rbf: don't return "incorrect" replaceability status by darosior)
#22539 (Re-include RBF replacement txs in fee estimation by darosior)
#22290 (Package Mempool Submission with Package Fee-Bumping by glozow)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

ariard

ACK f293c68

I verified that test_too_many_replacements in test/functional/feature_rbf.py still cover correctly the refactored bip 125 rule 5 check.

I think any comments if worthy can be addressed in a follow-up or be taken on another PR of this serie of works.

ariard · 2021-08-27T21:58:06Z

src/util/rbf.h

+* SEQUENCE_FINAL-1 is picked to still allow use of nLockTime by
+* non-replaceable transactions. All inputs rather than just one
+* is for the sake of multi-party protocols, where we don't
+* want a single party to be able to disable replacement. */


nit: "...by opt-in out its contributed input"

Noted, will add this to a33fdd0!

Taken in #22855

ariard · 2021-08-27T22:13:48Z

src/validation.cpp

-        CAmount m_conflicting_fees;
-        size_t m_conflicting_size;
+        /** Total modified fees of all transactions being replaced. */
+        CAmount m_conflicting_fees{0};


I think what we called "modified fees" in before-this-PR PreChecks() are currently the base_fee of the mempool candidate plus the delta from PrioritiseTransaction (L698 in src/validation.cpp). Maybe the "modified" adjective can be dropped ?

Correct, modified is base + delta from prioritise. But why should we drop the "modified" adjective? It's still very widely used in the mempool and MemPoolAccept code. In my view, just saying "fees" is less clear, as I would assume that to mean base fees.

ariard · 2021-08-27T23:06:43Z

src/validation.cpp


+        std::set<uint256> setConflictsParents;
+        for (const auto& mi : setIterConflicting) {


Note, I think this commit is changing a bit the processing, Previously, the 3 operations a) checking candidate feerate is better than any replaced transaction b) fulfilling the set of parents inputs and c) accounting replaced transactions with descendants in the same loop.

After this commit, setIterConflicting is iterated 3 times. Overall I think it's okay, though it might add a bit of work in case of well-sized set of replaced transactions. From a DoS viewpoint no changes. An upper bound is still enforced with nConflictingCount against MAX_BIP_125_REPLACEMENT_CANDIDATES (that would be great to have a set of QA assets with a wide range of mempool candidates payloads to confirm or infirm that kind of intuitions, not sure that's an information you can currently get from qa-assets/fuzz_seed_corpus/validation_load_mempool)

Right, I was hoping to make this clear in the commit messages. Because of the changes in the preceding commit, we now quit immediately when we see more than MAX_BIP_125_REPLACEMENT_CANDIDATES. Previously, we'd grab all potential candidates and only check the total number afterward. So this PR has, overall, reduced the complexity from O(n) to O(1) where n is the number of conflicts, which is why I think it's okay to iterate 3 times.

The only observable "behavior" change is that we might return too many potential replacements instead of insufficient fee when a transaction violates both.

ariard · 2021-08-27T23:31:04Z

src/policy/rbf.h

+ * @param[out]  err_string          Used to return errors, if any.
+ * @param[out]  allConflicting      Populated with all the mempool entries that would be replaced,
+ *                                  which includes descendants of setIterConflicting. Not cleared at
+ *                                  the start; any existing mempool entries will remain in the set.


What do you mean by "Not cleared at the start; any existing mempool entries will remain in the set" ?

If the replacement is successful, allConflicting are going to be removed in Finalize. For package support, I think this operation we'll remove the per-package tx allConflicting set all at once. If PreChecks or any new other package policy checks as failed, don't modify mempool state.

It's similar to other mempool functions where you can pass in a setEntries and it will insert entries, but not expect it to be empty beforehand, so you can call it multiple times and end up with an aggregated set. I'm expecting to use this in package RBF. It has nothing to do with what's going to end up in the mempool; this is just a helper function.

The reason I document it here is I think it's a relevant detail in the interface. It's the caller's responsibility to clear the set beforehand if it needs that.

src/policy/rbf.cpp

fanquake · 2021-08-31T14:34:01Z

Please comment here when you're addressing followup comments / nits via the next PRs in the series.

…y/rbf

darosior

Post-merge ACK f293c68

darosior · 2021-09-01T07:13:23Z

src/validation.cpp

+        for (CTxMemPool::txiter it : allConflicting) {
+            nConflictingFees += it->GetModifiedFee();
+            nConflictingSize += it->GetTxSize();
+        }


In the same spirit of exiting early, i think we could check if (nModifiedFees < nConflictingFees) here directly. We could probably also check the delta fee.

I'm not sure if that would be much of an improvement? There would be <=100 entries to aggregate fees for, so this is pretty tightly bounded already.

glozow · 2021-09-01T09:11:34Z

Next PR is #22675!
Addressing documentation-related comments in #22855

maflcko

cr ACK f293c68

but I don't like the symbol re-use

maflcko · 2021-09-02T10:11:13Z

src/validation.cpp

+        }
+
+        // Calculate all conflicting entries and enforce Rule #5.
+        if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) {


f293c68: I think for new code we should avoid leaking mutable return args into a greater scope than needed. Especially when the goal is to re-use (https://github.com/bitcoin/bitcoin/pull/22675/files) the symbol (err_string), when it shouldn't be re-used. With C++17 this is trivial to fix.

One example:

if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) { return ..., *err_string;

and make the util function return an error string or nullopt if no error is hit.

Re-using symbols has bitten us in ATMP in the past, so I don't think there is need to repeat that. See commit 116419e

Good point, I've added a followup commit for GetEntriesForConflicts and edited the rest of the functions to return a std::optional<std::string> in #22675

glozow added 5 commits August 24, 2021 15:47

[validation] default conflicting fees and size to 0

e0df41d

This should have no effect in practice, since we only ever call PreChecks once per transaction.

call SignalsOptInRBF instead of checking all inputs

badb9b1

MOVEONLY: getting mempool conflicts to policy/rbf

f293c68

glozow mentioned this pull request Aug 25, 2021

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

Merged

glozow added Refactoring TX fees and policy Validation labels Aug 25, 2021

glozow changed the title ~~RBF move (1/n): extract BIP125 Rule 5 into policy/rbf~~ RBF move (1/3): extract BIP125 Rule 5 into policy/rbf Aug 25, 2021

theStack approved these changes Aug 25, 2021

View reviewed changes

This was referenced Aug 26, 2021

Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698

Closed

cut the validation <-> txmempool circular dependency 2/2 #22677

Merged

policy/rbf: don't return "incorrect" replaceability status #22665

Closed

Herrytheeagle approved these changes Aug 26, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 26, 2021

Re-include RBF replacement txs in fee estimation #22539

Merged

Herrytheeagle mentioned this pull request Aug 26, 2021

. #22802

Closed

Herrytheeagle approved these changes Aug 26, 2021

View reviewed changes

This was referenced Aug 26, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

Add fee_est tool for debugging fee estimation code #10443

Closed

ariard reviewed Aug 27, 2021

View reviewed changes

fanquake merged commit 81f4a3e into bitcoin:master Aug 31, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 31, 2021

Merge bitcoin#22796: RBF move (1/3): extract BIP125 Rule 5 into polic…

74f4639

…y/rbf

darosior reviewed Sep 1, 2021

View reviewed changes

glozow deleted the 2021-08-rbf-1 branch September 1, 2021 09:10

maflcko reviewed Sep 2, 2021

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 2, 2022


		std::set<uint256> setConflictsParents;
		for (const auto& mi : setIterConflicting) {

RBF move (1/3): extract BIP125 Rule 5 into policy/rbf #22796

RBF move (1/3): extract BIP125 Rule 5 into policy/rbf #22796

Uh oh!

Conversation

glozow commented Aug 25, 2021

Uh oh!

jnewbery commented Aug 25, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 25, 2021

Conflicts

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Aug 31, 2021

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Sep 1, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow Aug 31, 2021 •

edited

Loading