mining: add early return to waitTipChanged() #31297

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2024/11/waittipchanged changing 4 files +13 −8
  1. Sjors commented at 5:32 pm on November 15, 2024: member

    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.

  2. DrahtBot commented at 5:32 pm on November 15, 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/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.

  3. DrahtBot added the label Mining on Nov 15, 2024
  4. Sjors commented at 5:33 pm on November 15, 2024: member
    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.
  5. in src/node/interfaces.cpp:982 in 435934fa02 outdated
    977+            const uint256 tip_hash{chainman().ActiveChain().Tip()->GetBlockHash()};
    978+            if (current_tip != uint256::ZERO || current_tip != tip_hash) {
    979+                return BlockRef{tip_hash, chainman().ActiveChain().Tip()->nHeight};
    980+            }
    981+        }
    982+
    


    maflcko commented at 5:46 pm on November 15, 2024:
    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.
  6. ryanofsky commented at 6:31 pm on November 15, 2024: contributor

    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:

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

    I don’t actually see how the early return if (current_tip != uint256::ZERO || current_tip != tip_hash) suggested in 435934fa020cec584b49c6151aa6b0d858054b37 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.

  7. DrahtBot added the label CI failed on Nov 15, 2024
  8. Sjors commented at 7:52 pm on November 15, 2024: member

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

    That’s how I use it in https://github.com/Sjors/bitcoin/pull/49 when the Template Provider spins up its main thread:

    0void Sv2TemplateProvider::ThreadSv2Handler()
    1{
    2    // Wait for the node chainstate to be ready if needed.
    3    auto tip{m_mining.waitTipChanged(uint256::ZERO)};
    4    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.

  9. in src/node/interfaces.cpp:978 in 435934fa02 outdated
    969@@ -970,6 +970,17 @@ class MinerImpl : public Mining
    970     BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
    971     {
    972         if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in std::chrono
    973+
    974+        // Early return if the tip is already different.
    975+        {
    976+            LOCK(::cs_main);
    977+            const uint256 tip_hash{chainman().ActiveChain().Tip()->GetBlockHash()};
    978+            if (current_tip != uint256::ZERO || current_tip != tip_hash) {
    


    Sjors commented at 11:10 am on November 18, 2024:
    435934fa020cec584b49c6151aa6b0d858054b37 oops, that should have been &&
  10. Sjors force-pushed on Nov 18, 2024
  11. Sjors commented at 11:15 am on November 18, 2024: member
    Pushed a bug fix, but still need to address the main feedback.
  12. maflcko commented at 11:23 am on November 18, 2024: member
    Also, it would be good to add test coverage?
  13. Sjors commented at 11:36 am on November 18, 2024: member
    @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.
  14. maflcko commented at 11:41 am on November 18, 2024: member
    Are you sure it is covered, because the code is reported as uncovered in https://corecheck.dev/bitcoin/bitcoin/pulls/31297?
  15. Sjors commented at 11:43 am on November 18, 2024: member
    @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.
  16. DrahtBot removed the label CI failed on Nov 18, 2024
  17. kernel: make m_tip_block std::optional e553d350c6
  18. mining: add early return to waitTipChanged() 8a15e46c68
  19. Sjors commented at 1:26 pm on November 19, 2024: member

    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:

    https://github.com/bitcoin/bitcoin/blob/746f93b4f0f47c67642057944fb79dddf17369f9/src/init.cpp#L1794-L1806

    So I was probably doing something wrong earlier.

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

    1. m_tip_block is set; AND
    2. 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.

  20. Sjors force-pushed on Nov 19, 2024
  21. in src/node/interfaces.cpp:977 in 8a15e46c68
    975             WAIT_LOCK(notifications().m_tip_block_mutex, lock);
    976-            notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
    977-                return (notifications().m_tip_block && notifications().m_tip_block.value() != current_tip) || chainman().m_interrupt;
    978-            });
    979+            // Early return if a tip is connected and already different.
    980+            if (!notifications().m_tip_block || notifications().m_tip_block.value() == current_tip) {
    


    ryanofsky commented at 1:54 pm on November 19, 2024:

    In commit “mining: add early return to waitTipChanged()” (71a539c5c3ae165be71a5c8d7f5b1860f8261bd1)

    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.


    Sjors commented at 2:12 pm on November 19, 2024:
    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.

    Sjors commented at 4:01 pm on November 19, 2024:

    In other words:

    0if !tip_changed
    1  wait_for: tip_changed
    2return new_tip
    

    ryanofsky commented at 6:01 pm on November 19, 2024:
    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.

    Sjors commented at 6:44 pm on November 19, 2024:
    I thought I did, but given what the documentation says, it indeed shouldn’t be needed. Will test again.
  22. ryanofsky commented at 6:32 pm on November 19, 2024: contributor

    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.

  23. Sjors marked this as a draft on Nov 19, 2024
  24. Sjors commented at 7:07 pm on November 19, 2024: member

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

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

  25. Sjors commented at 11:37 am on November 22, 2024: member

    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.

  26. Sjors closed this on Nov 22, 2024

  27. Sjors commented at 12:13 pm on November 22, 2024: member
    See #31346.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me