29242 Diagram check followups #29724

instagibbs · 2024-03-25T16:13:15Z

Follow-ups to #29242

DrahtBot · 2024-03-25T16:13:17Z

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.

DrahtBot · 2024-03-25T18:04:04Z

🚧 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}

glozow

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

src/txmempool.cpp

src/test/rbf_tests.cpp

src/test/fuzz/rbf.cpp

maflcko · 2024-03-26T11:00:47Z

src/util/feefrac.h

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


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

…ctors

…g conflicts

…hunking

instagibbs · 2024-03-26T12:21:07Z

rebased on master, ready for more review

…mparable

glozow · 2024-03-26T14:21:45Z

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

^^^
Trailing whitespace

…diagram contents

DrahtBot · 2024-03-26T15:42:55Z

🚧 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 · 2024-03-26T20:21:13Z

fixed, thanks

glozow

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

instagibbs · 2024-03-27T18:26:56Z

@sdaftuar @ismaelsadeeq @willcl-ark 🙏

murchandamus

crACK ee1b9b2

ismaelsadeeq

Code review ACK ee1b9b2

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

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

ismaelsadeeq · 2024-03-28T11:58:11Z

src/policy/rbf.cpp

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

ismaelsadeeq · 2024-03-28T11:59:56Z

src/test/rbf_tests.cpp

@@ -67,6 +98,20 @@ static CTransactionRef add_descendant_to_parents(const std::vector<CTransactionR
    return child_tx;
 }

+// Makes two children for a single parent


nit: code is self explanatory

Suggested change

// Makes two children for a single parent

ismaelsadeeq · 2024-03-28T12:00:49Z

src/test/rbf_tests.cpp

@@ -67,6 +98,20 @@ static CTransactionRef add_descendant_to_parents(const std::vector<CTransactionR
    return child_tx;
 }

+// Makes two children for a single parent
+static std::pair<CTransactionRef, CTransactionRef> add_children_to_parent(const CTransactionRef parent, CTxMemPool& pool)


nit:

Suggested change

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

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

ismaelsadeeq · 2024-03-28T12:07:42Z

src/test/rbf_tests.cpp

@@ -116,6 +161,10 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup)
    const auto tx12 = make_tx(/*inputs=*/ {m_coinbase_txns[8]}, /*output_values=*/ {995 * CENT});
    pool.addUnchecked(entry.Fee(normal_fee).FromTx(tx12));

+    // Will make two children of this single parent
+    const auto tx13 = make_tx(/*inputs=*/ {m_coinbase_txns[9]}, /*output_values=*/ {995 * CENT, 995 * CENT});


style nit

Suggested change

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

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

ismaelsadeeq · 2024-03-28T12:08:23Z

src/test/rbf_tests.cpp

+    const auto entry_sibling_2 = pool.GetIter(two_siblings.second->GetHash()).value();
+    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()));
+    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()));
+


style nit: trailing line

Suggested change

willcl-ark

ACK ee1b9b2

Amendments look good to me.

instagibbs · 2024-03-29T18:50:24Z

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

instagibbs force-pushed the 2024-03-diagram-followups branch from 4cfb540 to 252af7b Compare March 25, 2024 18:03

DrahtBot added the CI failed label Mar 25, 2024

DrahtBot removed the CI failed label Mar 25, 2024

glozow mentioned this pull request Mar 26, 2024

Package Relay Project Tracking #27463

Open

42 tasks

glozow reviewed Mar 26, 2024

View reviewed changes

maflcko reviewed Mar 26, 2024

View reviewed changes

instagibbs added 15 commits March 26, 2024 08:05

doc: fix comment about non-existing CompareFeeFrac

bb42402

ImprovesFeerateDiagram: Spelling fix and removal of unused diagram ve…

b62e2c0

…ctors

unit test: have CompareFeerateDiagram tested with diagrams both ways

c0c37f0

unit test: check tx4 conflict error message

69bd18c

unit test: add CheckConflictTopology case for not the only child

a80d809

unit test: add coverage showing priority affects diagram check results

216d5ff

fuzz: add PrioritiseTransaction coverage in diagram checks

defe023

unit test: make calc_feerate_diagram_rbf less brittle

d2bf923

unit test: improve ImprovesFeerateDiagram coverage with one less vb case

c377ae9

fuzz: finer grained ImprovesFeerateDiagram check on error result

2a3ada8

fuzz: Add more invariant checks for package_rbf

b684d82

CalculateFeerateDiagramsForRBF: remove size tie-breaking from chunkin…

d9391ec

…g conflicts

s/effected/affected/

890cb01

unit test: clarify unstated assumption for calc_feerate_diagram_rbf c…

a0376e1

…hunking

remove erroneous CompareFeerateDiagram comment about slope

cebcced

instagibbs force-pushed the 2024-03-diagram-followups branch from 252af7b to cebcced Compare March 26, 2024 12:20

CompareFeerateDiagram: short-circuit comparison when detected as inco…

a9d42b9

…mparable

CalculateFeerateDiagramsForRBF: update misleading description of old …

ee1b9b2

…diagram contents

instagibbs force-pushed the 2024-03-diagram-followups branch from de3af5f to ee1b9b2 Compare March 26, 2024 15:42

DrahtBot added the CI failed label Mar 26, 2024

DrahtBot removed the CI failed label Mar 26, 2024

glozow requested a review from murchandamus March 27, 2024 15:28

glozow reviewed Mar 27, 2024

View reviewed changes

murchandamus reviewed Mar 27, 2024

View reviewed changes

ismaelsadeeq approved these changes Mar 28, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 28, 2024

feefrac: avoid explicitly computing diagram; compare based on chunks #29757

Merged

willcl-ark approved these changes Mar 29, 2024

View reviewed changes

achow101 merged commit 61de64d into bitcoin:master Mar 29, 2024

bitcoin locked and limited conversation to collaborators Mar 29, 2025

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

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

29242 Diagram check followups #29724

29242 Diagram check followups #29724

Uh oh!

Conversation

instagibbs commented Mar 25, 2024

Uh oh!

DrahtBot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Mar 25, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 26, 2024

Uh oh!

glozow commented Mar 26, 2024

Uh oh!

DrahtBot commented Mar 26, 2024

Uh oh!

instagibbs commented Mar 26, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 27, 2024

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Mar 29, 2024

Uh oh!

Uh oh!

DrahtBot commented Mar 25, 2024 •

edited

Loading