ConnectTrace is now only used to track blocks that were connected during an ActivateBestChainStep call. Simplify it to a typedef and fix comments.
Rather than trying to remove ConnectTrace, which would change the order that notifications are fired in the case of a re-org (https://github.com/bitcoin/bitcoin/pull/17562#issuecomment-615251297), just simplify it and fix comments.
2511@@ -2512,45 +2512,20 @@ static int64_t nTimeFlush = 0;
2512 static int64_t nTimeChainState = 0;
2513 static int64_t nTimePostConnect = 0;
2514
2515+//! A single block and block index pointer.
2516 struct PerBlockConnectTrace {
2517 CBlockIndex* pindex = nullptr;
2518 std::shared_ptr<const CBlock> pblock;
2519- PerBlockConnectTrace() {}
2520+ PerBlockConnectTrace(CBlockIndex* index, std::shared_ptr<const CBlock> block) : pindex(index), pblock(block) {}
0 PerBlockConnectTrace(CBlockIndex* index, const std::shared_ptr<const CBlock>& block) : pindex(index), pblock(block) {}
style nit to avoid shared_ptr increment/decrement
Thanks for this comment! It sent me down a bit of a rabbit hole about how to pass smart pointers. Here’s what I’ve learned:
I think your suggestion to change the ctor to take a const lvalue reference makes a copy and therefore increases the ref count (the const is the callee saying to the caller “I won’t steal this reference”, and so if the callee wants to keep ownership, it needs to make a copy). We can avoid that copy in three ways:
std::shared_ptr<const CBlock>&) which is the callee saying “I may or may not steal this reference” and call the constructor with an lvalue (ie thisBlock below).std::shared_ptr<const CBlock>), and have the caller cast to rvalue reference in the call (ie std::move(thisBlock) below). The std::move is the caller saying “I no longer want ownership of this reference. In this case, the caller can still keep shared ownership by passing an lvalue (thisBlock), which would increase the ref count.std::shared_ptr<const CBlock>&&) which is the callee saying “I will steal ownership”. The caller has to call using an rvalue reference (std::move(thisBlock)). Calling by lvalue would be invalid.I think it’s best to keep this as pass-by-value and have the caller pass it with an rvalue reference.
Also, the initializer list needs to use a std::move to prevent the ref increment/decrement, which I’ll change.
Some references:
2556- assert(!blocksConnected.back().pindex);
2557- blocksConnected.pop_back();
2558- return blocksConnected;
2559- }
2560-};
2561+typedef std::vector<PerBlockConnectTrace> ConnectTrace;
2842@@ -2868,7 +2843,7 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2843 }
2844 pindexNewTip = m_chain.Tip();
2845
2846- for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2847+ for (auto& trace : connectTrace) {
auto shouldn’t be used in consensus code unless there is a reason
408@@ -409,7 +409,9 @@ enum DisconnectResult
409 DISCONNECT_FAILED // Something else went wrong.
410 };
411
412-class ConnectTrace;
413+// forward declaration for ActivateBestChainStep and ConnectTip
414+struct PerBlockConnectTrace;
415+typedef std::vector<PerBlockConnectTrace> ConnectTrace;
0using ConnectTrace = std::vector<PerBlockConnectTrace>;
style nit for C++11
I don’t think it makes a difference for aliases which are not templates (and I can’t see this in the style guide), but changed anyway.
This is my understanding as well after looking this up, along with typedef being the earlier, traditional syntax of the two. Perhaps one advantage typedef may have over using is that it is used only for declaring type aliases.
typedef as well.
2550- std::vector<PerBlockConnectTrace>& GetBlocksConnected() {
2551- // We always keep one extra block at the end of our list because
2552- // blocks are added after all the conflicted transactions have
2553- // been filled in. Thus, the last entry should always be an empty
2554- // one waiting for the transactions from the next block. We pop
2555- // the last entry here to make sure the list we return is sane.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
2584@@ -2610,7 +2585,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
2585 LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime5) * MILLI, nTimePostConnect * MICRO, nTimePostConnect * MILLI / nBlocksTotal);
2586 LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime1) * MILLI, nTimeTotal * MICRO, nTimeTotal * MILLI / nBlocksTotal);
2587
2588- connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
2589+ connectTrace.emplace_back(pindexNew, pthisBlock);
I’m still very new to move semantics, but just confirming, what’s the motivation for removing the std::move?
In my local experiments, it looks like taking out the std::move increments the refcount; could this have performance implications?
std::moveint main() {
CBlockIndex index;
CBlockIndex* pindex = &index;
std::shared_ptr<const CBlock> pblock(new CBlock);
// (lldb) p pblock
// strong=1 weak=1 {
// __ptr_ = 0x0000000100205290
// }
ConnectTrace connectTrace;
connectTrace.emplace_back(pindex, pblock);
// (lldb) p pblock
// strong=2 weak=1 {
// __ptr_ = 0x0000000100205290
// }
}
std::moveint main() {
CBlockIndex index;
CBlockIndex* pindex = &index;
std::shared_ptr<const CBlock> pblock(new CBlock);
// (lldb) p pblock
// strong=1 weak=1 {
// __ptr_ = 0x0000000100304150
// }
ConnectTrace connectTrace;
connectTrace.emplace_back(pindex, std::move(pblock));
// (lldb) p pblock
// nullptr {
// __ptr_ = 0x0000000000000000
// }
}Yes, you’re right! The caller should cast to rvalue reference by calling std::move() (that was my intention in #18685 (review)).
Thanks for catching this.
2538-public:
2539- explicit ConnectTrace() : blocksConnected(1) {}
2540-
2541- void BlockConnected(CBlockIndex* pindex, std::shared_ptr<const CBlock> pblock) {
2542- assert(!blocksConnected.back().pindex);
2543- assert(pindex);
ConnectTip guarantees guarantees neither the CBlockIndex nor the std::shared_ptr<const CBlock> will be null.
131@@ -132,14 +132,19 @@ class CValidationInterface {
132 virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
133 /**
134 * Notifies listeners of a block being connected.
135- * Provides a vector of transactions evicted from the mempool as a result.
136+ *
137+ * The mempool is not guaranteed to be in a consistent state when this
138+ * notification is fired. Listeners should use UpdatedBlockTip for that.
Beginner question: what do you mean by “consistent state” here?
i.e. what could go wrong with the mempool after some, but not all, of the ActivateBestChainStep involved in ActivateBestChain?
cs_main and mempool.cs are both still being held when the BlockConnected signal is called, and UpdateMempoolForReorg() may have been called after blocks were connected but before the BlockConnected signal is called. See the comment above for the TransactionAddedToMempool signal.
The UpdatedBlockTip signal is called just before releasing cs_main, when we know that the mempool is consistent with the chain tip.
Thanks for clarifying! Here’s my updated understanding:
If a validationinterface listener considers the callbacks as a canonical log of all events, then immediately after a BlockConnected but before the subsequent UpdatedBlockTip, the listener might have an inconsistent view (e.g. some transactions have disappeared from both the mempool and the active chain)
If a validationinterface callback locks both cs_main and m_mempool.cs before accessing the underlying state directly, it WOULD get a consistent view (because by the time the lock can be acquired, the UpdatedBlockTip would have happened), but I don’t think any callbacks do this at the moment
UpdatedBlockTip to be sure that the state is consistent.
Concept ACK, thanks for doing this cleanup!
Unit tests passed locally
(Aside: What’s the desired benefit of reviewers doing local testing, given the nice CI system? Is it to get more test runs on different system configurations?)
2556- * This class is single-use, once you call GetBlocksConnected() you have to throw
2557- * it away and make a new one.
2558+ * Used to track blocks that were connected as part of a single
2559+ * ActivateBestChainStep call. The BlockConnected validationinterface signal is
2560+ * fired after a single ABCS call for all blocks that were connected in that
2561+ * ABCS call.
Nit (feel free to ignore): How would you feel about rewording this to make it really obvious that there’s one BlockConnected signal per block? For example:
Used to track blocks that were connected as part of a single
ActivateBestChainStep call. After the ABCS call, a separate BlockConnected
validationinterface signal is fired for each block that was connected
in the ABCS call.2585- assert(!blocksConnected.back().pindex);
2586- blocksConnected.pop_back();
2587- return blocksConnected;
2588- }
2589-};
2590+using ConnectTrace = std::vector<PerBlockConnectTrace>;
using and moving the associated comment to the header. (In particular, if someone changes one using or the other, the one in validation.cpp wins; not precisely shadowing but still annoying)
2540@@ -2541,45 +2541,20 @@ static int64_t nTimeFlush = 0;
2541 static int64_t nTimeChainState = 0;
2542 static int64_t nTimePostConnect = 0;
2543
2544+//! A single block and block index pointer.
2545 struct PerBlockConnectTrace {
2546 CBlockIndex* pindex = nullptr;
2547 std::shared_ptr<const CBlock> pblock;
2548- PerBlockConnectTrace() {}
2549+ PerBlockConnectTrace(CBlockIndex* index, std::shared_ptr<const CBlock> block) : pindex(index), pblock(std::move(block)) {}
assert(index != nullptr && block); would replace the assertions you’re deleting below.
Clarify that the mempool is not guaranteed to be in a consistent state
with the chain tip when BlockConnected and BlockDisconnected are fired.
Remove outdated comment about BlockConnected providing a vector of
transactions.
ConnectTrace is now only used to track blocks that were connected during
an ActivateBestChainStep call. Simplify it to a typedef.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
