Followups for #30409 waitTipChanged() #30966

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2024/09/30409-followups changing 4 files +13 −11
  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


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-10-08 07:12 UTC

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