The sentinel pattern in ConnectTrace has been unnecessary since conflicted transaction tracking was removed in 5613f9842b4000fed088b8cf7b99674c328d15e1. Without that tracking ConnectTrace is a trivial wrapper around std::vector, so it seems better to just replace it with the vector directly.
Also modernize/update naming along the way, renaming PerBlockConnectTrace to ConnectedBlock
Refactor, no behaviour change.
<!--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.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
2994 | @@ -2995,9 +2995,6 @@ struct PerBlockConnectTrace { 2995 | /** 2996 | * Used to track blocks whose transactions were applied to the UTXO state as a 2997 | * part of a single ActivateBestChainStep call. 2998 | - * 2999 | - * This class is single-use, once you call GetBlocksConnected() you have to throw 3000 | - * it away and make a new one.
I think I'd keep the comment. It is not immediately obvious that the class is single use if you just skim its member function declarations.
3021 | // one waiting for the transactions from the next block. We pop 3022 | // the last entry here to make sure the list we return is sane. 3023 | assert(!blocksConnected.back().pindex); 3024 | blocksConnected.pop_back(); 3025 | - return blocksConnected; 3026 | + return std::move(blocksConnected);
Would this be better as:
assert(!blocksConnected.empty()); // cannot call GetBlocksConnected() twice
assert(!blocksConnected.back().pindex);
blocksConnected.pop_back();
std::vector<PerBlockConnectTrace> result;
std::swap(result, blocksConnected);
return result;
so that blocksConnected is left in a well-defined empty state?
Thanks for this suggestion. I now agree this would have been a better implementation for GetBlocksConnected() &&. I haven't implemented many rvalue ref-qualified methods, so this has been a good learning experience, appreciate the help.
Resolving since the class no longer exists in HEAD.
3423 | @@ -3426,7 +3424,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< 3424 | } 3425 | pindexNewTip = m_chain.Tip(); 3426 | 3427 | - for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { 3428 | + for (const PerBlockConnectTrace& trace : std::move(connectTrace).GetBlocksConnected()) {
Not really sure that does much -- std::move() doesn't really enforce anything much at compile-time (linters sometimes enforce things, eg clang-tidy's bugprone-use-after-move). For example this change compiles fine:
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3424,6 +3424,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
pindexNewTip = m_chain.Tip();
+ auto x = std::move(connectTrace).GetBlocksConnected();
for (const PerBlockConnectTrace& trace : std::move(connectTrace).GetBlocksConnected()) {
assert(trace.pblock && trace.pindex);
if (m_chainman.m_options.signals) {```
Previously reusing GetBlocksConnected() would fire an assertion as the extra block has been removed; but I think with this PR it just invokes UB as blocksConnected is no longer in a valid state. So this largely seems worse than the existing code to me.
I think changing the name to be ConsumeBlocksConnected() without touching the semantics would probably work better than messing with std::move, but given the existing comment and assertion I don't think changes here add much value at all.
The sentinel pattern was necessary to collect conflicted transactions
before their associated block was recorded, but that tracking was
removed in 5613f9842b4000fed088b8cf7b99674c328d15e1.
Thanks for the review and feedback, @sedited and @ajtowns. I'd forgotten that it's generally discouraged, but not illegal or even UB to access an object after it has been moved, making my suggested interface much less clear and robust than I thought it was.
I've now overhauled the approach. As I digged into its usage a bit more, it has become clear that the sentinel block is no longer necessary, allowing us to (b83de7f28e71f0b10a999327395198fbcb02f44b) make ConnectTrace reusable without overhead. But since basically the class has now become a trivial wrapper around its vector, I think it makes more sense to just remove (e80ace13ad94feb6efe1d50aaaceeb6c2a306991) it entirely, which is the approach I've taken here.
Updated OP and title with the new approach.
3027 | - } 3028 | +struct ConnectedBlock { 3029 | + CBlockIndex* const pindex; 3030 | + const std::shared_ptr<const CBlock> pblock; 3031 | + ConnectedBlock(CBlockIndex* pindex, std::shared_ptr<const CBlock> pblock) 3032 | + : pindex{Assert(pindex)}, pblock{Assert(std::move(pblock))} {}
The ConnectedBlock can be simplified.
const members suppress move semantics, so vector reallocation will copy the shared_ptr instead of moving it.
The Assert() null-checks were useful when the sentinel pattern could produce empty entries, but with the sentinel gone ConnectTip only reaches the emplace_back after successful connection where both pointers are known-valid.
CBlockIndex* pindex;
std::shared_ptr<const CBlock> pblock;
Thanks, I agree the correct-by-construction guarantees are a bit overkill here, so I've adopted your suggestion (but kept the Assert in ConnectTip() since this relies on another ActivateBestChainStep's implementation).
ACK e80ace13ad94feb6efe1d50aaaceeb6c2a306991
Force-pushed to remove the correct-by-construction commit since it's overkill and suppresses ConnectedBlock's move semantics, addressing #34708 (review).
reACK 5d558f024603ae5f4ed4231634394b7794b95eca
2989 | @@ -2990,39 +2990,24 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
2990 | struct PerBlockConnectTrace {
2991 | CBlockIndex* pindex = nullptr;
Could this be made const and not defaulted to nullptr?
Sure, pushed!
Nice, Approach ACK
Replace ConnectTrace with a plain std::vector<ConnectedBlock>, and
rename PerBlockConnectTrace to ConnectedBlock and connectTrace to
connected_blocks.
2986 | @@ -2987,57 +2987,22 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra 2987 | return true; 2988 | } 2989 | 2990 | -struct PerBlockConnectTrace { 2991 | - CBlockIndex* pindex = nullptr; 2992 | +struct ConnectedBlock { 2993 | + const CBlockIndex* pindex;
CBlockIndex& index;
nit: Haven't tried, but since you are touching all lines, it could make sense to use a reference or std::refernece_wrapper, as nullptr is never used and not allowed anyway?
I considered this but decided not to, because we can't use a similar pattern for pblock and it doesn't simplify things - just moves the asserts to different places. I don't really have a preference either way, if you feel like the reference is better here I'm happy to update it.
<details> <summary>git diff on 2f8f2e9001</summary>
diff --git a/src/validation.cpp b/src/validation.cpp
index 83c16867fe..873c269335 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2988,7 +2988,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
}
struct ConnectedBlock {
- const CBlockIndex* pindex;
+ CBlockIndex& pindex;
std::shared_ptr<const CBlock> pblock;
};
@@ -3099,7 +3099,7 @@ bool Chainstate::ConnectTip(
Chainstate& current_cs{m_chainman.CurrentChainstate()};
m_chainman.MaybeValidateSnapshot(*this, current_cs);
- connected_blocks.emplace_back(pindexNew, std::move(block_to_connect));
+ connected_blocks.emplace_back(*Assert(pindexNew), std::move(block_to_connect));
return true;
}
@@ -3393,7 +3393,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
for (const auto& [index, block] : connected_blocks) {
if (m_chainman.m_options.signals) {
- m_chainman.m_options.signals->BlockConnected(chainstate_role, Assert(block), Assert(index));
+ m_chainman.m_options.signals->BlockConnected(chainstate_role, Assert(block), &index);
}
}
</details>
it doesn't simplify things - just moves the asserts to different places.
I think this is the simplification: When seeing an Assert(block)->whatever, I wonder if the assert can be reached (remotely or at all).
Also, if there is more than one place that "needs" the assert, the code will become both harder to read and more verbose.
because we can't use a similar pattern for pblock
I think it is probably time to cherry-pick the header-only gsl::not_null. It seems better to have a compiler-enforced reference type, than having to guess every time if a shared_ptr is meant to be a reference, or a pointer. Also:
typedef std::shared_ptr<const CTransaction> CTransactionRef;
I think it is probably time to cherry-pick the header-only gsl::not_null. It seems better to have a compiler-enforced reference type, than having to guess every time if a shared_ptr is meant to be a reference, or a pointer.
I agree, and happy to review if you decide to work on that!
Ok, done in #34844. I may push the above diff some time later after or with #34704 (review).
ACK 2f8f2e900118c98699db91075bd1b72e6460c6b1
ACK 2f8f2e900118c98699db91075bd1b72e6460c6b1