Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace #17562

Open
wants to merge 7 commits into
base: master
from

Conversation

@jnewbery
Copy link
Member

jnewbery commented Nov 22, 2019

Builds on #17477. Please review that PR first.

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().

jnewbery added 6 commits Nov 11, 2019
The only CValidationInterface client that cares about transactions that
are removed from the mempool because of CONFLICT is the wallet.

Start using the TransactionRemovedFromMempool method to notify about
conflicted transactions instead of using the vtxConflicted vector in
BlockConnected.
The wallet now uses TransactionRemovedFromMempool to be notified about
conflicted wallet, and no other clients use vtxConflicted.
Since we don't add a vtxConflicte vector to BlockConnected the
conflictedTxs member of PerBlockConnectTrace is no longer used.
ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved
callback to be notified of transactions removed for conflict. Since
PerBlockConnectTrace no longer tracks conflicted transactions,
ConnectTrace no longer requires these notifications.
NotifyEntryAdded never had any subscribers so can be removed.

Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.

The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
It's no longer used for anything.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 22, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17477 (Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals by jnewbery)
  • #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
  • #16688 (log: Add validation interface logging by jkczyz)
  • #15606 ([experimental] UTXO snapshots by jamesob)

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.

Copy link
Contributor

ariard left a comment

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.

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().
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Nov 26, 2019

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

@jnewbery jnewbery force-pushed the jnewbery:2019-11-remove-connect-trace branch from 6b04c24 to aa882b2 Nov 26, 2019
@jnewbery jnewbery changed the title Validation: Remove ConnectTrace and PerBlockConnectTrace WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace Jan 2, 2020
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 9, 2020

Needs rebase
Copy link
Member

jonatack left a comment

Concept ACK.

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

@@ -2562,7 +2520,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime5) * MILLI, nTimePostConnect * MICRO, nTimePostConnect * MILLI / nBlocksTotal);
LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime1) * MILLI, nTimeTotal * MICRO, nTimeTotal * MILLI / nBlocksTotal);

connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
GetMainSignals().BlockConnected(pthisBlock, pindexNew);

This comment has been minimized.

Copy link
@jonatack

jonatack Jan 22, 2020

Member

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"
@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Jan 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.