CheckSequenceLocksAtTip()
#23897
This PR is follow up for bitcoin/bitcoin#22677 and bitcoin/bitcoin#23683.
On master (013daed9acca1b723f599d63ab36b9c2a5c60e5f) it is not obvious that CheckSequenceLocksAtTip() function can modify its LockPoints* lp parameter which leads to #22677 (review).
This PR:
CheckSequenceLocksAtTip() function into a new CalculateLockPointsAtTip() oneCheckSequenceLocksAtTip() function interface
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
No conflicts as of last run.
210@@ -211,14 +211,75 @@ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, i
211 return IsFinalTx(tx, nBlockHeight, nBlockTime);
212 }
213
214+bool CalculateLockPoints(
215+ CBlockIndex* tip,
216+ const CCoinsView& coins_view,
217+ const CTransaction& tx,
218+ int flags,
219+ LockPoints* lp)
CalculateLockPoints() return a std::optional<LockPoints> which encodes the same information as the bool.
235+ prevheights.resize(tx.vin.size());
236+ for (size_t txinIndex = 0; txinIndex < tx.vin.size(); ++txinIndex) {
237+ const CTxIn& txin = tx.vin[txinIndex];
238+ Coin coin;
239+ if (!coins_view.GetCoin(txin.prevout, coin)) {
240+ return error("%s: Missing input", __func__);
LogPrintf and return std::nullopt. Also seems like this log could be improved by adding the transaction id and/or prevout that is missing.
279- const CCoinsView& coins_view,
280- const CTransaction& tx,
281- int flags,
282- LockPoints* lp,
283- bool useExistingLockPoints)
284+ const LockPoints* lp)
const LockPoints& lp. It doesn’t make sense to allow a nullptr lp to be passed in anymore, since we can’t check sequence locks on empty lockpoints anyway.
385- // If CheckSequenceLocks fails, remove the tx and don't depend on the LockPoints.
386- return true;
387- } else if (!validLP) {
388- // If CheckSequenceLocks succeeded, it also updated the LockPoints.
389- // Now update the mempool entry lockpoints as well.
390+ if (!validLP) {
Note that we do have a bit of a behavior change here: we now update the mempool entry LockPoints even if we’re going to remove the entry later. I’m not sure if this is desirable.
I suggest adding documentation as well, e.g. “If the cached LockPoints are now invalid, recalculate them and update the mempool entry before calling CheckSequenceLocks().
CheckSequenceLocks() was mutating the lockpoints passed in. There is a slight behavior change to MaybeUpdateMempoolForReorg() here.
Updated 220475bdbca0598c6f5158e870b89ff9503a7258 -> aa8f68b2352dd37cae59867276b38127d7041685 (pr23897.03 -> pr23897.04):
Also the PR description has been updated.
299+ * coins, or a CCoinsViewMemPool that is connected to the
300+ * mempool and chainstate UTXO set. In the latter case, the caller is
301+ * responsible for holding the appropriate locks to ensure that
302+ * calls to GetCoin() return correct coins.
303+ *
304+ * @returns The resulting height and time calculated and the hash of the block needed for calculation.
in fc31aa12b9599f3a23c22fd65b3eaa73dbb05d81:
Should document that this returns std::nullopt if there is an error, i.e., if inputs cannot be found in the coins_view
184@@ -185,11 +185,67 @@ bool CheckFinalTxAtTip(const CBlockIndex* active_chain_tip, const CTransaction&
185 return IsFinalTx(tx, nBlockHeight, nBlockTime);
186 }
187
188+std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
189+ const CCoinsView& coins_view,
190+ const CTransaction& tx)
191+{
192+ assert(tip);
tip should be a reference instead of a pointer?
CChain::Tip() is been passed as an argument, and it could be a nullptr.
tip, as in, if a function expects a non-nullptr tip, then perhaps it should take a reference instead of a pointer. But I think I get what you mean, that CChain::Tip() returns a pointer and that’s passed around pretty frequently in the codebase.
309+
310+/**
311+ * Check if transaction will be BIP68 final in the next block to be created on top of tip.
312+ * @param[in] tip Chain tip to check tx sequence locks against. For example,
313+ * the tip of the current active chain.
314+ * @param[in] lp Relative locktime constraints (BIP68).
in 5a6ce8cca31b3105cb8e96cd3e5e6c0da0fb3479
0 * [@param](/bitcoin-bitcoin/contributor/param/)[in] lp LockPoints containing the height and time at which this transaction is final.
code review ACK 5a6ce8cca31b3105cb8e96cd3e5e6c0da0fb3479
This looks correct to me, a few suggestions to make things clearer. Probably at least 1 other person should take a look as well.
Updated 5a6ce8cca31b3105cb8e96cd3e5e6c0da0fb3479 -> 803869954e0e3bc32279dac71cf40a0614b79566 (pr23897.05 -> pr23897.06, diff):
Not sure.
reACK 803869954e0e3bc32279dac71cf40a0614b79566
Only changes are doc updates, for #23897 (review) and #23897 (review)
No objection if you want to move-only some stuff out of a function and then call the newly created function in place.
fwiw this is >50% move-only.
205+ } else {
206+ prevheights[txinIndex] = coin.nHeight;
207+ }
208+ }
209+
210+ const std::pair<int, int64_t> lock_pair{CalculateSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, prevheights, index)};
nit in ed9db580c764cf26fb534be54684c46e79300a38
0 const auto [minHeight, minTime] = CalculateSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, prevheights, index);
199+ LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, txinIndex, tx.GetHash().GetHex());
200+ return std::nullopt;
201+ }
202+ if (coin.nHeight == MEMPOOL_HEIGHT) {
203+ // Assume all mempool transaction confirm in the next block
204+ prevheights[txinIndex] = tip->nHeight + 1;
nit in ed9db580c764cf26fb534be54684c46e79300a38: Avoids dereferencing multiple times:
0diff --git a/src/validation.cpp b/src/validation.cpp
1index a25e817a5..0fa8dc30c 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -201,7 +201,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
5 }
6 if (coin.nHeight == MEMPOOL_HEIGHT) {
7 // Assume all mempool transaction confirm in the next block
8- prevheights[txinIndex] = tip->nHeight + 1;
9+ prevheights[txinIndex] = index.nHeight;
10 } else {
11 prevheights[txinIndex] = coin.nHeight;
12 }
13@@ -222,7 +222,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
14 int max_input_height{0};
15 for (const int height : prevheights) {
16 // Can ignore mempool inputs since we'll fail if they had non-zero locks
17- if (height != tip->nHeight + 1) {
18+ if (height != index.nHeight) {
19 max_input_height = std::max(max_input_height, height);
20 }
21 }
Updated bebaddb0767b9c52507e8d50a18510b592c14778 -> ed2d714bd1130291a2f781e644f787d421cdf26e (pr23897.08 -> pr23897.09, diff):
189+ // Thus if we want to know if a transaction can be part of the
190+ // *next* block, we need to use one more than active_chainstate.m_chain.Height()
191+ index.nHeight = tip->nHeight + 1;
192+
193+ std::vector<int> prevheights;
194+ prevheights.resize(tx.vin.size());
I’d avoid using resize() since there’s no need to default-initialize everything. reserve() seems more appropriate:
0 std::vector<int> prevheights;
1 prevheights.reserve(tx.vin.size());
180+ const CCoinsView& coins_view,
181+ const CTransaction& tx)
182+{
183+ assert(tip);
184+
185+ CBlockIndex index;
nit: would this be a more helpful name?
0 CBlockIndex next_tip;
175@@ -176,11 +176,67 @@ bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction&
176 return IsFinalTx(tx, nBlockHeight, nBlockTime);
177 }
178
179+std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
180+ const CCoinsView& coins_view,
181+ const CTransaction& tx)
182+{
183+ assert(tip);
CBlockIndex& instead and avoid checking for nullptr here? If this fn requires a non-nullptr CBlockIndex, I think the fn signature should reflect that and the callsite to take responsibility.
275+ * @returns The resulting height and time calculated and the hash of the block needed for
276+ * calculation, or std::nullopt if there is an error.
277+ */
278+std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
279+ const CCoinsView& coins_view,
280+ const CTransaction& tx);
tx param
183+ assert(tip);
184+
185+ CBlockIndex index;
186+ index.pprev = tip;
187+ // When SequenceLocks() is called within ConnectBlock(), the height
188+ // of the block *being* evaluated is what is used.
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()andConnectBlock()and this function - but maybe I just missed it?
202+ if (coin.nHeight == MEMPOOL_HEIGHT) {
203+ // Assume all mempool transaction confirm in the next block
204+ prevheights[txinIndex] = index.nHeight;
205+ } else {
206+ prevheights[txinIndex] = coin.nHeight;
207+ }
CalculatePrevHeights function?
238 bool CheckSequenceLocksAtTip(CBlockIndex* tip,
239- const CCoinsView& coins_view,
240- const CTransaction& tx,
241- LockPoints* lp,
242- bool useExistingLockPoints)
243+ const LockPoints& lp)
nit: I think overly abbreviated variable names should be avoided for readability
0 const LockPoints& lock_points)
194+ prevheights.resize(tx.vin.size());
195+ for (size_t txinIndex = 0; txinIndex < tx.vin.size(); ++txinIndex) {
196+ const CTxIn& txin = tx.vin[txinIndex];
197+ Coin coin;
198+ if (!coins_view.GetCoin(txin.prevout, coin)) {
199+ LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, txinIndex, tx.GetHash().GetHex());
Assume(false) here to ensure this is visible in debug builds? I don’t think we ever expect this path to be reached?
335@@ -334,20 +336,23 @@ void Chainstate::MaybeUpdateMempoolForReorg(
336
337 // 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.
0diff --git a/src/validation.cpp b/src/validation.cpp
1index 0986147b7..1657323bd 100644
2--- a/src/validation.cpp
3+++ b/src/validation.cpp
4@@ -323,60 +323,60 @@ void Chainstate::MaybeUpdateMempoolForReorg(
5 // the disconnectpool that were added back and cleans up the mempool state.
6 m_mempool->UpdateTransactionsFromBlock(vHashUpdate);
7
8- // Predicate to use for filtering transactions in removeForReorg.
9- // Checks whether the transaction is still final and, if it spends a coinbase output, mature.
10- // Also updates valid entries' cached LockPoints if needed.
11- // If false, the tx is still valid and its lockpoints are updated.
12- // If true, the tx would be invalid in the next block; remove this entry and all of its descendants.
13- const auto filter_final_and_mature = [this](CTxMemPool::txiter it)
14- EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) {
15- AssertLockHeld(m_mempool->cs);
16- AssertLockHeld(::cs_main);
17- const CTransaction& tx = it->GetTx();
18+ // We also need to remove any now-immature transactions
19+ m_mempool->removeForReorg(m_chain, [this](CTxMemPool::txiter it){ return !this->IsTxFinalAndMature(it); });
20+ // Re-limit mempool size, in case we added any transactions
21+ LimitMempoolSize(*m_mempool, this->CoinsTip());
22+}
23
24- // The transaction must be final.
25- if (!CheckFinalTxAtTip(*Assert(m_chain.Tip()), tx)) return true;
26+// Predicate to use for filtering transactions in removeForReorg.
27+// Checks whether the transaction is still final and, if it spends a coinbase output, mature.
28+// Also updates valid entries' cached LockPoints if needed.
29+// If true, the tx is still valid and its lockpoints are updated.
30+// If false, the tx would be invalid in the next block; remove this entry and all of its descendants.
31+bool Chainstate::IsTxFinalAndMature(CTxMemPool::txiter it) EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main)
32+{
33+ AssertLockHeld(m_mempool->cs);
34+ AssertLockHeld(::cs_main);
35+ const CTransaction& tx = it->GetTx();
36
37- const LockPoints& lp = it->GetLockPoints();
38- // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be
39- // created on top of the new chain.
40- if (TestLockPointValidity(m_chain, lp)) {
41- if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
42- return true;
43- }
44+ // The transaction must be final.
45+ if (!CheckFinalTxAtTip(*Assert(m_chain.Tip()), tx)) return true;
46+
47+ const LockPoints& lp = it->GetLockPoints();
48+ // CheckSequenceLocksAtTip checks if the transaction will be final in the next block to be
49+ // created on top of the new chain.
50+ if (TestLockPointValidity(m_chain, lp)) {
51+ if (!CheckSequenceLocksAtTip(m_chain.Tip(), lp)) {
52+ return false;
53+ }
54+ } else {
55+ const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool};
56+ const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)};
57+ if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) {
58+ // Now update the mempool entry lockpoints as well.
59+ m_mempool->mapTx.modify(it, [&new_lock_points](CTxMemPoolEntry& e) { e.UpdateLockPoints(*new_lock_points); });
60 } else {
61- const CCoinsViewMemPool view_mempool{&CoinsTip(), *m_mempool};
62- const std::optional<LockPoints> new_lock_points{CalculateLockPointsAtTip(m_chain.Tip(), view_mempool, tx)};
63- if (new_lock_points.has_value() && CheckSequenceLocksAtTip(m_chain.Tip(), *new_lock_points)) {
64- // Now update the mempool entry lockpoints as well.
65- m_mempool->mapTx.modify(it, [&new_lock_points](CTxMemPoolEntry& e) { e.UpdateLockPoints(*new_lock_points); });
66- } else {
67- return true;
68- }
69+ return false;
70 }
71+ }
72
73- // If the transaction spends any coinbase outputs, it must be mature.
74- if (it->GetSpendsCoinbase()) {
75- for (const CTxIn& txin : tx.vin) {
76- auto it2 = m_mempool->mapTx.find(txin.prevout.hash);
77- if (it2 != m_mempool->mapTx.end())
78- continue;
79- const Coin& coin{CoinsTip().AccessCoin(txin.prevout)};
80- assert(!coin.IsSpent());
81- const auto mempool_spend_height{m_chain.Tip()->nHeight + 1};
82- if (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY) {
83- return true;
84- }
85+ // If the transaction spends any coinbase outputs, it must be mature.
86+ if (it->GetSpendsCoinbase()) {
87+ for (const CTxIn& txin : tx.vin) {
88+ auto it2 = m_mempool->mapTx.find(txin.prevout.hash);
89+ if (it2 != m_mempool->mapTx.end())
90+ continue;
91+ const Coin& coin{CoinsTip().AccessCoin(txin.prevout)};
92+ assert(!coin.IsSpent());
93+ const auto mempool_spend_height{m_chain.Tip()->nHeight + 1};
94+ if (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY) {
95+ return false;
96 }
97 }
98- // Transaction is still valid and cached LockPoints are updated.
99- return false;
100- };
101-
102- // We also need to remove any now-immature transactions
103- m_mempool->removeForReorg(m_chain, filter_final_and_mature);
104- // Re-limit mempool size, in case we added any transactions
105- LimitMempoolSize(*m_mempool, this->CoinsTip());
106+ }
107+ // Transaction is still valid and cached LockPoints are updated.
108+ return true;
109 }
110
111 /**
112diff --git a/src/validation.h b/src/validation.h
113index bd6f710d1..52f1ee66f 100644
114--- a/src/validation.h
115+++ b/src/validation.h
116@@ -291,6 +291,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
117 * passed in for evaluation.
118 * The LockPoints should not be considered valid if CheckSequenceLocksAtTip returns false.
119 */
120+
121 bool CheckSequenceLocksAtTip(CBlockIndex* tip,
122 const LockPoints& lp);
123
124@@ -770,6 +771,8 @@ private:
125 DisconnectedBlockTransactions& disconnectpool,
126 bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);
127
128+ bool IsTxFinalAndMature(CTxMemPool::txiter it) EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main);
129+
130 /** Check warning conditions and do some notifications on new chain tip set. */
131 void UpdateTip(const CBlockIndex* pindexNew)
132 EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
831@@ -827,7 +832,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
832 }
833
834 entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(),
835- fSpendsCoinbase, nSigOpsCost, lp));
836+ fSpendsCoinbase, nSigOpsCost, *lock_points));
Since we’re doing the .has_value() check at quite a distance from here, I think it’s safer to use .value() for robustness with future refactoring.
0 fSpendsCoinbase, nSigOpsCost, lock_points.value()));
287+ * transaction is final.
288 * Simulates calling SequenceLocks() with data from the tip passed in.
289 * Optionally stores in LockPoints the resulting height and time calculated and the hash
290 * of the block needed for calculation or skips the calculation and uses the LockPoints
291 * passed in for evaluation.
292 * The LockPoints should not be considered valid if CheckSequenceLocksAtTip returns false.
@hebasto I think we don’t modify LockPoints anymore?
0 * Optionally stores in LockPoints the resulting height and time calculated and the hash
1 * of the block needed for calculation or skips the calculation and uses the LockPoints
2 * passed in for evaluation.
223+ for (const int height : prevheights) {
224+ // Can ignore mempool inputs since we'll fail if they had non-zero locks
225+ if (height != index.nHeight) {
226+ max_input_height = std::max(max_input_height, height);
227+ }
228+ }
Alternative approach using replace_if and max_element
0 // Set heights of mempool inputs to 0 since we'll fail if they had non-zero locks
1 std::replace_if(prevheights.begin(), prevheights.end(), [&index](const int height){ return height == index.nHeight; }, 0);
2 const auto max_input_height{*std::max_element(prevheights.begin(), prevheights.end())};
Approach ACK ed2d714bd
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.
Updated ed2d714bd1130291a2f781e644f787d421cdf26e -> 1636bc465fd96b9561de3dd21bed7f989f409de1 (pr23897.09 -> pr23897.10, diff):
175@@ -176,11 +176,79 @@ bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction&
176 return IsFinalTx(tx, nBlockHeight, nBlockTime);
177 }
178
179+namespace {
180+std::optional<std::vector<int>> CalculatePrevHeights(
175@@ -176,11 +176,79 @@ bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction&
176 return IsFinalTx(tx, nBlockHeight, nBlockTime);
177 }
178
179+namespace {
180+std::optional<std::vector<int>> CalculatePrevHeights(
181+ const CBlockIndex next_tip,
any reason not to pass a reference here?
0 const CBlockIndex& next_tip,
214+ // of the block *being* evaluated is what is used.
215+ // Thus if we want to know if a transaction can be part of the
216+ // *next* block, we need to use one more than active_chainstate.m_chain.Height()
217+ next_tip.nHeight = tip->nHeight + 1;
218+
219+ auto prev_heights = CalculatePrevHeights(next_tip, coins_view, tx);
nit
0 auto prev_heights{CalculatePrevHeights(next_tip, coins_view, tx)};
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto:
Exception Avoid
autofor initializer lists and in cases where you know exactly which type you want and where an initializer might require conversion.
auto i{1}. In this case, CalculatePrevHeights() has a well defined return type, so list initialization seems appropriate.
187+ for (size_t i = 0; i < tx.vin.size(); ++i) {
188+ const CTxIn& txin = tx.vin[i];
189+ Coin coin;
190+ if (!coins.GetCoin(txin.prevout, coin)) {
191+ LogPrintf("ERROR: %s: Missing input %d in transaction \'%s\'\n", __func__, i, tx.GetHash().GetHex());
192+ 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.
If I missed your point, what are benefits of propagating the error message back?
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.
Updated 1636bc465fd96b9561de3dd21bed7f989f409de1 -> 63d6b9a9f067b81fed6029099a4643996e8d274b (pr23897.10 -> pr23897.11):
244@@ -245,7 +245,9 @@ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxM
245 bool CheckFinalTxAtTip(const CBlockIndex& active_chain_tip, const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
246
247 /**
248- * Check if transaction will be BIP68 final in the next block to be created on top of tip.
249+ * Calculate LockPoints required to check if transaction will be BIP68 final in the next block
250+ * to be created on top of tip.
251+ *
252 * @param[in] tip Chain tip to check tx sequence locks against. For example,
ACK 68c0f16fa2a41d6a25361f0bfa0b74ac5c8c43e2
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):
Updated 68c0f16fa2a41d6a25361f0bfa0b74ac5c8c43e2 -> 75db62ba4cae048e742ca02dc6a52b3b3d6727de (pr23897.12 -> pr23897.13, diff).
Addressed some of @stickies-v’s comments:
