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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31297.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
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.
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 | +
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.
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 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.
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:
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.
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) {
435934fa020cec584b49c6151aa6b0d858054b37 oops, that should have been &&
Pushed a bug fix, but still need to address the main feedback.
Also, it would be good to add test coverage?
@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.
Are you sure it is covered, because the code is reported as uncovered in https://corecheck.dev/bitcoin/bitcoin/pulls/31297?
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:
So I was probably doing something wrong earlier.
I dropped the workaround. An early return only happens if:
m_tip_block is set; ANDm_tip_block.value() is different from current_blockIf 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.
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) {
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.
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.
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.
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.
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.