Set notifications m_tip_block in LoadChainTip()

Sjors commented at 12:13 pm on November 22, 2024: member

Ensure KernelNotifications m_tip_block is set even if no new block arrives.

Suggested in #31297 (comment)

DrahtBot commented at 12:13 pm on November 22, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31346.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31325 (Make m_tip_block std::optional by Sjors)
#31283 (Add waitNext() to BlockTemplate interface by Sjors)
#31177 (rpc, logging: return “verificationprogress” of 1 when up to date by polespinasa)

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.

in src/validation.cpp:4712 in b6d5be198a outdated

4705@@ -4706,6 +4706,11 @@ bool Chainstate::LoadChainTip()
4706               m_chain.Height(),
4707               FormatISO8601DateTime(tip->GetBlockTime()),
4708               GuessVerificationProgress(m_chainman.GetParams().TxData(), tip));
4709+
4710+    // Ensure KernelNotifications m_tip_block is set even if no new block arrives.
4711+    // Ignoring return value for now.
4712+    (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(/*init=*/true, m_chainman.m_blockman.m_blockfiles_indexed), *pindex);

l0rinc commented at 12:17 pm on November 22, 2024:

nit: at this point it may make more sense to just remove the [[nodiscard]] from blockTip instead

Sjors commented at 12:30 pm on November 22, 2024:

Maybe, or we could do something with the return value. I’m not familiar enough with init code and kernel notifications to have a strong opinion on this. cc @ryanofsky, @TheCharlatan

ryanofsky commented at 1:16 pm on November 22, 2024:

Just ignoring the return value is probably the best thing for now since this function and its callers return bool. After #29700 it could more easily bubble up the interrupted status. The kernel API is designed for a better world where operations can be more easily interrupted but not everything is interruptible for now.

maflcko commented at 12:42 pm on November 22, 2024: member

This is incomplete. You’ll have to update the docs as well: “If the tip was not connected on * startup, this will wait.”, and “which may //! be true even long after startup, until shutdown.”. Also, in init.cpp you can remove the if-guard?

Sjors force-pushed on Nov 22, 2024

Sjors commented at 2:19 pm on November 22, 2024: member

I updated the waitTipChanged() and m_tip_block doc. @maflcko which if-guard do you mean?

ryanofsky commented at 2:23 pm on November 22, 2024: contributor

@maflcko which if-guard do you mean?

I think probably this one:

0    // Wait for genesis block to be processed
1    if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip() == nullptr)) {

Removing the if() might help verify that m_tip_block is set reliably

maflcko commented at 2:29 pm on November 22, 2024: member

Also, the ZERO workaround could be removed completely, ensuring that any interfaces will only be spun up after that point in init?

Sjors force-pushed on Nov 22, 2024

Sjors commented at 2:42 pm on November 22, 2024: member

I dropped the if guard.

ensuring that any interfaces will only be spun up after that point in init

It could, but I’m not sure if it’s worth putting such a restriction on the interfaces.

Similarly we have startupnotify which happens at the end of init. If we also ensure that the interfaces are ready to listen before this notification, then an application launched with systemd (or similiar) can wait for this signal before trying to connect.

If on the other hand an external application is started at the same time, it would just have to keep trying to connect until we listen. Such a race would exist even if we opened the interface a bit earlier in the init process, though it would be shorter.

ryanofsky commented at 3:00 pm on November 22, 2024: contributor

It could, but I’m not sure if it’s worth putting such a restriction on the interfaces.

Tend to agree for now. Having waitTipChanged wait for a tip to be connected seems like a natural thing to implement and might avoid the need for clients to poll the node on startup. But I think this check could be dropped in BlockTemplate::waitNext in #31283 (maybe replaced with an assert there) since it should not be possible to get a blocktemplate reference before the tip is connected. After #31283 it might make sense to drop the waitTipChanged method entirely too.

Sjors commented at 4:02 pm on November 22, 2024: member

Several reindex related tests are broken, investigating…

After #31283 it might make sense to drop the waitTipChanged method entirely too.

I think it’s generically useful though. There are various types of applications that need to know when a new block is added to the chain (and have no need for a template).

DrahtBot added the label CI failed on Nov 22, 2024

DrahtBot commented at 4:06 pm on November 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33386413030

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Sjors commented at 4:46 pm on November 22, 2024: member

Specifically -reindex-chainstate is broken, easy to reproduce with e.g. testnet4.

It stops after the second block:

02024-11-22T16:44:14.338986Z Setting NODE_NETWORK on non-prune mode
12024-11-22T16:44:14.339014Z Wait for genesis block to be processed
22024-11-22T16:44:14.339023Z initload thread start
32024-11-22T16:44:14.341109Z UpdateTip: new best=00000000da84f2bafbbc53dee25a72ae507ff4914b867c565be350b0da8bf043 height=0 version=0x00000001 log2_work=32.000022 tx=1 date='2024-05-03T23:11:00Z' progress=0.000001 cache=0.3MiB(0txo)
42024-11-22T16:44:14.341179Z UpdateTip: new best=0000000012982b6d5f621229286b880e909984df669c2afabb102ce311b13f28 height=1 version=0x20000000 log2_work=33.000022 tx=2 date='2024-05-06T13:08:48Z' progress=0.000002 cache=0.3MiB(1txo)

Putting the if-guard back “fixes” it. When I put a log statement before WAIT_LOCK and after the wait function returns, I can see that this lines are called regardless of the if-guard.

Turns out the only critical thin is that the WAIT_LOCK stuff lives in its own scope.

Set notifications m_tip_block in LoadChainTip()

Ensure KernelNotifications m_tip_block is set even if no new block arrives.

Additionally, have node init always wait for this to happen.

3317123aa3

Sjors force-pushed on Nov 22, 2024

DrahtBot removed the label CI failed on Nov 22, 2024

Set notifications m_tip_block in LoadChainTip() #31346

Code Coverage & Benchmarks

Reviews

Conflicts