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

hebasto · 2021-12-29T17:58:15Z

This PR is follow up for #22677 and #23683.

On master (013daed) it is not obvious that CheckSequenceLocksAtTip() function can modify its LockPoints* lp parameter which leads to #22677 (comment).

This PR:

separates the lockpoint calculate logic from CheckSequenceLocksAtTip() function into a new CalculateLockPointsAtTip() one
cleans up the CheckSequenceLocksAtTip() function interface
makes code easier to reason about (hopefully)

hebasto · 2022-01-03T14:47:31Z

Rebased 154307c -> c4476a5 (pr23897.01 -> pr23897.02) on top of the merged #23683.

DrahtBot · 2022-01-03T15:03:47Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, achow101
Stale ACK	glozow, aureleoules

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

Conflicts

No conflicts as of last run.

hebasto · 2022-01-19T21:53:53Z

Rebased c4476a5 -> 220475b (pr23897.02 -> pr23897.03) on top of the merged #23976.

glozow

Concept ACK, I agree that calculating lockpoints and checking them against the current chain should be separate methods and it was definitely unexpected that CheckSequenceLocks() was mutating the lockpoints passed in. There is a slight behavior change to MaybeUpdateMempoolForReorg() here.

src/validation.cpp

hebasto · 2022-04-19T16:06:16Z

Updated 220475b -> aa8f68b (pr23897.03 -> pr23897.04):

rebased
addressed @glozow's comments

Also the PR description has been updated.

hebasto · 2022-05-28T12:35:38Z

Rebased aa8f68b -> 5a6ce8c (pr23897.04 -> pr23897.05) due to conflict with #24804.

aureleoules

ACK bebaddb
The code is now much clearer and easier to understand and the behavior doesn't seem to have changed.

src/validation.cpp

hebasto · 2022-10-14T12:06:08Z

Updated bebaddb -> ed2d714 (pr23897.08 -> pr23897.09, diff):

addressed @aureleoules's comments

aureleoules

reACK ed2d714

stickies-v

Concept ACK. Hooray for clearer function intents and effects!

src/validation.cpp

src/validation.h

stickies-v

Approach ACK ed2d714

I think this is already a great step in the right direction, but imo there are a bunch more refactoring maintainability improvements in the touched functions that we can make. Quite a few of my comments are suggestions in that direction - I realize some may be out of scope. Even though I'd like to see them incorporated, I'm not very fussed about necessarily doing them in this PR - I'm happy working on a follow-up too.

stickies-v · 2022-11-04T15:29:53Z

src/validation.cpp

+    // When SequenceLocks() is called within ConnectBlock(), the height
+    // of the block *being* evaluated is what is used.


This documentation seems out of date now

@hebasto I believe my comment was aimed at that I couldn't find any connection between SequenceLocks() and ConnectBlock() and this function - but maybe I just missed it? I also don't really understand what the documentation means, but maybe that's just my lack of context.

I couldn't find any connection between SequenceLocks() and ConnectBlock() and this function - but maybe I just missed it?

bitcoin/src/validation.cpp

Line 2257 in 357d750

if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) {

src/validation.cpp

stickies-v · 2022-11-10T11:55:15Z

src/validation.cpp

+        const CTxIn& txin = tx.vin[txinIndex];
+        Coin coin;
+        if (!coins_view.GetCoin(txin.prevout, coin)) {
+            LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, txinIndex, tx.GetHash().GetHex());


Perhaps you could Assume(false) here to ensure this is visible in debug builds? I don't think we ever expect this path to be reached?

This change could be considered as a behavior change, so leaving it for now.

stickies-v · 2022-11-10T12:44:42Z

src/validation.cpp

@@ -334,20 +336,23 @@ void Chainstate::MaybeUpdateMempoolForReorg(

        // The transaction must be final.


While refactoring this function, would it be appropriate to eliminate this behemoth filter_final_and_mature lambda and turn it into a member function? It's a very straightforward carve-out and I think that massively helps with readability. Example very quick diff below (some lock warnings but compiles otherwise). In addition to carve-out, it also flips the true/false behaviour for an (imo) more intuitive behaviour and naming and thus readability.

git diff

diff --git a/src/validation.cpp b/src/validation.cpp index 0986147b7..1657323bd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -323,60 +323,60 @@ void Chainstate::MaybeUpdateMempoolForReorg( // the disconnectpool that were added back and cleans up the mempool state. m_mempool->UpdateTransactionsFromBlock(vHashUpdate); - // Predicate to use for filtering transactions in removeForReorg. - // Checks whether the transaction is still final and, if it spends a coinbase output, mature. - // Also updates valid entries' cached LockPoints if needed. - // If false, the tx is still valid and its lockpoints are updated. - // If true, the tx would be invalid in the next block; remove this entry and all of its descendants. - const auto filter_final_and_mature = [this](CTxMemPool::txiter it) - EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) { - AssertLockHeld(m_mempool->cs); - AssertLockHeld(::cs_main); - const CTransaction& tx = it->GetTx(); + // We also need to remove any now-immature transactions + m_mempool->removeForReorg(m_chain, [this](CTxMemPool::txiter it){ return !this->IsTxFinalAndMature(it); }); + // Re-limit mempool size, in case we added any transactions + LimitMempoolSize(*m_mempool, this->CoinsTip()); +} - // The transaction must be final. - if (!CheckFinalTxAtTip(*Assert(m_chain.Tip()), tx)) return true; +// Predicate to use for filtering transactions in removeForReorg. +// Checks whether the transaction is still final and, if it spends a coinbase output, mature. +// Also updates valid entries' cached LockPoints if needed. +// If true, the tx is still valid and its lockpoints are updated. +// If false, the tx would be invalid in the next block; remove this entry and all of its descendants. +bool Chainstate::IsTxFinalAndMature(CTxMemPool::txiter it) EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) +{ + AssertLockHeld(m_mempool->cs); + AssertLockHeld(::cs_main); + const CTransaction& tx = it->GetTx(); - const LockPoints& lp = it->GetLockPoints(); - // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be - // created on top of the new chain. - if (TestLockPointValidity(m_chain, lp)) { - if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) { - return true; - } + // The transaction must be final. + if (!CheckFinalTxAtTip(*Assert(m_chain.Tip()), tx)) return true; + + const LockPoints& lp = it->GetLockPoints(); + // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be + // created on top of the new chain. + if (TestLockPointValidity(m_chain, lp)) { + if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) { + return false; + } + } else { + const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool}; + const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)}; + if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) { + // Now update the mempool entry lockpoints as well. + m_mempool->mapTx.modify(it, [&new_lock_points](CTxMemPoolEntry& e) { e.UpdateLockPoints(*new_lock_points); }); } else { - const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool}; - const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)}; - if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) { - // Now update the mempool entry lockpoints as well. - m_mempool->mapTx.modify(it, [&new_lock_points](CTxMemPoolEntry& e) { e.UpdateLockPoints(*new_lock_points); }); - } else { - return true; - } + return false; } + } - // If the transaction spends any coinbase outputs, it must be mature. - if (it->GetSpendsCoinbase()) { - for (const CTxIn& txin : tx.vin) { - auto it2 = m_mempool->mapTx.find(txin.prevout.hash); - if (it2 != m_mempool->mapTx.end()) - continue; - const Coin& coin{CoinsTip().AccessCoin(txin.prevout)}; - assert(!coin.IsSpent()); - const auto mempool_spend_height{m_chain.Tip()->nHeight + 1}; - if (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY) { - return true; - } + // If the transaction spends any coinbase outputs, it must be mature. + if (it->GetSpendsCoinbase()) { + for (const CTxIn& txin : tx.vin) { + auto it2 = m_mempool->mapTx.find(txin.prevout.hash); + if (it2 != m_mempool->mapTx.end()) + continue; + const Coin& coin{CoinsTip().AccessCoin(txin.prevout)}; + assert(!coin.IsSpent()); + const auto mempool_spend_height{m_chain.Tip()->nHeight + 1}; + if (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY) { + return false; } } - // Transaction is still valid and cached LockPoints are updated. - return false; - }; - - // We also need to remove any now-immature transactions - m_mempool->removeForReorg(m_chain, filter_final_and_mature); - // Re-limit mempool size, in case we added any transactions - LimitMempoolSize(*m_mempool, this->CoinsTip()); + } + // Transaction is still valid and cached LockPoints are updated. + return true; } /** diff --git a/src/validation.h b/src/validation.h index bd6f710d1..52f1ee66f 100644 --- a/src/validation.h +++ b/src/validation.h @@ -291,6 +291,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip, * passed in for evaluation. * The LockPoints should not be considered valid if CheckSequenceLocksAtTip returns false. */ + bool CheckSequenceLocksAtTip(CBlockIndex* tip, const LockPoints& lp); @@ -770,6 +771,8 @@ private: DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs); + bool IsTxFinalAndMature(CTxMemPool::txiter it) EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main); + /** Check warning conditions and do some notifications on new chain tip set. */ void UpdateTip(const CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

Going to leave it as this to keep this PR in more reviewable state.

src/validation.cpp

src/validation.h

stickies-v · 2022-11-10T16:51:24Z

src/validation.cpp

+    int max_input_height{0};
+    for (const int height : prevheights) {
+        // Can ignore mempool inputs since we'll fail if they had non-zero locks
+        if (height != index.nHeight) {
+            max_input_height = std::max(max_input_height, height);
+        }
+    }


Alternative approach using replace_if and max_element

Suggested change

int max_input_height{0};

for (const int height : prevheights) {

// Can ignore mempool inputs since we'll fail if they had non-zero locks

if (height != index.nHeight) {

max_input_height = std::max(max_input_height, height);

}

}

// Set heights of mempool inputs to 0 since we'll fail if they had non-zero locks

std::replace_if(prevheights.begin(), prevheights.end(), [&index](const int height){ return height == index.nHeight; }, 0);

const auto max_input_height{*std::max_element(prevheights.begin(), prevheights.end())};

Going to leave it as this to keep this PR in more reviewable state.

hebasto · 2022-11-29T16:55:51Z

Updated ed2d714 -> 1636bc4 (pr23897.09 -> pr23897.10, diff):

addressed some of @stickies-v's comments

src/validation.cpp

stickies-v · 2022-12-02T12:57:50Z

src/validation.cpp

+            LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
+            return std::nullopt;


util::Result would be a good candidate here too, so you can propagate the error message back

Why? The error message is being logged, and it is not exposed to a user in any other way.

bitcoin/src/util/result.h

Lines 22 to 26 in cac29f5

//! It is intended for high-level functions that need to report error strings to

//! end users. Lower-level functions that don't need this error-reporting and

//! only need error-handling should avoid util::Result and instead use standard

//! classes like std::optional, std::variant, and std::tuple, or custom structs

//! and enum types to return function results.

If I missed your point, what are benefits of propagating the error message back?

Same reason we want to let the callsite decide how to handle what to do when the calculation failed: with util::Result, the callsite can also decide if they want to log the error or not. In this case, there is only one callsite so it doesn't matter much. Can always refactor later on if necessary. Happy to mark as resolved, just a thought i had.

hebasto · 2022-12-03T16:16:29Z

Updated 1636bc4 -> 63d6b9a (pr23897.10 -> pr23897.11):

rebased to include refactor: Nuke policy/fees->mempool circular dependencies #17786.
addressed more @stickies-v's comments

stickies-v

ACK 68c0f16

A couple of improvements for follow-up that I think would further benefit the readability of this code, but I agree with @hebasto's assessment that they may reduce the reviewability of this PR and they are not blocking imo (so comments can be marked resolved, keeping track here):

src/validation.h

hebasto · 2023-01-31T13:35:47Z

Updated 68c0f16 -> 75db62b (pr23897.12 -> pr23897.13, diff).

Addressed some of @stickies-v's comments:

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

stickies-v

re-ACK 75db62b

achow101 · 2023-02-28T15:35:29Z

ACK 75db62b

glozow

code LGTM

…Sequenc…

DrahtBot added Mempool Refactoring Validation labels Dec 29, 2021

hebasto force-pushed the 211229-lockpoints branch from 81841f0 to 154307c Compare December 29, 2021 18:09

hebasto marked this pull request as ready for review December 29, 2021 19:27

hebasto mentioned this pull request Dec 29, 2021

cut the validation <-> txmempool circular dependency 2/2 #22677

Merged

DrahtBot mentioned this pull request Jan 3, 2022

refactor: Replace struct update_lock_points with lambda #23958

Closed

hebasto force-pushed the 211229-lockpoints branch from 154307c to c4476a5 Compare January 3, 2022 14:44

This was referenced Jan 5, 2022

document and clean up MaybeUpdateMempoolForReorg #23976

Merged

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

Closed

DrahtBot mentioned this pull request Jan 16, 2022

policy: Remove unused locktime flags #24080

Merged

DrahtBot added the Needs rebase label Jan 19, 2022

hebasto force-pushed the 211229-lockpoints branch from c4476a5 to 220475b Compare January 19, 2022 21:53

DrahtBot removed the Needs rebase label Jan 19, 2022

glozow reviewed Jan 26, 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

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Mar 14, 2022

hebasto force-pushed the 211229-lockpoints branch from 220475b to aa8f68b Compare April 19, 2022 15:59

hebasto changed the title ~~refactor: Separate lockpoint calculate logic from CheckSequenceLocks function~~ refactor: Move calculation logic out from CheckSequenceLocksAtTip() Apr 19, 2022

DrahtBot removed the Needs rebase label Apr 19, 2022

This was referenced Apr 19, 2022

Sanity assert GetAncestor() != nullptr where appropriate #24804

Merged

Enforce BIP 68 from genesis #24567

Closed

DrahtBot mentioned this pull request Apr 29, 2022

refactor: Remove unused CTxMemPool::clear() helper #19909

Merged

DrahtBot added the Needs rebase label May 6, 2022

hebasto force-pushed the 211229-lockpoints branch from aa8f68b to 5a6ce8c Compare May 28, 2022 12:33

DrahtBot removed the Needs rebase label May 28, 2022

aureleoules approved these changes Oct 11, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 211229-lockpoints branch from bebaddb to ed2d714 Compare October 14, 2022 12:03

aureleoules approved these changes Oct 14, 2022

View reviewed changes

stickies-v reviewed Nov 4, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

stickies-v reviewed Nov 10, 2022

View reviewed changes

hebasto force-pushed the 211229-lockpoints branch from ed2d714 to 1636bc4 Compare November 29, 2022 16:52

stickies-v reviewed Dec 2, 2022

View reviewed changes

hebasto force-pushed the 211229-lockpoints branch from 1636bc4 to 63d6b9a Compare December 3, 2022 16:13

hebasto marked this pull request as draft December 3, 2022 16:45

hebasto force-pushed the 211229-lockpoints branch 2 times, most recently from 4e83b98 to 68c0f16 Compare December 3, 2022 17:18

hebasto marked this pull request as ready for review December 3, 2022 17:30

stickies-v approved these changes Jan 18, 2023

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

hebasto added 2 commits January 31, 2023 13:26

refactor: Add CalculateLockPointsAtTip() function

3bc434f

refactor: Move calculation logic out from CheckSequenceLocksAtTip()

75db62b

hebasto force-pushed the 211229-lockpoints branch from 68c0f16 to 75db62b Compare January 31, 2023 13:36

stickies-v approved these changes Jan 31, 2023

View reviewed changes

achow101 requested a review from glozow February 28, 2023 15:35

DrahtBot requested a review from aureleoules February 28, 2023 15:35

glozow reviewed Feb 28, 2023

View reviewed changes

DrahtBot requested a review from glozow February 28, 2023 16:48

glozow merged commit a8080c0 into bitcoin:master Feb 28, 2023

hebasto deleted the 211229-lockpoints branch February 28, 2023 16:54

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 1, 2023

Merge bitcoin#23897: refactor: Move calculation logic out from `Check…

8978946

…Sequenc…

bitcoin locked and limited conversation to collaborators Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented Dec 29, 2021 •

edited

Loading

hebasto commented Jan 3, 2022

DrahtBot commented Jan 3, 2022 •

edited

Loading

hebasto commented Jan 19, 2022

glozow left a comment

hebasto commented Apr 19, 2022

hebasto commented May 28, 2022

aureleoules left a comment

hebasto commented Oct 14, 2022

aureleoules left a comment

stickies-v left a comment

stickies-v left a comment •

edited

Loading

stickies-v Nov 4, 2022

stickies-v Nov 29, 2022

hebasto Jan 31, 2023

stickies-v Nov 10, 2022

hebasto Nov 29, 2022

stickies-v Nov 10, 2022

hebasto Nov 29, 2022

stickies-v Nov 10, 2022

hebasto Nov 29, 2022

hebasto commented Nov 29, 2022

stickies-v Dec 2, 2022

hebasto Dec 3, 2022

stickies-v Jan 17, 2023

hebasto commented Dec 3, 2022

stickies-v left a comment

hebasto commented Jan 31, 2023

stickies-v left a comment

achow101 commented Feb 28, 2023

glozow left a comment

		// When SequenceLocks() is called within ConnectBlock(), the height
		// of the block being evaluated is what is used.

		@@ -334,20 +336,23 @@ void Chainstate::MaybeUpdateMempoolForReorg(

		// The transaction must be final.

		LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
		return std::nullopt;

	//! It is intended for high-level functions that need to report error strings to
	//! end users. Lower-level functions that don't need this error-reporting and
	//! only need error-handling should avoid util::Result and instead use standard
	//! classes like std::optional, std::variant, and std::tuple, or custom structs
	//! and enum types to return function results.

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

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

Conversation

hebasto commented Dec 29, 2021 • edited Loading

hebasto commented Jan 3, 2022

DrahtBot commented Jan 3, 2022 • edited Loading

Reviews

Conflicts

hebasto commented Jan 19, 2022

glozow left a comment

Choose a reason for hiding this comment

hebasto commented Apr 19, 2022

hebasto commented May 28, 2022

aureleoules left a comment

Choose a reason for hiding this comment

hebasto commented Oct 14, 2022

aureleoules left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Dec 3, 2022

stickies-v left a comment

Choose a reason for hiding this comment

hebasto commented Jan 31, 2023

stickies-v left a comment

Choose a reason for hiding this comment

achow101 commented Feb 28, 2023

glozow left a comment

Choose a reason for hiding this comment

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

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

hebasto commented Dec 29, 2021 •

edited

Loading

DrahtBot commented Jan 3, 2022 •

edited

Loading

stickies-v left a comment •

edited

Loading