document and clean up MaybeUpdateMempoolForReorg #23976

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:

        }  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

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

<details><summary>Show signature</summary>

Signature:

</details>

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