Move the blocknotify callback ahead of peer announcement.

gmaxwell commented at 7:13 am on November 17, 2015: contributor

Wizkid057 reported getbestblockhash polling was frequently beating blocknotify, sometimes by 2 seconds. Under the theory that getting cs_vNodes and running running PushInventory for all peers was sometimes taking a while or UpdatedBlockTip (see below), I suggested moving up the notify. He reported doing so made it consistently faster.

I think it’s reasonable to signal blocknotify first: All it does is fire off a new thread, so it should never block.

This does not move up GetMainSignals().UpdatedBlockTip() which because that signal includes the wallet, and if there is a wallet with many keys the wallet processing of a block can be quite slow. Unfortunately, it also sends out the ZMQ notify.

Wizkid057 reported getbestblockhash polling was frequently beating
 blocknotify, sometimes by 2 seconds.  Under the theory that getting
 cs_vNodes and running running PushInventory for all peers was
 sometimes taking a while or UpdatedBlockTip (see below), I
 suggested moving up the notify.  He reported doing so made it
 consistently faster.

I think it's reasonable to signal blocknotify first: All it does
 is fire off a new thread, so it should never block.

This does not move up GetMainSignals().UpdatedBlockTip() which
 because that signal includes the wallet, and if there is a
 wallet with many keys the wallet processing of a block can
 be quite slow. Unfortunately, it also sends out the ZMQ notify.

c7121a548b

gmaxwell added the label Mining on Nov 17, 2015

dcousens commented at 7:31 am on November 17, 2015: contributor

utACK

promag commented at 11:11 am on November 17, 2015: member

It’s strange to have the same notification in two different moments.

Can we make the inventory relay asynchronous and as a slot connected to GetMainSignals().UpdatedBlockTip()?

In other words, I don’t see these kind of tweaks the right way to go, instead a refactor to fix these long operations.

sdaftuar commented at 2:33 pm on November 17, 2015: member

I agree that it’s strange to have notifications effectively three times, but I think we should wait to refactor until after #6494, which is tagged for 0.12.

Also I’d be surprised if it turns out the p2p block relay operations are slow, but I haven’t timed yet; I’ll do some analysis. Anyway what’s the reasoning behind having two different notification systems (UpdatedBlockTip and NotifyBlockTip)? Is there a reason we can’t or shouldn’t consolidate down to one callback system for block notifications?

sipa commented at 2:39 pm on November 17, 2015: member

@sdaftuar One was historically for the wallet and the other for the GUI, but I don’t think there is any reason to keep both now.

morcos commented at 2:59 pm on November 17, 2015: member

Don’t forget cvBlockChange which is what lets getblocktemplate know the tip has changed and is called on every block connected or disconnected. I haven’t thought about it until just now, but does it make sense to be calling that inside of ActivateBestChainStep? I guess it won’t be able to do anything until ActivateBestChainStep releases cs_main, at which point it may have received several notifications in a reorg.

promag commented at 3:16 pm on November 17, 2015: member

@sdaftuar it would be great to have that analysis. Still, any slot connected to UpdatedBlockTip can be blocking causing RPC to wait for the lock.

gmaxwell commented at 4:32 am on November 18, 2015: contributor

@promag They’re not in different moments, there is a sequence of things that get notified. And this patch moves the last to the first because when present it’s often the most latency critical and it won’t block.

This is sufficient to make blocknotify faster. When the wallet fires is not latency critical. Fortunately they already use different mechanisms, so it was easy to order them.

gmaxwell commented at 10:44 pm on November 22, 2015: contributor

The discussion above leaves me dangling. I think we should do this because its a simple and effectively free performance improvement… that it wouldn’t be possible if everything were using the same signals, is IMO just an example of the limitations in that particular approach; but while we do, we should take the benefit available.

sipa commented at 9:28 pm on November 26, 2015: member

utACK. Won’t hurt.

laanwj commented at 12:56 pm on November 27, 2015: member

I don’t care about having two signals.

I do think the zmq notify should use this as well. Having the old launch-a-process have less latency than zmq, which we supposedly added to have a lower latency notification mechanism, just isn’t acceptable :) (especially if we’re talking about seconds here)

gmaxwell commented at 11:28 am on November 28, 2015: contributor

AFAICT ZMQ and the wallet ride on the same notification, and there appears to be no way to control what order they execute in. Suggestions?

dcousens commented at 4:06 am on November 30, 2015: contributor

Needs rebase

jonasschnelli commented at 7:20 am on November 30, 2015: contributor

IMO we should remove uiInterface.NotifyBlockTip() and use CMainSignals.UpdatedBlockTip() for the blocknotify execution.

dcousens commented at 7:48 am on November 30, 2015: contributor

Agreed with @jonasschnelli, but that will require a larger code change…

laanwj commented at 10:20 am on November 30, 2015: member

IMO we should remove uiInterface.NotifyBlockTip() and use CMainSignals.UpdatedBlockTip() for the blocknotify execution.

For this to work, no signal handlers should be executing anything expensive. The problem is that the wallet does this, resulting in the problem that this pull tries to mitigate. That should be solved first.

sipa commented at 11:57 am on November 30, 2015: member

This is possibly an issue if wallet-based services use this signal to trigger wallet RPCs, as those may complete before the wallet was notified about the change.

dcousens commented at 0:37 am on December 1, 2015: contributor

@sipa then perhaps the wallet should have a ‘block’ notification of its own, as, conceptually, that is what you’re saying. (Users are waiting on both the new block notification AND the wallet notification of having recognised that block, not the wallet ‘receiving’ the block notification itself)

laanwj commented at 11:25 am on December 1, 2015: member

then perhaps the wallet should have a ‘block’ notification of its own, as, conceptually, that is what you’re saying.

Yes, a ProcessedBlock signal on wallet would make sense (if there’s any users for that? most wallet users care about transactions, not blocks), as handling becomes more asynchronous it’s no longer possible to rely on the fact that core signals mean that anything wallet (or other subsystem) related has happened.

TheBlueMatt commented at 0:50 am on December 2, 2015: member

An equivalent change was merged in #7112. This should be closed.

gmaxwell closed this on Dec 2, 2015

dcousens commented at 3:39 am on December 2, 2015: contributor

We should still think about consolidating the signals to avoid these issues in future. Probably low priority at the moment though.

MarcoFalke locked this on Sep 8, 2021

Move the blocknotify callback ahead of peer announcement. #7037