utACK

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.

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?

@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.

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.

@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.

@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.

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.

utACK. Won't hurt.

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)

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?

Needs rebase

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

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

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.

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.

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

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.

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

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