Followups for #30409 waitTipChanged() #30966

1. 
   Sjors commented at 8:15 am on September 25, 2024: member

   Based on post-merge comments on #30409.
2. kernel: guard m_tip_block 7fa252e86a
3. refactor: drop useless overflow check

   An earlier version of this code was written to handle the case where now + timeout calculation would overflow, basically assuming both values were unsigned integer values and overflow behavior would be well defined.
   
   But since timeout is a floating point number where "overflow" would just result in inf being set, trying to deal with it this way doesn't make sense.
   
   Co-authored-by:  Ryan Ofsky <ryan@ofsky.org>

   93ad641d6c
4. Use assert in StopRPC

   This will print a more usable error message. EnsureAnyNodeContext is intended for RPC method implementations.
   
   Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

   ceb9151c87
5. 
   DrahtBot commented at 8:15 am on September 25, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

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

6. 
   Sjors commented at 8:17 am on September 25, 2024: member

   @theuni also wrote:

   Related: It’s a shame that the condvar/mutex leak out of here. Is there a reason not to add getter/waiter/notifier functions?

   Not sure how to go about that, but happy to take a patch.

7. in src/test/validation_chainstate_tests.cpp:144 in ceb9151c87

   140@@ -139,7 +141,8 @@ BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
   141     // g_best_block should be unchanged after adding a block to the background
   142     // validation chain.
   143     BOOST_CHECK(block_added);
   144-    BOOST_CHECK_EQUAL(curr_tip, m_node.notifications->m_tip_block);
   145+    new_tip = WITH_LOCK(m_node.notifications->m_tip_block_mutex, return m_node.notifications->m_tip_block;);
   

   ---

   ---

   

   maflcko commented at 8:56 am on September 25, 2024:

   Seems verbose to lock this every time. Also, the comment above is still wrong. It would be good to drop this commit, or replace it with fa7dd1d072dc36997e2d5d702e2da529a093c90f, which also fixes up the outdated comment.
8. in src/rpc/server.cpp:307 in ceb9151c87

   303@@ -303,7 +304,7 @@ void StopRPC(const std::any& context)
   304         LogDebug(BCLog::RPC, "Stopping RPC\n");
   305         WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear());
   306         DeleteAuthCookie();
   307-        node::NodeContext& node = EnsureAnyNodeContext(context);
   308+        node::NodeContext& node = *Assert(util::AnyPtr<node::NodeContext>(context));
   

   ---

   ---

   

   maflcko commented at 8:58 am on September 25, 2024:

   Seems overly complicated to turn a reference into a pointer, then turn it into std::any, then recover it from any, then assert the pointer isn’t null.

   It would be better to remove all of this and just pass the reference.

   So it would be good to drop this commit or replace it with fadd531a57710809df0e4b027ce30b5524e5f40f, which does the above.

9. 
   maflcko commented at 9:00 am on September 25, 2024: member

   I’ve opened #30967, which includes the follow-ups here, but I solved them in another way.

   Feel free to take or leave them. I can drop them from mine, once this one is merged.

10. 
    Sjors commented at 10:30 am on September 25, 2024: member

    Let’s go with #30967.
11. Sjors closed this on Sep 25, 2024

    ---

Contributors
Sjors DrahtBot maflcko