ConnectTrace was introduced in 6fdd43b968f984ef92ca4576872dc65462ba7312
when the SyncTransactions signal needed to be fired outside the cs_main
scope. The CValidationInterface is now asynchronous and signals can be
fired while holding cs_main. Remove the ConnectTrace struct that needs
to be passed up and down the stack and just fire BlockConnected()
directly in ConnectTip().
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Concept ACK.
Not sure about the wording of commit message, signals can be fired while holding cs_main, would say something more like callbacks can be scheduled while holding cs_main, at a later timer, the scheduler thread may execute them without regard to holding cs_main or not.
2519@@ -2562,7 +2520,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
2520 LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime5) * MILLI, nTimePostConnect * MICRO, nTimePostConnect * MILLI / nBlocksTotal);
2521 LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime1) * MILLI, nTimeTotal * MICRO, nTimeTotal * MILLI / nBlocksTotal);
2522
2523- connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
2524+ GetMainSignals().BlockConnected(pthisBlock, pindexNew);
If you need to retouch this PR: perhaps add code documentation here like that for GetMainSignals().BlockDisconnected (or GetMainSignals().ChainStateFlushed or GetMainSignals().UpdatedBlockTip)
0git grep -C1 "0-confirmed or conflicted"
ConnectTrace was introduced in 6fdd43b968f984ef92ca4576872dc65462ba7312
when the SyncTransactions signal needed to be fired outside the cs_main
scope. The CValidationInterface is now asynchronous and callbacks can be
scheduled without holding cs_main. The scheduler thread can execute the
callback later without regard to cs_main.
Remove the ConnectTrace struct that needs to be passed up and down the
stack and just fire BlockConnected() directly in ConnectTip().
GitHub payload is missing a merge commit (mergeable_state: "blocked", merged: false)
Ok, this was stupid. Ci is running now
Concept ACK
2532- std::vector<PerBlockConnectTrace>& GetBlocksConnected() {
2533- // We always keep one extra block at the end of our list because
2534- // blocks are added after all the conflicted transactions have
2535- // been filled in. Thus, the last entry should always be an empty
2536- // one waiting for the transactions from the next block. We pop
2537- // the last entry here to make sure the list we return is sane.
Removing code is fine, but I’d like to see the documentation updated a bit more. This will also explain why it is safe to reorganize the signals as a result of the changes in this pull request.
https://doxygen.bitcoincore.org/class_c_validation_interface.html#ac63500b25a37b34ad2ec234fde239405 says “Provides a vector of transactions evicted from the mempool as a result.”, which is wrong after 312d27b11ceaacd3be75a14907f856352b7dcf48. This should be fixed.
https://doxygen.bitcoincore.org/class_c_validation_interface.html#a6de358ebc946cbd33ce73eef61993fe5 implies that mempool removal signals are ordered before block connection signals. After the changes in this pull request, this is no longer true. So an operation in a BlockConnected callback, that depends on an operation run in the TransactionRemovedFromMempool, might now fail. Someone should check the wallet code to make sure this is not the case. Though, at the very least in this pull, the documentation should be updated.
https://doxygen.bitcoincore.org/class_c_validation_interface.html#a794911828f9350d82bc1941ba82e7463 doesn’t explicitly say that this can be used a synchronisation mechanism to catch up with the current chain state and mempool state. With the UpdatedBlockTip signal, it is possible to wait for a chain client to be sure to be up to date (mempool and chainstate) as of that new blockindex. The documentation could be updated to make this explicit.
Concept ACK ab00be08e8e4371bf1483d139ecb50dacba40e0a, removing code is my favorite! Though, needs some doc updated, I think 🎌
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3Concept ACK ab00be08e8e4371bf1483d139ecb50dacba40e0a, removing code is my favorite! Though, needs some doc updated, I think 🎌
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUi1Xwv8C0bXBK8kwoBLkKTv34M+YT29ESnNrB2kRI95LUZ1J/sXyzIR7nU39wZz
8y+/w7cGwxVAITspBSctzKsu42BmrQLs9h4YVYLZLJEmoybHggrvm6c331xMwPexQ
9Ktcozmee8qH5i+B0Pqyusk39Jnq/j6S2pm9ahgCh0woypucZWh27Rrjfg/YAgV6i
10tbnhe0SY/FFffCH2d9vCFy/HPfPy+ZUSKQOLjY06P+DAJA7NBFzmddKMR7pOp+NB
11jGA4RzcqYqACBmjh8WabTWoDf2hgYMyklMYe/HyaUE0Vg1WtYjJf3qapeeFtkMfx
12iiGoO+HiASHvqi18Q0489o6nuDhWCIqpKkXl18+hdWvSbiNkySqd7KeWfPEU6s9G
13SbEuImgxMJa7KbWkPlsjZxJKPStj8EP9zQr1NaVKt85E+MTCQAgrLj5kZL0/g9Ra
14IOfg2RntWbOOqyE3AlT376IAk3sMvhzJZDxWQGhbRMQ4ew1ut+phojsylFGDNyVB
15pJVLAPyQ
16=fKV2
17-----END PGP SIGNATURE-----
Timestamp of file with hash 33cca684633289f50bf74baf0a39711a82c5a0b6fcd6d62d0dc47653f74fa2fa -
2546 * corresponding to pindexNew, to bypass loading it again from disk.
2547- *
2548- * The block is added to connectTrace if connection succeeds.
2549 */
2550-bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
2551+bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)
style-nit:
0bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions& disconnectpool)
BlockConnected to determine when it is synced (BlockUntilSyncedToCurrentChain()). This is broken now.
2783@@ -2826,14 +2784,13 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2784 LimitValidationInterfaceQueue();
2785
2786 {
2787- LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2788+ // Do not unlock cs_main or mempool until we've made forward
2789+ // progress (with the exception of shutdown due to hardware issues,
2811@@ -2855,13 +2812,11 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2812 // Wipe cache, we may need another branch now.
2813 pindexMostWork = nullptr;
2814 }
2815+
2816 pindexNewTip = m_chain.Tip();
2817
2818- for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2819- assert(trace.pblock && trace.pindex);
2820- GetMainSignals().BlockConnected(trace.pblock, trace.pindex);
BlockUntilSyncedToCurrentChain waits until scheduler queue being drained before letting the wallet moving forward. From where we shot BlockConnected shouldn’t change anything ?
@ariard If m_last_block_processed is the chain tip, it will not empty the scheduler queue, but return early, so it is going to miss tx updates in case of a reorg. Note that m_last_block_processed is set on blockConnected/Disconnected.
See also my second bullet point here: #17562#pullrequestreview-379685371
https://doxygen.bitcoincore.org/class_c_validation_interface.html#a6de358ebc946cbd33ce73eef61993fe5 implies that mempool removal signals are ordered before block connection signals. After the changes in this pull request, this is no longer true. So an operation in a BlockConnected callback, that depends on an operation run in the TransactionRemovedFromMempool, might now fail.
This PR doesn’t inverse mempool removal signals and block connection signals as far as I understand. Mempool removal are triggered in removeUnchecked which is indirectly called via removeForBlock in ConnectTip before new call emplacement of BlockConnected. This order holds same for DisconnectTip.
This order guarantee us than when wallet query the chain to know if it’s behind (BlockUntilSyncedToCurrentChain) it’s either wallet tip is equivalent to chain tip and there is not mempool update to wait for or wallet tip is lagging behind but catching up will only happen after TransactionsRemovedFromMempool has been drought out of the scheduler queue.
I read again wallet code, as far as I can tell, transactionsRemovedFromMempool only tweak mempool flag for wallet transactions which is only used at broadcast but not for block connection.
Or maybe I miss something, code isn’t the clearest here ?
UpdateMempoolForReorg?
Thanks for pointing this out Marco. You’re right that in a re-org, this changes the order of notification. If there’s a one block diconnect/two block connect reorg, then before this PR:
After this PR
So at the point that BlockConnected is called for B2, the mempool is in an inconsistent state. As Marco points out, that should be ok since the validationinterface doesn’t guarantee a consistent mempool when BlockConnected is fired. Listeners should use UpdatedBlockTip if they want that.
However, I’m going to close this PR since it’s difficult to review. It should be possible to bring all of the notification firing into ActivateBestChainStep() in a future PR, but to minimize review burden I’ll open an alternative that doesn’t remove ConnectTrace but instead just simplifies it and improves commenting.