29242 Diagram check followups

instagibbs commented at 4:13 pm on March 25, 2024: member

Follow-ups to https://github.com/bitcoin/bitcoin/pull/29242

DrahtBot commented at 4:13 pm on March 25, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, murchandamus, ismaelsadeeq, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29757 (feefrac: avoid explicitly computing diagram; compare based on chunks by sipa)

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.

instagibbs force-pushed on Mar 25, 2024

DrahtBot added the label CI failed on Mar 25, 2024

DrahtBot commented at 6:04 pm on March 25, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23064649168

DrahtBot removed the label CI failed on Mar 25, 2024

in src/txmempool.cpp:1305 in 836841e268 outdated

1301@@ -1302,7 +1302,7 @@ util::Result<std::pair<std::vector<FeeFrac>, std::vector<FeeFrac>>> CTxMemPool::
1302     // Step 1: build the old diagram.
1303 
1304     // The above clusters are all trivially linearized;
1305-    // they have a strict topology of 1 or two connected transactions.
1306+    // they have a strict topology of one or two connected transactions.

glozow commented at 9:14 am on March 26, 2024:

836841e2683fa0ca3fd7837726d935e52be142bc seems a bit trivial :sweat_smile:

instagibbs commented at 12:20 pm on March 26, 2024:

removed :)

in src/test/rbf_tests.cpp:399 in b380a1df36 outdated

395@@ -396,6 +396,14 @@ BOOST_FIXTURE_TEST_CASE(improves_feerate, TestChain100Setup)
396     // With one more satoshi it does
397     BOOST_CHECK(ImprovesFeerateDiagram(pool, {entry1}, {entry1, entry2}, tx1_fee + tx2_fee + 1, tx1_size + tx2_size) == std::nullopt);
398 
399+    // With prioritisation of in-mempool conflicts, it effects the results of the comparison using the same args as just above

glozow commented at 9:17 am on March 26, 2024:

b380a1df36a95c85310949aeb81d8c57f4d023c4: s/effects/affects

instagibbs commented at 12:09 pm on March 26, 2024:

RFC: merge the two words into “ffect”

instagibbs commented at 12:20 pm on March 26, 2024:

done

in src/test/rbf_tests.cpp:64 in 44ba5e9bea outdated

59+        tx1.vout[i].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
60+        tx1.vout[i].nValue = output_values[i];
61+    }
62+
63+    // Second tx takes second parent output
64+    CMutableTransaction tx2 = CMutableTransaction();

glozow commented at 9:19 am on March 26, 2024:

Could just make a copy of tx1 and change the input prevout? Can remove all the duplicate code.

instagibbs commented at 12:20 pm on March 26, 2024:

done

in src/test/rbf_tests.cpp:40 in 44ba5e9bea outdated

36@@ -37,6 +37,47 @@ static inline CTransactionRef make_tx(const std::vector<CTransactionRef>& inputs
37     return MakeTransactionRef(tx);
38 }
39 
40+// Make two sibling transactions

glozow commented at 9:21 am on March 26, 2024:

0// Make two child transactions from parent (which must have at least 2 outputs).
1// Each tx will have the same outputs, using the amounts specified in output_values.

instagibbs commented at 12:20 pm on March 26, 2024:

sure. taken

in src/test/rbf_tests.cpp:122 in 44ba5e9bea outdated

117+    TestMemPoolEntryHelper entry;
118+    // Assumes this isn't already spent in mempool
119+    auto children_tx = make_two_siblings(/*parent=*/parent, /*output_values=*/{50 * CENT});
120+    pool.addUnchecked(entry.FromTx(children_tx.first));
121+    pool.addUnchecked(entry.FromTx(children_tx.second));
122+    // Return last created tx

glozow commented at 9:22 am on March 26, 2024:

Returns both, actually?

instagibbs commented at 12:20 pm on March 26, 2024:

copy/paste, removed

in src/test/fuzz/rbf.cpp:189 in 4f910a6944 outdated

185+        // Diagram check succeeded
186+        if (!err_tuple.has_value()) {
187+            // New diagram's final fee should always match or exceed old diagram's
188+            assert(calc_results->first.back().fee <= calc_results->second.back().fee);
189+        } else if (calc_results->first.back().fee > calc_results->second.back().fee) {
190+            // Or it failed, and if old diagram had higher fees, it should be a straight

glozow commented at 9:26 am on March 26, 2024:

4f910a69448203fc51abd993bef27bf00768a1ec “a straight”?

instagibbs commented at 12:20 pm on March 26, 2024:

…. failure, fixed

glozow commented at 2:18 pm on March 26, 2024:

glozow commented at 9:27 am on March 26, 2024: member

LGTM, thanks for taking my test nits. I haven’t run the package_rbf fuzzer with changes yet. Will review again after rebase.

in src/util/feefrac.h:76 in 252af7bd6d outdated

71+
72+    inline FeeFrac(const FeeFrac&) noexcept = default;
73+    inline FeeFrac& operator=(const FeeFrac&) noexcept = default;
74+
75+    /** Check if this is empty (size and fee are 0). */
76+    bool inline IsEmpty() const noexcept {

maflcko commented at 11:00 am on March 26, 2024:

style nit (feel free to ignore): Member functions are already inline, so could drop the inline everywhere. See https://en.cppreference.com/w/cpp/language/inline

A function defined entirely inside a class/struct/union definition, whether it’s a member function or a non-member friend function, is implicitly an inline function …

doc: fix comment about non-existing CompareFeeFrac bb42402945

ImprovesFeerateDiagram: Spelling fix and removal of unused diagram vectors b62e2c0fa5

unit test: have CompareFeerateDiagram tested with diagrams both ways c0c37f07eb

unit test: check tx4 conflict error message 69bd18ca80

unit test: add CheckConflictTopology case for not the only child a80d80936a

unit test: add coverage showing priority affects diagram check results 216d5ff162

fuzz: add PrioritiseTransaction coverage in diagram checks defe023f6e

unit test: make calc_feerate_diagram_rbf less brittle d2bf923eb1

unit test: improve ImprovesFeerateDiagram coverage with one less vb case c377ae9ba0

fuzz: finer grained ImprovesFeerateDiagram check on error result 2a3ada8b21

fuzz: Add more invariant checks for package_rbf b684d82d7e

CalculateFeerateDiagramsForRBF: remove size tie-breaking from chunking conflicts d9391ec095

s/effected/affected/ 890cb015f3

unit test: clarify unstated assumption for calc_feerate_diagram_rbf chunking a0376e1061

remove erroneous CompareFeerateDiagram comment about slope cebcced65e

instagibbs force-pushed on Mar 26, 2024

instagibbs commented at 12:21 pm on March 26, 2024: member

rebased on master, ready for more review

CompareFeerateDiagram: short-circuit comparison when detected as incomparable a9d42b9aa5

glozow commented at 2:21 pm on March 26, 2024: member

0src/txmempool.cpp:1298:    // all_conflicts.  every such transaction will either be at its own feerate (followed 
1src/txmempool.cpp:1299:    // by any descendant at its own feerate), or as a single chunk at the descendant's 
2
3^^^
4Trailing whitespace

CalculateFeerateDiagramsForRBF: update misleading description of old diagram contents ee1b9b231a

instagibbs force-pushed on Mar 26, 2024

DrahtBot added the label CI failed on Mar 26, 2024

DrahtBot commented at 3:42 pm on March 26, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23103026830

instagibbs commented at 8:21 pm on March 26, 2024: member

fixed, thanks

DrahtBot removed the label CI failed on Mar 26, 2024

glozow requested review from murchandamus on Mar 27, 2024

glozow commented at 3:29 pm on March 27, 2024: member

ACK ee1b9b231a0a7e89b77cbf8ea54e0534f0970dd0, reviewed the changes and package_rbf fuzzer seems to run fine

instagibbs commented at 6:26 pm on March 27, 2024: member

@sdaftuar @ismaelsadeeq @willcl-ark :pray:

murchandamus commented at 7:37 pm on March 27, 2024: contributor

crACK ee1b9b231a0a7e89b77cbf8ea54e0534f0970dd0

in src/policy/rbf.cpp:1 in b62e2c0fa5 outdated

ismaelsadeeq commented at 11:58 am on March 28, 2024:

nit: commit is not really a spelling mistake but grammar fix “ImprovesFeerateDiagram: doc fix and removal of unused diagram vectors”

in src/test/rbf_tests.cpp:101 in a80d80936a outdated

 97@@ -67,6 +98,20 @@ static CTransactionRef add_descendant_to_parents(const std::vector<CTransactionR
 98     return child_tx;
 99 }
100 
101+// Makes two children for a single parent

ismaelsadeeq commented at 11:59 am on March 28, 2024:

nit: code is self explanatory

in src/test/rbf_tests.cpp:102 in a80d80936a outdated

 97@@ -67,6 +98,20 @@ static CTransactionRef add_descendant_to_parents(const std::vector<CTransactionR
 98     return child_tx;
 99 }
100 
101+// Makes two children for a single parent
102+static std::pair<CTransactionRef, CTransactionRef> add_children_to_parent(const CTransactionRef parent, CTxMemPool& pool)

ismaelsadeeq commented at 12:00 pm on March 28, 2024:

nit:

0static std::pair<CTransactionRef, CTransactionRef> add_two_children_to_parent(const CTransactionRef parent, CTxMemPool& pool)

in src/test/rbf_tests.cpp:165 in a80d80936a outdated

160@@ -116,6 +161,10 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
161     const auto tx12 = make_tx(/*inputs=*/ {m_coinbase_txns[8]}, /*output_values=*/ {995 * CENT});
162     pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx12));
163 
164+    // Will make two children of this single parent
165+    const auto tx13 = make_tx(/*inputs=*/ {m_coinbase_txns[9]}, /*output_values=*/ {995 * CENT, 995 * CENT});

ismaelsadeeq commented at 12:07 pm on March 28, 2024:

style nit

0    const auto tx13 = make_tx(/*inputs=*/{m_coinbase_txns[9]}, /*output_values=*/{995 * CENT, 995 * CENT});

in src/test/rbf_tests.cpp:352 in a80d80936a outdated

347+    const auto two_siblings = add_children_to_parent(tx13, pool);
348+    const auto entry_sibling_1 = pool.GetIter(two_siblings.first->GetHash()).value();
349+    const auto entry_sibling_2 = pool.GetIter(two_siblings.second->GetHash()).value();
350+    BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry_sibling_1}).value(), strprintf("%s is not the only child of parent %s", entry_sibling_1->GetSharedTx()->GetHash().ToString(), entry13_unchained->GetSharedTx()->GetHash().ToString()));
351+    BOOST_CHECK_EQUAL(pool.CheckConflictTopology({entry_sibling_2}).value(), strprintf("%s is not the only child of parent %s", entry_sibling_2->GetSharedTx()->GetHash().ToString(), entry13_unchained->GetSharedTx()->GetHash().ToString()));
352+

ismaelsadeeq commented at 12:08 pm on March 28, 2024:

style nit: trailing line

ismaelsadeeq approved

ismaelsadeeq commented at 12:21 pm on March 28, 2024: member

Code review ACK ee1b9b231a0a7e89b77cbf8ea54e0534f0970dd0

I’ve tested this locally but havent run the fuzz test.

Feel free to ignore the review comments below, mostly style nits.

willcl-ark approved

willcl-ark commented at 11:28 am on March 29, 2024: contributor

ACK ee1b9b231a0a7e89b77cbf8ea54e0534f0970dd0

Amendments look good to me.

instagibbs commented at 6:50 pm on March 29, 2024: member

@ismaelsadeeq I’ll consider these if I have to touch again

achow101 merged this on Mar 29, 2024

achow101 closed this on Mar 29, 2024

29242 Diagram check followups #29724

Code Coverage

Reviews

Conflicts