refactor: replace isPotentialtip/waitForNotifications by higher method #15842

pull ariard wants to merge 2 commits into bitcoin:master from ariard:2019-04-is-potential-tip changing 5 files +29 −37
  1. ariard commented at 12:31 PM on April 18, 2019: member

    In Chain interface, instead of a isPotentialTip and a WaitForNotifications method, both used only once in CWallet::BlockUntilSyncedToCurrentChain, combine them in a higher WaitForNotificationsUpToTip method. Semantic should be unchanged, wallet wait for pending notifications to be processed unless block hash points to the current chain tip or a descendant.

  2. fanquake added the label Refactoring on Apr 18, 2019
  3. jnewbery added the label Wallet on Apr 18, 2019
  4. in src/wallet/wallet.cpp:1295 in 1bcf91d49f outdated 
    1306 | +    // Skip the queue-draining stuff if we know we're caught up with
1307 | +    // chainActive.Tip(), otherwise put a callback in the validation interface queue and wait
1308 |      // for the queue to drain enough to execute it (indicating we are caught up
1309 |      // at least with the time we entered this function).
1310 | -    chain().waitForNotifications();
1311 | +    chain().waitForNotificationsUpToTip(m_last_block_processed);
    jnewbery commented at 2:09 PM on April 18, 2019:

    Accessing m_last_block_processed requires the cs_wallet lock. I think to make this work you'll need to:

    • grab cs_wallet
    • access m_last_block_processed
    • release cs_wallet
    • call the waitForNotificationsUpToTip interface method

    Alternatively, we could make m_last_block_processed an atomic.

    Thoughts, @ryanofsky ?

    ryanofsky commented at 3:39 PM on April 18, 2019:

    Yes, John's right this needs to synchronize access to m_last_block_processed. Since I don't think there's a straightforward way to make uint256 atomic, maybe a clean way to do this would be to add a new sync.h macro:

    -chain().waitForNotificationsUpToTip(m_last_block_processed);
+chain().waitForNotificationsUpToTip(WITH_LOCK(cs_wallet, return m_last_block_processed));

    //! Run code while locking a mutex.
//!
//! Examples:
//!
//!   WITH_LOCK(cs, shared_val = shared_val + 1);
//!
//!   int val = WITH_LOCK(cs, return shared_val);
//!
#define WITH_LOCK(cs, code) [&]{ LOCK(cs); code; }()
    jnewbery commented at 3:53 PM on April 18, 2019:

    I like the macro, but I don't like putting the complete expression in the waitForNotificationsUpToTip() function call. It's too easy for a casual observer to think that the lock is held for the entire call to waitForNotificationsUpToTip() (which of course would cause a deadlock). How about:

    uint256 last_block_hash = WITH_LOCK(cs_wallet, m_last_block_processed);
chain().waitForNotificationsUpToTip(last_block_hash);

    which perhaps compiles to the same thing.

    ariard commented at 12:21 PM on April 19, 2019:

    Ah yes, wasn't sure of cs_wallet lock covering. Added a WITH_LOCK commit and used to get back m_last_block_processed as showed in example.

  5. DrahtBot commented at 3:13 PM on April 18, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
    • #15855 ([refactor] interfaces: Add missing LockAnnotation for cs_main by MarcoFalke)

    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.

  6. in src/interfaces/chain.h:266 in 1bcf91d49f outdated 
     259 | @@ -271,8 +260,10 @@ class Chain
 260 |      //! Register handler for notifications.
 261 |      virtual std::unique_ptr<Handler> handleNotifications(Notifications& notifications) = 0;
 262 |  
 263 | -    //! Wait for pending notifications to be handled.
 264 | -    virtual void waitForNotifications() = 0;
 265 | +    //! Wait for pending notifications to be processed unless block hash points to the current,
 266 | +    //! chain tip, or to a possible descendant of the current chain tip that isn't currently
 267 | +    //! connected.
 268 | +    virtual void waitForNotificationsUpToTip(const uint256& hash) = 0;
    ryanofsky commented at 3:31 PM on April 18, 2019:

    I think the "wait up to" name I suggested earlier in the todo comment is misleading, because this isn't really deciding what to wait for based on the argument, but deciding whether to wait based on the argument.

    I think a better name might be waitForNotificationsIfNewBlocks(old_tip) or waitForNotificationsIfNewBlocksConnected(old_tip)

    ariard commented at 12:19 PM on April 19, 2019:

    Yes agree wasn't clear so I added Tip but even followed waitForNotificationsIfNewBlocksConnected(old_tip) suggestion, a little bit more verbose but far more insightful

  7. ariard force-pushed on Apr 19, 2019
  8. in src/interfaces/chain.cpp:355 in a4c51ed8ee outdated 
     351 | @@ -358,7 +352,16 @@ class ChainImpl : public Chain
 352 |      {
 353 |          return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
 354 |      }
 355 | -    void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
 356 | +    void waitForNotificationsIfNewBlocksConnected(const uint256& hash) override
    jnewbery commented at 2:48 PM on April 19, 2019:

    nit: please rename the argument old_tip to be consistent with the function declaration.

  9. in src/interfaces/chain.h:263 in a4c51ed8ee outdated 
     259 | @@ -271,8 +260,10 @@ class Chain
 260 |      //! Register handler for notifications.
 261 |      virtual std::unique_ptr<Handler> handleNotifications(Notifications& notifications) = 0;
 262 |  
 263 | -    //! Wait for pending notifications to be handled.
 264 | -    virtual void waitForNotifications() = 0;
 265 | +    //! Wait for pending notifications to be processed unless block hash points to the current,
    jnewbery commented at 2:50 PM on April 19, 2019:

    nit: remove trailing comma

  10. jnewbery commented at 2:52 PM on April 19, 2019: member

    utACK a4c51ed8ee2b087c1228402c3d4e665b4f0fcf37 with a couple of minor nits.

    Please remove the @ryanofsky from the commit log. Github buries you in notifications if your name appears in a commit log. You can keep the ryanofsky as long as it's not prefixed by @.

  11. Add WITH_LOCK macro: run code while locking a mutex
    Results from ryanofksy suggestion on isPotentialTip/
waitForNotifications refactoring
    edfe9438ca
  12. ariard force-pushed on Apr 20, 2019
  13. ariard force-pushed on Apr 20, 2019
  14. ariard commented at 12:22 PM on April 20, 2019: member

    Noted for the github trick, solved the nits on deb5f75

  15. jnewbery commented at 3:01 PM on April 22, 2019: member

    Doesn't build. Please build/test locally before pushing :slightly_smiling_face:

  16. ariard force-pushed on Apr 23, 2019
  17. ariard commented at 1:02 PM on April 23, 2019: member

    Sorry for that, working on dev scripts to avoid this kind of mistake even for nits updates!

  18. ryanofsky approved
  19. ryanofsky commented at 3:21 PM on April 23, 2019: member

    utACK 57007b8c9e6a70065d4c31fad3c81e60f7aeccd9. This change isn't as much of a simplification as I was hoping when I wrote the TODO suggesting it, but it's good to remove this one Chain::Lock usage, just as a prerequisite for removing the entire Chain::Lock interface later.

    One more suggestion would be to add GUARDED_BY(cs_wallet) annotation to:

    https://github.com/bitcoin/bitcoin/blob/cd14d210c4156a0a17450d3d451a92ae3938b019/src/wallet/wallet.h#L690

    because after this PR, accesses are now guarded by cs_wallet instead of cs_main.

  20. refactor: replace isPotentialtip/waitForNotifications by higher method
    Add GUARDED_BY(cs_wallet) annotation to m_last_block_processed, given
that its now guarded by cs_wallet instead of cs_main
    422677963a
  21. ariard force-pushed on Apr 23, 2019
  22. ariard commented at 6:13 PM on April 23, 2019: member

    Updated at 4226779 with GUARDED_BY annotation addition.

    (btw @ryanofsky, I've started to work on removing bits of Chain::Lock interface, notably the GetDepthInMainChain by tracking last_block_processed_height in CWallet, hope to PR soon to get feedback)

  23. ryanofsky approved
  24. ryanofsky commented at 6:44 PM on April 23, 2019: member

    utACK 422677963a7b41e340b911b4cd53d29dd8d63f21. Only change is adding the cs_wallet lock annotation.

    I'm glad to hear you're working on removing cs_main locking from GetDepthInMainChain. This should improve performance and allow removing locked_chain variables from a swath of wallet code. I think it'll require adding something like a map from block hash to height for CMerkleTx::hashBlock blocks, but there could be other approaches.

  25. ariard commented at 5:35 PM on April 24, 2019: member

    Maybe hold on a bit on this.

    While working on removing cs_main locking, it appears to me than, given wallet will be tracking chain height thanks to asynchronous updates and not direct chain state queries, it's needed to SyncWithValidationInterfaceQueue before each take of cs_wallet lock even if wallet is at the tip, because we may have BlockDisconnected in the callbacks validation interface queue and so it may cause races between RPC calls (at least spotted one thanks to mempool_resurrect)

    And yes doing remove by adding m_last_block_processed_height in both CWallet and m_block_height in CMerkleTx

  26. ryanofsky commented at 6:19 PM on April 24, 2019: member

    I don't think I understand the need to call SyncWithValidationInterfaceQueue unconditionally in the other PR, but even if there's no way around it, this PR doesn't prevent that because it's still possible to call waitForNotificationsIfNewBlocksConnected({})

  27. ariard commented at 7:21 PM on April 24, 2019: member

    Ah get your point, I'm going to far in locking removing (at least for now), was trying to not rely at all on Chain locking, even without '''::chainActive.Tip()->GetBlockHash()''' call, but only on callbacks hinting chain state.

  28. jnewbery commented at 11:28 PM on April 29, 2019: member

    ACK 422677963a7b41e340b911b4cd53d29dd8d63f21

  29. MarcoFalke referenced this in commit 0936f35f65 on May 1, 2019
  30. MarcoFalke merged this on May 1, 2019
  31. MarcoFalke closed this on May 1, 2019
  32. sidhujag referenced this in commit d652c80190 on May 1, 2019
  33. deadalnix referenced this in commit 7f8a7a1b16 on May 27, 2020
  34. kittywhiskers referenced this in commit 704238530a on Jun 5, 2021
  35. kittywhiskers referenced this in commit 8472dd01cb on Jun 6, 2021
  36. kittywhiskers referenced this in commit 60db7e0ed8 on Jun 6, 2021
  37. kittywhiskers referenced this in commit 8ad0733412 on Jun 7, 2021
  38. kittywhiskers referenced this in commit ce06a984af on Jun 8, 2021
  39. kittywhiskers referenced this in commit 42f12071a1 on Jun 9, 2021
  40. kittywhiskers referenced this in commit 96a64dabaa on Jun 10, 2021
  41. kittywhiskers referenced this in commit a612ec5087 on Jun 11, 2021
  42. UdjinM6 referenced this in commit c5cc285d0e on Jun 26, 2021
  43. DrahtBot locked this on Dec 16, 2021
Contributors
ariardjnewberyryanofskyDrahtBot
Labels
RefactoringWallet
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: 2026-04-21 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me