document and clean up MaybeUpdateMempoolForReorg by glozow · Pull Request #23976 · bitcoin/bitcoin

glozow · 2022-01-04T16:10:59Z

followup to #23683, addressing #23683 (comment)

glozow · 2022-01-04T16:11:12Z

cc @jnewbery, as requested here

DrahtBot · 2022-01-05T06:28:32Z

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 · 2022-01-05T12:55:08Z

ACK c633c41e3089c44a90f1f8c32a2b411889cefcdf

src/validation.cpp

hebasto · 2022-01-05T15:15:33Z

Related #23897.

hebasto

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)) {

?

src/validation.cpp

glozow · 2022-01-17T10:52:18Z

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

instagibbs

ACK 283936b

hebasto

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?

Co-authored-by: John Newbery <[email protected]>

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 <[email protected]>

glozow · 2022-01-18T11:57:02Z

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

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

instagibbs

ACK e177fca

maflcko · 2022-01-19T13:44:54Z

src/validation.cpp

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


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

maflcko

Approach ACK e177fca 😶

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

Show signature

Signature:

… row fa2bcc4 Run coin.IsSpent only once in a row (MarcoFalke) Pull request description: Follow-up to commit 64e4963 and bitcoin/bitcoin#23976 (comment) ACKs for top commit: glozow: utACK fa2bcc4, agree the assertion is sufficient theStack: Code-review ACK fa2bcc4 w0xlt: crACK bitcoin/bitcoin@fa2bcc4 shaavan: Code Review ACK fa2bcc4 brunoerg: crACK fa2bcc4 Tree-SHA512: 3be9d6b313bf6bb835f031826c81777b4659118d839001d084e72462391cb64ba81d06a5e07fd21fcfb709a71b08892b23212a98604ce8481da489476b72f072

fa2bcc4 Run coin.IsSpent only once in a row (MarcoFalke) Pull request description: Follow-up to commit 64e4963 and bitcoin#23976 (comment) ACKs for top commit: glozow: utACK fa2bcc4, agree the assertion is sufficient theStack: Code-review ACK fa2bcc4 w0xlt: crACK bitcoin@fa2bcc4 shaavan: Code Review ACK fa2bcc4 brunoerg: crACK fa2bcc4 Tree-SHA512: 3be9d6b313bf6bb835f031826c81777b4659118d839001d084e72462391cb64ba81d06a5e07fd21fcfb709a71b08892b23212a98604ce8481da489476b72f072

Summary: Co-authored-by: John Newbery <[email protected]> This is a partial backport of [[bitcoin/bitcoin#23976 | core#23976]] bitcoin/bitcoin@c7cd98c Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12587

Summary: 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 <[email protected]> This concludes backport of [[bitcoin/bitcoin#23976 | core#23976]] bitcoin/bitcoin@e177fca Depends on D12587 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12588

maflcko added the Refactoring label Jan 4, 2022

This was referenced Jan 5, 2022

refactor: Replace struct update_lock_points with lambda #23958

Closed

refactor: Move calculation logic out from CheckSequenceLocksAtTip() #23897

Merged

maflcko reviewed Jan 5, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

hebasto reviewed Jan 5, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Jan 10, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

This was referenced Jan 11, 2022

wallet: Use MTP for locktime checks, not adjusted time #24038

Closed

policy: Remove unused locktime flags #24080

Merged

glozow force-pushed the 2022-01-lp branch from c633c41 to 283936b Compare January 17, 2022 10:50

instagibbs approved these changes Jan 17, 2022

View reviewed changes

hebasto reviewed Jan 18, 2022

View reviewed changes

glozow and others added 2 commits January 18, 2022 11:55

document and clean up MaybeUpdateMempoolForReorg

c7cd98c

Co-authored-by: John Newbery <[email protected]>

glozow force-pushed the 2022-01-lp branch from 283936b to e177fca Compare January 18, 2022 11:56

hebasto approved these changes Jan 18, 2022

View reviewed changes

instagibbs approved these changes Jan 18, 2022

View reviewed changes

fanquake requested a review from jnewbery January 19, 2022 12:47

maflcko approved these changes Jan 19, 2022

View reviewed changes

maflcko merged commit 06b6369 into bitcoin:master Jan 19, 2022

maflcko mentioned this pull request Jan 19, 2022

mempool: Run coin.IsSpent only once in a row #24102

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 20, 2022

Merge bitcoin#23976: document and clean up MaybeUpdateMempoolForReorg

41e3b66

bitcoin locked and limited conversation to collaborators Jan 19, 2023

Conversation

glozow commented Jan 4, 2022

Uh oh!

glozow commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Jan 5, 2022

Uh oh!

Uh oh!

hebasto commented Jan 5, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Jan 17, 2022

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Jan 18, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

glozow commented Jan 4, 2022 •

edited

Loading

DrahtBot commented Jan 5, 2022 •

edited

Loading