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
Rebased 154307c10b0d3c0e7dfbe463de7d040b2eb97c74 -> c4476a5ea787e3722d8c1e5fed5f0c6e2a554327 (pr23897.01 -> pr23897.02) on top of the merged #23683.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Rebased c4476a5ea787e3722d8c1e5fed5f0c6e2a554327 -> 220475bdbca0598c6f5158e870b89ff9503a7258 (pr23897.02 -> pr23897.03) on top of the merged #23976.
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)
It would be nice to remove the raw pointers while we're changing the interface anyway. I think a better interface is to have 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__);
Can be replaced with a 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)
Same here, would be nice to remove the raw pointer and make this parameter a 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().
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.
Updated 220475bdbca0598c6f5158e870b89ff9503a7258 -> aa8f68b2352dd37cae59867276b38127d7041685 (pr23897.03 -> pr23897.04):
Also the PR description has been updated.
Rebased aa8f68b2352dd37cae59867276b38127d7041685 -> 5a6ce8cca31b3105cb8e96cd3e5e6c0da0fb3479 (pr23897.04 -> pr23897.05) due to conflict with #24804.
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);
To me, this suggests tip should be a reference instead of a pointer?
CChain::Tip() is been passed as an argument, and it could be a nullptr.
Sorry I should have said the parameter 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
* [@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.
Rebased 803869954e0e3bc32279dac71cf40a0614b79566 -> 32e3fc173410641569bc4df805d5431cf0013d74 (pr23897.06 -> pr23897.07) due to the conflict with #25677.
Rebased 32e3fc173410641569bc4df805d5431cf0013d74 -> bebaddb0767b9c52507e8d50a18510b592c14778 (pr23897.07 -> pr23897.08) due to the conflict with #25073.
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
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:
diff --git a/src/validation.cpp b/src/validation.cpp
index a25e817a5..0fa8dc30c 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -201,7 +201,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
}
if (coin.nHeight == MEMPOOL_HEIGHT) {
// Assume all mempool transaction confirm in the next block
- prevheights[txinIndex] = tip->nHeight + 1;
+ prevheights[txinIndex] = index.nHeight;
} else {
prevheights[txinIndex] = coin.nHeight;
}
@@ -222,7 +222,7 @@ std::optional<LockPoints> CalculateLockPointsAtTip(CBlockIndex* tip,
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 != tip->nHeight + 1) {
+ if (height != index.nHeight) {
max_input_height = std::max(max_input_height, height);
}
}
ACK bebaddb0767b9c52507e8d50a18510b592c14778 The code is now much clearer and easier to understand and the behavior doesn't seem to have changed.
Updated bebaddb0767b9c52507e8d50a18510b592c14778 -> ed2d714bd1130291a2f781e644f787d421cdf26e (pr23897.08 -> pr23897.09, diff):
reACK ed2d714bd1130291a2f781e644f787d421cdf26e
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:
std::vector<int> prevheights;
prevheights.reserve(tx.vin.size());
Going to leave it as this because the suggested diff won't work.
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?
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);
Would it make sense to pass a 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.
Going to leave it as this to keep this PR in more reviewable state.
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);
nit: missing doxygen for tx param
Concept ACK. Hooray for clearer function intents and effects!
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.
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()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 | + }
For readability, I think this chunk of code could be separated out into a 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
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());
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.
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.
<details> <summary>git diff</summary>
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);
</details>
Going to leave it as this to keep this PR in more reviewable state.
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.
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.
This documentation now seems out of date
@hebasto I think we don't modify LockPoints anymore?
* Optionally stores in LockPoints the resulting height and time calculated and the hash
* of the block needed for calculation or skips the calculation and uses the LockPoints
* 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
// 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.
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(
I think this function would benefit from a docstring
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?
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
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.
Correct me if I'm wrong, but I don't think this applies here. The recommendation you reference is if the type is not specified, e.g. 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?
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.
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,
nit: "check tx sequence locks" I think should be updated to reflect that this is a pure calculation function, not a checking function?
E.g. "Chain tip to check tx sequence locks against." -> "Chain tip for which tx sequence locks are calculated."
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:
re-ACK 75db62b
ACK 75db62ba4cae048e742ca02dc6a52b3b3d6727de
code LGTM