document and clean up MaybeUpdateMempoolForReorg

glozow commented at 4:10 pm on January 4, 2022: member

followup to #23683, addressing #23683 (comment)

glozow commented at 4:11 pm on January 4, 2022: member

cc @jnewbery, as requested here

MarcoFalke added the label Refactoring on Jan 4, 2022

DrahtBot commented at 6:28 am on January 5, 2022: 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:

#24080 (policy: Remove unused locktime flags by MarcoFalke)
#23897 (refactor: Separate lockpoint calculate logic from CheckSequenceLocks function by hebasto)

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.

jnewbery commented at 12:55 pm on January 5, 2022: member

ACK c633c41e3089c44a90f1f8c32a2b411889cefcdf

in src/validation.cpp:375 in c633c41e30 outdated

363+        if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
364+        // Note if CheckSequenceLocks fails the LockPoints may still be invalid
365+        // So it's critical that we remove the tx and not depend on the LockPoints.
366+        if (!CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {
367+            return true;
368+        } else if (!validLP) {

MarcoFalke commented at 1:12 pm on January 5, 2022:

0        }  if (!validLP) {

nit: no need for else. I find it confusing in early return statements.

jnewbery commented at 2:57 pm on January 5, 2022:

See rationale here: #23683 (review) , but you’re right that they’ve equivalent, so both are fine.

hebasto commented at 3:37 pm on January 5, 2022:

Why this branch–if (!validLP)–has been moved before if (it->GetSpendsCoinbase()) ?

jnewbery commented at 4:47 pm on January 7, 2022:

Because logically it’s connected to the previous if statement - if CheckSequenceLocks() returns false then this filter function fails immediately. If it returns true, then the lockpoints may be invalid so they should be updated.

Functionally this is equivalent, but it groups the related functionality together.

glozow commented at 10:52 am on January 17, 2022:

Clarified the comments so it should be more clear to a first-time reader why the branches are linked

hebasto commented at 3:15 pm on January 5, 2022: member

Related #23897.

hebasto commented at 3:37 pm on January 5, 2022: member

Concept ACK.

What about

 0--- a/src/validation.cpp
 1+++ b/src/validation.cpp
 2@@ -348,11 +348,11 @@ void CChainState::MaybeUpdateMempoolForReorg(
 3         AssertLockHeld(m_mempool->cs);
 4         AssertLockHeld(::cs_main);
 5         const CTransaction& tx = it->GetTx();
 6+        if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
 7         LockPoints lp = it->GetLockPoints();
 8         const bool validLP{TestLockPointValidity(m_chain, lp)};
 9         CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool);
10         // The transaction must be final.
11-        if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
12         // Note if CheckSequenceLocks fails the LockPoints may still be invalid
13         // So it's critical that we remove the tx and not depend on the LockPoints.
14         if (!CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {

?

in src/validation.cpp:400 in c633c41e30 outdated

384+        return false;
385     };
386 
387     // We also need to remove any now-immature transactions
388-    m_mempool->removeForReorg(m_chain, check_final_and_mature);
389+    m_mempool->removeForReorg(m_chain, filter_final_and_mature);

instagibbs commented at 1:56 am on January 10, 2022:

nit: this renaming isn’t reflected in the signature of removeForReorg.

glozow commented at 10:52 am on January 17, 2022:

Fixed

in src/validation.cpp:357 in c633c41e30 outdated

339@@ -340,21 +340,29 @@ void CChainState::MaybeUpdateMempoolForReorg(
340     // the disconnectpool that were added back and cleans up the mempool state.
341     m_mempool->UpdateTransactionsFromBlock(vHashUpdate);
342 
343-    const auto check_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it)
344+    // Check whether the transaction is still final and, if it spends a coinbase output, mature.
345+    // Update the entry's cached LockPoints if needed. If this returns true, the tx would be invalid
346+    // in the next block, so the mempool must remove this entry and all of its descendants.
347+    const auto filter_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it)

instagibbs commented at 2:00 am on January 10, 2022:

nit: this sounds like we’re getting rid of final/mature txs.

a little longer but filter_for_final_and_mature makes it less ambiguous to me

glozow commented at 10:54 am on January 17, 2022:

I’ve updated the comments to make it more clear that this is meant to be a predicate for filtering, so returning true for something we want to filter out is natural.

in src/validation.cpp:356 in c633c41e30 outdated

359-            // So it's critical that we remove the tx and not depend on the LockPoints.
360-            should_remove = true;
361-        } else if (it->GetSpendsCoinbase()) {
362+        // The transaction must be final.
363+        if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
364+        // Note if CheckSequenceLocks fails the LockPoints may still be invalid

instagibbs commented at 2:25 am on January 10, 2022:

while we’re here: I’m not sure what this comment block is saying.

glozow commented at 10:55 am on January 17, 2022:

Better now?

instagibbs commented at 11:21 am on January 17, 2022:

yes the comment you left actually matches my understanding of the code

glozow force-pushed on Jan 17, 2022

glozow commented at 10:52 am on January 17, 2022: member

Sorry for the delay! I’ve addressed @hebasto and @instagibbs comments and grouped in @hebasto’s commit from #23958.

instagibbs approved

instagibbs commented at 11:22 am on January 17, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/23976/commits/283936b10d2e218726648bc9cd640d56d6922bb2

hebasto commented at 2:51 am on January 18, 2022: member

It looks like some changes that belong to the second commit–a replacing update_lock_points with a lambda–go to the first commit accidentally, no?

document and clean up MaybeUpdateMempoolForReorg

Co-authored-by: John Newbery <john@johnnewbery.com>

c7cd98c717

Replace `struct update_lock_points` with lambda

No behavior change.
This code was introduced in 5add7a7 before we required C++11, which is
why the struct was needed. As we are now using more modern C++ and this
is the only place where lockpoints are updated for mempool entries, it
is more idiomatic to call `modify` with a lambda.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

e177fcab38

glozow force-pushed on Jan 18, 2022

glozow commented at 11:57 am on January 18, 2022: member

It looks like some changes that belong to the second commit–a replacing update_lock_points with a lambda–go to the first commit accidentally, no?

Oops! Good catch, fixed.

hebasto approved

hebasto commented at 12:28 pm on January 18, 2022: member

ACK e177fcab3831b6d259da5164cabedcc9e78f6957, I have reviewed the code and it looks OK, I agree it can be merged.

instagibbs approved

instagibbs commented at 10:40 pm on January 18, 2022: member

ACK e177fcab3831b6d259da5164cabedcc9e78f6957

fanquake requested review from jnewbery on Jan 19, 2022

in src/validation.cpp:390 in c7cd98c717 outdated

387@@ -372,18 +388,16 @@ void CChainState::MaybeUpdateMempoolForReorg(
388                 assert(!coin.IsSpent());
389                 const auto mempool_spend_height{m_chain.Tip()->nHeight + 1};
390                 if (coin.IsSpent() || (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY)) {

MarcoFalke commented at 1:44 pm on January 19, 2022:

nit: any reason to check for coin.IsSpent(), given the assertion?

MarcoFalke approved

MarcoFalke commented at 2:31 pm on January 19, 2022: member

Approach ACK e177fcab3831b6d259da5164cabedcc9e78f6957 😶

Did not review the last commit, but given the assert added in b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b it should be correct.

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Approach ACK e177fcab3831b6d259da5164cabedcc9e78f6957 😶
 4
 5Did not review the last commit, but given the assert added in b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b it should be correct.
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 9pUj/Ugv+N4lEh7A93jSO5imukN2scMEs6r7j7KNiTudrany2XPrSbTvJQgl+q46K
10j5UPWvQCzJ0M+NpenYE0Eiefl51eZGRo+4rT06GlQJ/jNyZkutbVmbbNvspM19gg
11lsnpx7pfaLR9MFAmIZQ3xz2y5zu2adewPtFrTAfKbxfkZbkR17+EayICGvXRked9
12u/UGf/F/XmA/Yopnfn4jAHpPUlvvgKFxOaRFI76302IDY6D9XgzTAg2cjLBlUuy9
133KCf5UGPJ2fVYFcpf4OZdsGi+vzo17+UGOK0dcYb/wLqfr2VH/mZPL5o/jo2l3d4
14EcGvDAzYRrM3tvRp9pFDO0O27avyvMw106RI/u1/TyqKX4apcblX954bAugIBXuN
15ZgAed34pqaHeRqm9R1WdY1O270pehbUtv+NKWu6awEZ+j79vwF8UVwOModkepwbd
16Fp0c+f4aEAzHLEEdpMR2l+XUrF97ZviaVNGXX06bfuIpSsw7MpR3K98SMmLxVyn9
17CVsQAgjV
18=YL6U
19-----END PGP SIGNATURE-----

MarcoFalke merged this on Jan 19, 2022

MarcoFalke closed this on Jan 19, 2022

sidhujag referenced this in commit 41e3b664aa on Jan 20, 2022

MarcoFalke referenced this in commit e3de7cb903 on Jan 24, 2022

sidhujag referenced this in commit 5d6fa560dc on Jan 28, 2022

Fabcien referenced this in commit 597f5af02b on Nov 24, 2022

Fabcien referenced this in commit 5cf1f28e56 on Nov 24, 2022

DrahtBot locked this on Jan 19, 2023

document and clean up MaybeUpdateMempoolForReorg #23976

Conflicts