Validation: Remove ConnectTrace and PerBlockConnectTrace #17562

jnewbery · 2019-11-22T19:27:22Z

ConnectTrace was introduced in 6fdd43b
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().

DrahtBot · 2019-11-22T23:42:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

ariard

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.

jnewbery · 2019-11-26T14:21:04Z

Thanks @ariard . I agree with you about the commit message. I've reworded along the lines you suggested.

jonatack

Concept ACK.

Nice simplification and code removal that builds on the work in #17477.

jonatack · 2020-01-22T14:10:00Z

src/validation.cpp

If you need to retouch this PR: perhaps add code documentation here like that for GetMainSignals().BlockDisconnected (or GetMainSignals().ChainStateFlushed or GetMainSignals().UpdatedBlockTip)

git grep -C1 "0-confirmed or conflicted"

The docs are here: https://doxygen.bitcoincore.org/class_c_validation_interface.html#ac63500b25a37b34ad2ec234fde239405

fjahr · 2020-01-22T16:57:16Z

Concept ACK. Change seems fine but will wait with code review until #17477 is finalized and merged.

jnewbery · 2020-03-13T20:57:19Z

Closing for now. No need for this to be open while #17477 is not yet merged. Will reopen when #17477 is merged.

ConnectTrace was introduced in 6fdd43b 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().

maflcko · 2020-03-19T21:30:01Z

Run ci

maflcko · 2020-03-19T21:32:10Z

There is a GitHub corruption: GitHub payload is missing a merge commit (mergeable_state: "blocked", merged: false)

maflcko · 2020-03-19T21:33:15Z

Ok, this was stupid. Ci is running now

Concept ACK

jnewbery · 2020-03-22T00:58:48Z

#17477 is now merged. Re-opened this PR and rebased it on master.

maflcko

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 312d27b. 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 ab00be0, removing code is my favorite! Though, needs some doc updated, I think 🎌

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Concept ACK ab00be08e8e4371bf1483d139ecb50dacba40e0a, removing code is my favorite! Though, needs some doc updated, I think 🎌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi1Xwv8C0bXBK8kwoBLkKTv34M+YT29ESnNrB2kRI95LUZ1J/sXyzIR7nU39wZz
y+/w7cGwxVAITspBSctzKsu42BmrQLs9h4YVYLZLJEmoybHggrvm6c331xMwPexQ
Ktcozmee8qH5i+B0Pqyusk39Jnq/j6S2pm9ahgCh0woypucZWh27Rrjfg/YAgV6i
tbnhe0SY/FFffCH2d9vCFy/HPfPy+ZUSKQOLjY06P+DAJA7NBFzmddKMR7pOp+NB
jGA4RzcqYqACBmjh8WabTWoDf2hgYMyklMYe/HyaUE0Vg1WtYjJf3qapeeFtkMfx
iiGoO+HiASHvqi18Q0489o6nuDhWCIqpKkXl18+hdWvSbiNkySqd7KeWfPEU6s9G
SbEuImgxMJa7KbWkPlsjZxJKPStj8EP9zQr1NaVKt85E+MTCQAgrLj5kZL0/g9Ra
IOfg2RntWbOOqyE3AlT376IAk3sMvhzJZDxWQGhbRMQ4ew1ut+phojsylFGDNyVB
pJVLAPyQ
=fKV2
-----END PGP SIGNATURE-----

Timestamp of file with hash 33cca684633289f50bf74baf0a39711a82c5a0b6fcd6d62d0dc47653f74fa2fa -

maflcko · 2020-03-23T18:00:54Z

src/validation.cpp

-        // blocks are added after all the conflicted transactions have
-        // been filled in. Thus, the last entry should always be an empty
-        // one waiting for the transactions from the next block. We pop
-        // the last entry here to make sure the list we return is sane.


Note to other reviewers: This comment obviously no longer applies after 312d27b

maflcko · 2020-03-23T19:25:50Z

src/validation.cpp

- * The block is added to connectTrace if connection succeeds.
 */
-bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
+bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)


style-nit:

Suggested change

bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)

bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions& disconnectpool)

maflcko

Looks like the wallet is using BlockConnected to determine when it is synced (BlockUntilSyncedToCurrentChain()). This is broken now.

ariard

Code Review ACK ab00be0

@MarcoFalke can you describe your point further ? IIRC BlockUntilSyncedToCurrentChain waits until scheduler queue being drained before letting the wallet moving forward. From where we shot BlockConnected shouldn't change anything ?

ariard · 2020-03-31T01:22:17Z

src/validation.cpp

        {
-            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
+            // Do not unlock cs_main or mempool until we've made forward
+            // progress (with the exception of shutdown due to hardware issues,


nit: define forward progress (chain tip is the valid most-work chain or most-work chain has been found invalid)

ariard · 2020-03-31T01:32:01Z

src/validation.cpp


-                for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
-                    assert(trace.pblock && trace.pindex);
-                    GetMainSignals().BlockConnected(trace.pblock, trace.pindex);


Does it change anything for BlockConnected subscribers ? In case of block connections burst or reorgs, instead of receiving the diff at once after full validation, subscribers will now receive block connection after each individual validation in ConnectTip (assuming scheduler catch up). I think it's slightly better because discrepancy is shorter but likely a really marginal case.

maflcko · 2020-03-31T03:17:36Z

@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 (review)

ariard · 2020-04-01T01:52:45Z

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 ?

maflcko · 2020-04-13T22:02:55Z

What about the ones in UpdateMempoolForReorg?

jnewbery · 2020-04-17T13:42:30Z

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:

Block A1 disconnected (txs added to disconnect pool)
- BlockDisconnected notification for A1
Block A2 connected (block added to ConnectTrace)
- TransactionRemovedFromMempool notifications for conflicted transactions
Block B2 connected (block added to ConnectTrace)
- TransactionRemovedFromMempool notifications for conflicted transactions
UpdateMempoolForReorg called
- TransactionRemovedFromMempool notifications for reorg-removed transactions
ActivateBestChainStep exits
- BlockConnected notification for A2
- BlockConnected notification for B2

After this PR

Block A1 disconnected (txs added to disconnect pool)
- BlockDisconnected notification for A1
Block A2 connected (block added to ConnectTrace)
- TransactionRemovedFromMempool notifications for conflicted transactions
- BlockConnected notification for A2
Block B2 connected (block added to ConnectTrace)
- TransactionRemovedFromMempool notifications for conflicted transactions
- BlockConnected notification for B2
UpdateMempoolForReorg called
- TransactionRemovedFromMempool notifications for reorg-removed transactions
ActivateBestChainStep exits

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.

jnewbery · 2020-04-17T14:40:38Z

Opened a PR to simplify ConnectTrace here: #18685

fanquake added the Validation label Nov 22, 2019

jnewbery mentioned this pull request Nov 22, 2019

Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals #17477

Merged

ariard reviewed Nov 25, 2019

View reviewed changes

jnewbery force-pushed the 2019-11-remove-connect-trace branch from 6b04c24 to aa882b2 Compare November 26, 2019 14:21

jnewbery changed the title ~~Validation: Remove ConnectTrace and PerBlockConnectTrace~~ WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace Jan 2, 2020

DrahtBot added the Needs rebase label Jan 9, 2020

jonatack reviewed Jan 22, 2020

View reviewed changes

jnewbery closed this Mar 13, 2020

jnewbery reopened this Mar 19, 2020

jnewbery force-pushed the 2019-11-remove-connect-trace branch from aa882b2 to ab00be0 Compare March 19, 2020 20:46

maflcko closed this Mar 19, 2020

maflcko reopened this Mar 19, 2020

maflcko closed this Mar 19, 2020

maflcko reopened this Mar 19, 2020

DrahtBot removed the Needs rebase label Mar 19, 2020

jnewbery changed the title ~~WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace~~ Validation: Remove ConnectTrace and PerBlockConnectTrace Mar 22, 2020

maflcko mentioned this pull request Mar 23, 2020

Check for successful runs of all checks bitcoin-core/bitcoin-maintainer-tools#54

Closed

maflcko approved these changes Mar 23, 2020

View reviewed changes

maflcko reviewed Mar 23, 2020

View reviewed changes

maflcko suggested changes Mar 27, 2020

View reviewed changes

ariard approved these changes Mar 31, 2020

View reviewed changes

DrahtBot mentioned this pull request Apr 14, 2020

assumeutxo #15606

Closed

18 tasks

jnewbery closed this Apr 17, 2020

jnewbery mentioned this pull request Apr 17, 2020

consensus: Simplify ConnectTrace #18685

Closed

jnewbery mentioned this pull request Jan 25, 2021

validation: Remove ConnectTrace and PerBlockConnectTrace jnewbery/bitcoin#15

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)
	bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions& disconnectpool)

Validation: Remove ConnectTrace and PerBlockConnectTrace #17562

Validation: Remove ConnectTrace and PerBlockConnectTrace #17562

Uh oh!

Conversation

jnewbery commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 26, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jan 22, 2020

Uh oh!

jnewbery commented Mar 13, 2020

Uh oh!

maflcko commented Mar 19, 2020

Uh oh!

maflcko commented Mar 19, 2020

Uh oh!

maflcko commented Mar 19, 2020

Uh oh!

jnewbery commented Mar 22, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Apr 1, 2020

Uh oh!

maflcko commented Apr 13, 2020

Uh oh!

jnewbery commented Apr 17, 2020

Uh oh!

jnewbery commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jnewbery commented Nov 22, 2019 •

edited

Loading

DrahtBot commented Nov 22, 2019 •

edited

Loading

jonatack Jan 22, 2020 •

edited

Loading

maflcko commented Mar 31, 2020 •

edited

Loading