refactor: replace isPotentialtip/waitForNotifications by higher method #15842

ariard · 2019-04-18T12:31:44Z

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.

jnewbery · 2019-04-18T14:09:27Z

src/wallet/wallet.cpp

    // for the queue to drain enough to execute it (indicating we are caught up
    // at least with the time we entered this function).
-    chain().waitForNotifications();
+    chain().waitForNotificationsUpToTip(m_last_block_processed);


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 ?

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; }()

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.

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.

DrahtBot · 2019-04-18T15:13:24Z

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

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.

ryanofsky · 2019-04-18T15:31:13Z

src/interfaces/chain.h

+    //! Wait for pending notifications to be processed unless block hash points to the current,
+    //! chain tip, or to a possible descendant of the current chain tip that isn't currently
+    //! connected.
+    virtual void waitForNotificationsUpToTip(const uint256& hash) = 0;


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)

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

ryanofsky · 2019-04-18T15:39:39Z

src/wallet/wallet.cpp

    // for the queue to drain enough to execute it (indicating we are caught up
    // at least with the time we entered this function).
-    chain().waitForNotifications();
+    chain().waitForNotificationsUpToTip(m_last_block_processed);


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

utACK a4c51ed 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 @.

src/interfaces/chain.cpp

src/interfaces/chain.h

Results from ryanofksy suggestion on isPotentialTip/ waitForNotifications refactoring

ariard · 2019-04-20T12:22:42Z

Noted for the github trick, solved the nits on deb5f75

jnewbery · 2019-04-22T15:01:07Z

Doesn't build. Please build/test locally before pushing 🙂

ariard · 2019-04-23T13:02:48Z

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

ryanofsky

utACK 57007b8. 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:

bitcoin/src/wallet/wallet.h

Line 690 in cd14d21

uint256 m_last_block_processed;

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

Add GUARDED_BY(cs_wallet) annotation to m_last_block_processed, given that its now guarded by cs_wallet instead of cs_main

ariard · 2019-04-23T18:13:57Z

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)

ryanofsky

utACK 4226779. 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.

ariard · 2019-04-24T17:35:15Z

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

ryanofsky · 2019-04-24T18:19:42Z

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({})

ariard · 2019-04-24T19:21:52Z

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.

jnewbery · 2019-04-29T23:28:48Z

ACK 4226779

…tions by higher method 4226779 refactor: replace isPotentialtip/waitForNotifications by higher method (Antoine Riard) edfe943 Add WITH_LOCK macro: run code while locking a mutex (Antoine Riard) Pull request description: 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. ACKs for commit 422677: jnewbery: ACK 4226779 ryanofsky: utACK 4226779. Only change is adding the cs_wallet lock annotation. Tree-SHA512: 2834ff0218795ef607543fae822e5cce25d759c1a9cfcb1f896a4af03071faed5276fbe0966e0c6ed65dc0e88af161899c5b2ca358a2d24fe70969a550000bf2

…tions b

…s by higher method Summary: Add GUARDED_BY(cs_wallet) annotation to m_last_block_processed, given that its now guarded by cs_wallet instead of cs_main bitcoin/bitcoin@4226779 --- Add WITH_LOCK macro: run code while locking a mutex Results from ryanofksy suggestion on isPotentialTip/ waitForNotifications refactoring bitcoin/bitcoin@edfe943 --- Depends on D6258 Backport of Core [[bitcoin/bitcoin#15842 | PR15842]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6260

…mutex

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

fanquake added the Refactoring label Apr 18, 2019

jnewbery added the Wallet label Apr 18, 2019

jnewbery reviewed Apr 18, 2019

View reviewed changes

ryanofsky reviewed Apr 18, 2019

View reviewed changes

ariard force-pushed the 2019-04-is-potential-tip branch from 1bcf91d to a4c51ed Compare April 19, 2019 12:16

jnewbery reviewed Apr 19, 2019

View reviewed changes

src/interfaces/chain.cpp Outdated Show resolved Hide resolved

src/interfaces/chain.h Outdated Show resolved Hide resolved

Add WITH_LOCK macro: run code while locking a mutex

edfe943

Results from ryanofksy suggestion on isPotentialTip/ waitForNotifications refactoring

ariard force-pushed the 2019-04-is-potential-tip branch 2 times, most recently from 9bf61e0 to deb5f75 Compare April 20, 2019 12:21

ariard force-pushed the 2019-04-is-potential-tip branch from deb5f75 to 57007b8 Compare April 23, 2019 13:01

ryanofsky approved these changes Apr 23, 2019

View reviewed changes

refactor: replace isPotentialtip/waitForNotifications by higher method

4226779

Add GUARDED_BY(cs_wallet) annotation to m_last_block_processed, given that its now guarded by cs_wallet instead of cs_main

ariard force-pushed the 2019-04-is-potential-tip branch from 57007b8 to 4226779 Compare April 23, 2019 18:08

ryanofsky approved these changes Apr 23, 2019

View reviewed changes

maflcko merged commit 4226779 into bitcoin:master May 1, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2019

Merge bitcoin#15842: refactor: replace isPotentialtip/waitForNotifica…

d652c80

…tions b

kwvg added a commit to kwvg/dash that referenced this pull request Jun 5, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

7042385

…mutex

kwvg mentioned this pull request Jun 5, 2021

partial #15842, merge #15849, #17342, #18710: Add local thread pool to CCheckQueue dashpay/dash#4191

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

8472dd0

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

60db7e0

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 7, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

8ad0733

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 8, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

ce06a98

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 9, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

42f1207

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

96a64da

…mutex

kwvg added a commit to kwvg/dash that referenced this pull request Jun 11, 2021

partial bitcoin#15842: Add WITH_LOCK macro: run code while locking a …

a612ec5

…mutex

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 26, 2021

Merge pull request #4191 from kittywhiskers/checkqueue

c5cc285

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

refactor: replace isPotentialtip/waitForNotifications by higher method #15842

refactor: replace isPotentialtip/waitForNotifications by higher method #15842

Uh oh!

Conversation

ariard commented Apr 18, 2019

Uh oh!

jnewbery Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

ariard Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

ariard Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard commented Apr 20, 2019

Uh oh!

jnewbery commented Apr 22, 2019

Uh oh!

ariard commented Apr 23, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Apr 24, 2019

Uh oh!

ryanofsky commented Apr 24, 2019

Uh oh!

ariard commented Apr 24, 2019

Uh oh!

jnewbery commented Apr 29, 2019

Uh oh!

Uh oh!

DrahtBot commented Apr 18, 2019 •

edited

Loading

ariard commented Apr 23, 2019 •

edited

Loading