mining: add early return to waitTipChanged() #31297

Sjors · 2024-11-15T17:32:34Z

It's useful to be able to wait for an active chaintip to appear, by calling waitTipChanged(uint256::ZERO);.

Unfortunately this doesn't work out of the box, because notifications().m_tip_block is ZERO until a new block arrives.

Additionally we need an early return before calling wait_for.

DrahtBot · 2024-11-15T17:32:38Z

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/31297.

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)

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.

Sjors · 2024-11-15T17:33:47Z

cc @TheCharlatan life would be siugnifancly better if notifications().m_tip_block just jumped to the tip as soon as there is one, rather than wait for the next block to come in. That applies to #31283 too.

maflcko · 2024-11-15T17:46:48Z

src/node/interfaces.cpp

+                return BlockRef{tip_hash, chainman().ActiveChain().Tip()->nHeight};
+            }
+        }
+


I wonder if it is clearer to make m_tip_block nullopt, instead of a magic value? Would be nice to see the diff and pick the better one.

ryanofsky · 2024-11-15T18:31:52Z

It's useful to be able to wait for an active chaintip to appear, by calling waitTipChanged(uint256::ZERO);.

I think the bug here is just that blockTip() is not called on startup so m_tip_block is not set when node starts up, only after it starts up and then a new block get connected. Otherwise the waitTipChanged method is already written to provide desired behavior by waiting until there is any new tip and the new tip is not zero:

            notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
                return (notifications().m_tip_block != current_tip && notifications().m_tip_block != uint256::ZERO) || chainman().m_interrupt;
            });

I don't actually see how the early return if (current_tip != uint256::ZERO || current_tip != tip_hash) suggested in 435934f helps because that will be trivially true unless current_tip and tip_hash are both 0. But maybe if it said (tip_hash != uint256::ZERO && current_tip != tip_hash) that could be a partial workaround. It would still wait too long if waitTipChanged were called really early before the chain tip was loaded, but would work well as long as waitTipChanged were called after that point.

But It would seem cleaner to just call blockTip() when the tip is first loaded so no workaround is needed.

Sjors · 2024-11-15T19:52:15Z

that will be trivially true unless current_tip and tip_hash are both 0

That's how I use it in Sjors#49 when the Template Provider spins up its main thread:

void Sv2TemplateProvider::ThreadSv2Handler()
{
    // Wait for the node chainstate to be ready if needed.
    auto tip{m_mining.waitTipChanged(uint256::ZERO)};
    Assert(tip.hash != uint256::ZERO);

Update 2024-11-18: just noticed || instead of &&

But It would seem cleaner to just call blockTip() when the tip is first loaded so no workaround is needed.

I tend to agree.

Sjors · 2024-11-18T11:10:55Z

src/node/interfaces.cpp

+        {
+            LOCK(::cs_main);
+            const uint256 tip_hash{chainman().ActiveChain().Tip()->GetBlockHash()};
+            if (current_tip != uint256::ZERO || current_tip != tip_hash) {


435934f oops, that should have been &&

Sjors · 2024-11-18T11:15:02Z

Pushed a bug fix, but still need to address the main feedback.

maflcko · 2024-11-18T11:23:14Z

Also, it would be good to add test coverage?

Sjors · 2024-11-18T11:36:39Z

@maflcko it's implicitly tested by the shutdown test, which is how I found this bug. Not sure how to explicitly test an early return? I could add a comment to the shutdown test that the ['active_commands']) == 2 condition will fail if waitfornewblock returned prematurely.

maflcko · 2024-11-18T11:41:09Z

Are you sure it is covered, because the code is reported as uncovered in https://corecheck.dev/bitcoin/bitcoin/pulls/31297?

Sjors · 2024-11-18T11:43:38Z

@maflcko feature_shutdown.py only checks that the early return is not used. It makes sense to test the scenario where it is used too.

Sjors · 2024-11-19T13:26:41Z

I added a commit to make m_tip_block and std::optional.

Also, looking at m_tip_block again, I'm fairly sure it's set to the tip during node initialization. Otherwise we'd never get past this point:

bitcoin/src/init.cpp

Lines 1794 to 1806 in 746f93b

    
           // Wait for genesis block to be processed 
        
           if (WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip() == nullptr)) { 
        
               WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock); 
        
               kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) { 
        
                   return !kernel_notifications.m_tip_block.IsNull() || ShutdownRequested(node); 
        
               }); 
        
           } 
        
           if (ShutdownRequested(node)) { 
        
               return false; 
        
           } 
        
           // ********************************************************* Step 12: start node

So I was probably doing something wrong earlier.

I dropped the workaround. An early return only happens if:

m_tip_block is set; AND
m_tip_block.value() is different from current_block

If a caller sets current_tip to the special value ZERO very early in the node startup process, it will never match m_tip_block.value(), so it won't return early.

I'm still not sure how to add a functional test for this.

ryanofsky · 2024-11-19T13:54:10Z

src/node/interfaces.cpp

-                return (notifications().m_tip_block && notifications().m_tip_block.value() != current_tip) || chainman().m_interrupt;
-            });
+            // Early return if a tip is connected and already different.
+            if (!notifications().m_tip_block || notifications().m_tip_block.value() == current_tip) {


In commit "mining: add early return to waitTipChanged()" (71a539c)

Does this change have any effect? It looks like this is basically changing cv.wait_for(condition) to if (!condition) cv.wait_for(condition) which I would expect to be equivalent.

I'm assuming wait_for won't be triggered until m_tip_block is set or modified, or the timeout passes. So it doesn't return early.

In other words:

if !tip_changed wait_for: tip_changed return new_tip

Are you are seeing a difference in practice? At least according to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for, wait_for(lock, rel_time, pred) is equivalent to wait_until(lock, rel_time + now, pred) and according to https://en.cppreference.com/w/cpp/thread/condition_variable/wait_until that should be equivalent to while(!pred()) wait... checking the predicate before there is any waiting.

I thought I did, but given what the documentation says, it indeed shouldn't be needed. Will test again.

ryanofsky · 2024-11-19T18:32:20Z

re: #31297 (comment)

Otherwise we'd never get past this point:

I think we typically get past that point on line 1795 in AppInitMain even though m_tip_block is still null at that point, because when the node restarted the chainman.ActiveTip() == nullptr check will normally be false. This is because of the InitAndLoadChainstate call on line 1624, which calls Chainstate::LoadChainTip() and sets the active tip to coins_cache.GetBestBlock().

I think maybe a good fix for this issue could be to call GetNotifications().blockTip() in LoadChainTip() or maybe to just set m_tip_block directly.

It would be nice if we could have a functional test to reproduce the exact problem, but that is hard as long as python code doesn't have an easy way to call the waitTipChanged() method. It would probably be easier to test this after #30437 we have a bitcoin-mine executable that python code could run, and could test various mining methods.

Sjors · 2024-11-19T19:07:03Z

It's a bit tricky at the moment to test the interaction of multiple pull requests on my stratum v2 branches. I'll test this again later, and most likely close the PR then. See #31297 (comment)

Meanwhile I extracted the make m_tip_block std::optional commit into #31325 for separate consideration.

Sjors · 2024-11-22T11:37:52Z

I tested that m_tip_block_cv.wait_for indeed immediately checks the condition before waiting. And I could see that notifications().m_tip_block isn't set when you start a node that is already fully caught up.

I'll open a new PR to try and deal with that.

Sjors · 2024-11-22T12:13:20Z

See #31346.

@mzumsande

37946c0 Set notifications m_tip_block in LoadChainTip() (Sjors Provoost) Pull request description: Ensure KernelNotifications `m_tip_block` is set even if no new block arrives. Suggested in #31297 (comment) ACKs for top commit: ryanofsky: Code review ACK 37946c0, fixing comment bug caught by @mzumsande in #31346 (comment) in another really helpful clarification mzumsande: Code Review ACK 37946c0 TheCharlatan: ACK 37946c0 Tree-SHA512: 931bf820440a0cdda276f6dbd63f03fdbcdc90b18e7d5e160a74bdd9d0290acc706c35aab15bbdcd6e5e0b77565b3d07ff49b0dcf6551cb83961bae67be5d1bb

81cea5d Ensure m_tip_block is never ZERO (Sjors Provoost) e058544 Make m_tip_block an std::optional (Sjors Provoost) Pull request description: Suggested in #31297 (comment) ACKs for top commit: fjahr: re-ACK 81cea5d tdb3: code review re ACK 81cea5d l0rinc: ACK 81cea5d Tree-SHA512: 31a75ba29e3d567bab32e4e7925a419d9d7a4d2d85ed1c1012116d8d22adc14d31d5b4ce5f6c499c994188dcd26a01cced05be74f94c892fc90ae17a6783a472

DrahtBot added the Mining label Nov 15, 2024

Sjors mentioned this pull request Nov 15, 2024

Add waitNext() to BlockTemplate interface #31283

Merged

maflcko reviewed Nov 15, 2024

View reviewed changes

This was referenced Nov 15, 2024

Stratum v2 via IPC Mining Interface tracking issue #31098

Open

Stratum v2 Template Provider common functionality Sjors/bitcoin#49

Closed

DrahtBot added the CI failed label Nov 15, 2024

Sjors commented Nov 18, 2024

View reviewed changes

Sjors force-pushed the 2024/11/waittipchanged branch from 435934f to 71a539c Compare November 18, 2024 11:14

DrahtBot removed the CI failed label Nov 18, 2024

Sjors added 2 commits November 19, 2024 13:18

kernel: make m_tip_block std::optional

e553d35

mining: add early return to waitTipChanged()

8a15e46

Sjors force-pushed the 2024/11/waittipchanged branch from 71a539c to 8a15e46 Compare November 19, 2024 13:30

ryanofsky reviewed Nov 19, 2024

View reviewed changes

Sjors marked this pull request as draft November 19, 2024 18:44

Sjors mentioned this pull request Nov 19, 2024

Make m_tip_block std::optional #31325

Merged

Sjors closed this Nov 22, 2024

Sjors mentioned this pull request Nov 22, 2024

Set notifications m_tip_block in LoadChainTip() #31346

Merged

mining: add early return to waitTipChanged() #31297

mining: add early return to waitTipChanged() #31297

Uh oh!

Conversation

Sjors commented Nov 15, 2024

Uh oh!

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

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Sjors commented Nov 15, 2024

Uh oh!

maflcko Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 15, 2024

Uh oh!

Sjors commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 18, 2024

Uh oh!

maflcko commented Nov 18, 2024

Uh oh!

Sjors commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 18, 2024

Uh oh!

Sjors commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 19, 2024 • 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!

ryanofsky commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 22, 2024

Uh oh!

Sjors commented Nov 22, 2024

Uh oh!

Uh oh!

DrahtBot commented Nov 15, 2024 •

edited

Loading

Sjors commented Nov 15, 2024 •

edited

Loading

Sjors commented Nov 18, 2024 •

edited

Loading

Sjors commented Nov 18, 2024 •

edited

Loading

Sjors commented Nov 19, 2024 •

edited

Loading

Sjors Nov 19, 2024 •

edited

Loading

ryanofsky commented Nov 19, 2024 •

edited

Loading

Sjors commented Nov 19, 2024 •

edited

Loading