Hold cs_main while calling UpdatedBlockTip() signal #12988

skeees · 2018-04-15T15:25:40Z

Resolves #12978

skeees · 2018-04-15T15:37:33Z

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

ryanofsky · 2018-04-15T16:31:34Z

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

Would have to look into it more, but if the problem is NotifyBlockTip events being sent to the GUI in the wrong order, we should determine whether one of @TheBlueMatt's PRs fixes this. Assuming one does, it's probably sufficient for this PR to just update the NotifyBlockTip documentation to say that notifications can arrive at the same time from multiple threads and there are no ordering guarantees (or whatever the current situation is).

skeees · 2018-04-15T16:39:06Z

#11856 is the one that I think you are referring to which eliminates that particular call to NotifyBlockTip and replaces it with the ValidationInterface signals

TheBlueMatt · 2018-04-15T16:40:58Z

I think this PR is fine as-is, the GUI I'd be surprised if it were broken, though the mining stuff may be (but #11856 pulls it all out to using validationinterface, which should be a less-likely-to-break-things fix as it doesn't introduce new lockorders). It'd also be nice to see the no-change-callbacks go away ala a9db3da since we're touching this code anyway.

ryanofsky · 2018-04-15T16:50:15Z

I think this PR is fine as-is

I think it would be good for this PR to update NotifyBlockTip documentation if notifications can come out of order, regardless of whether there are currently any gui bugs.

skeees · 2018-04-15T17:25:02Z

@ryanofsky - i'll update docs
@TheBlueMatt - will evaluate whether feasible to cherry-pick that commit into this pr

laanwj · 2018-04-16T13:34:10Z

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

I expect the GUI doesn't care - it transfers the signals to a different thread anyway.

jnewbery · 2018-04-16T14:25:43Z

Looks good to me. I don't think a9db3da needs to be pulled in - it's an orthogonal change for whether to fire notifications at all. It's useful, but no need to widen the scope of this PR.

TheBlueMatt · 2018-04-16T14:29:40Z

Heh, @jnewbery pointed out that a9db3da doesn't actually fix it, I was looking at the wrong callback, sorry for the noise.

TheBlueMatt · 2018-04-16T14:44:58Z

As for the uiInterface callback, I think its best to move it into the cs_main as well - as @laanwj points out in the GUI its immediately pushed to a queue for main thread execution so that should be fine, and the RPCNotifyBlockChange call should be cs_main-safe as it only notify_all()s and the cs_blockchange mutex is never taken with cs_main in another thread. Obviously this really sucks and risks future bugs and I'd like to see #11856 kill the uiInterface callback there entirely, but for a backport/fix, it looks pretty safe to me with current code.

promag · 2018-04-16T14:54:15Z

src/validation.cpp

+
+            // Notify external listeners about the new tip.
+            // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected
+            GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);


Should add

AssertLockHeld(cs_main);

to CMainSignals::UpdatedBlockTip? And a comment.

promag · 2018-04-16T14:56:14Z

src/validation.cpp

        }
        // When we reach this point, we switched to a new tip (stored in pindexNewTip).

        // Notifications/callbacks that can run without cs_main

-        // Notify external listeners about the new tip.
-        GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
-
        // Always notify the UI if a new block tip was connected
        if (pindexFork != pindexNewTip) {
            uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);


This means that the UI can present a tip that is not the tip?

skeees · 2018-04-16T15:53:47Z

Consensus seems to be to update the gui callbacks in cs_main as well (until #11856 which moves these gui callbacks to the ValidationInterface) - latest commit does that and addresses @promag's other review comment

jnewbery · 2018-04-16T17:06:28Z

ACK b6f486c85c85e024f03941cbbeadb348ddceaaf8. Commits can be squashed.

skeees · 2018-04-16T18:03:07Z

Squashed

jnewbery · 2018-04-16T18:19:25Z

ACK 713c066

→ git diff b6f486c 713c066

TheBlueMatt

utACK 713c066cdc1cc4750dfa1d41149621356ff2ad04

TheBlueMatt · 2018-04-16T19:32:49Z

src/validationinterface.cpp

+    // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which
+    // the chain actually updates. In order to ensure this, these signals should be enqueued
+    // in the same critical section where the chain is updated
+    AssertLockHeld(cs_main);


Bleh, if you're gonna assert this here you should probably assert it in pretty much every signal, as they all have ordering requirements. It also just seems kinda strange to do so cause its not really a requirement of the validationinterface, its a requirement that they be ordered, not done in one specific cs (eg the mempool ones could probably be ordered by mempool.cs instead).

I was also not loving this AssertLockHeld() for the same reason. I'd prefer we drop it and just leave a comment explaining the ordering requirement (maybe with a comment mentioning the cs_main lock as one way to achieve it).

Agreed - updated

I'm not a fan of spreading AssertLockHeld either but in this case the current fix is to lock and having the assert here makes that explicitly required. With just the comment a future refactor can break that requirement and reintroduce the bug.

But it was just a suggestion.

Note that the AssertLockHeld wouldn't have prevented the caller from releasing cs_main after updating the tip, and reacquiring cs_main before the validation interface callbacks -- which would still result in violating the ordering requirements.

@sdaftuar that's true and the same could be said to every AssertLockHeld 😛

Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978

promag · 2018-04-17T13:08:14Z

utACK d86edd3.

sdaftuar · 2018-04-17T13:34:44Z

utACK d86edd3

d86edd3 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) Pull request description: Resolves #12978 Tree-SHA512: 2aed09434cd8dbf541ea75462070b73ee87ff31409bede210f6999ffee4a37e32202a289efd37609485d4cbdfe134fe4660a10bfb41e8a8acdba7cd0b61b8780

Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978 GitHub-Pull: bitcoin#12988 Rebased-From: d86edd3

acdf433 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) 5ff571e [wallet] [tests] Test disallowed multiwallet params (John Newbery) 4c14e7b [wallet] Fix zapwallettxes/multiwallet interaction. (John Newbery) 4087dd0 RPC Docs: gettxout*: clarify bestblock and unspent counts (David A. Harding) b8aacd6 [qa] Handle disconnect_node race (Suhas Daftuar) Pull request description: Backports: - #13201 [qa] Handle disconnect_node race - #13184 RPC Docs: gettxout*: clarify bestblock and unspent counts - #13030 [bugfix] [wallet] Fix zapwallettxes/multiwallet interaction. - #12988 Hold cs_main while calling UpdatedBlockTip() signal to the 0.16 branch. Tree-SHA512: 8f65002bbafaf9c436f89051b2d79bf6a668fbd07bd317c64af238ed4a7c8efe776864b739a7f2869f1e3daa16f2f4366a85f41b188f9c454879d2c7b309be50

Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978 GitHub-Pull: bitcoin#12988 Rebased-From: d86edd3

d86edd3 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) Pull request description: Resolves bitcoin#12978 Tree-SHA512: 2aed09434cd8dbf541ea75462070b73ee87ff31409bede210f6999ffee4a37e32202a289efd37609485d4cbdfe134fe4660a10bfb41e8a8acdba7cd0b61b8780

d86edd3 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) Pull request description: Resolves bitcoin#12978 Tree-SHA512: 2aed09434cd8dbf541ea75462070b73ee87ff31409bede210f6999ffee4a37e32202a289efd37609485d4cbdfe134fe4660a10bfb41e8a8acdba7cd0b61b8780 fix indentation Signed-off-by: pasta <pasta@dashboost.org>

…tChain + optimization 50dbec5 Add unit tests for signals generated by ProcessNewBlock() (furszy) f68251d Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy) a51a755 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen) 198f435 Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo) 8d15cf5 Optimize ActivateBestChain for long chains (Pieter Wuille) 8640be1 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo) ef24337 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) f9d2ab3 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) cb9bb25 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 4ea2048 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: Made some back ports and adaptations to validate further the work introduced in #2203 and #2209. Now that the scheduler thread is processing most of the chain events, the callbacks execution order must be verified to dispatch the events serially, ensuring a consistent memory state in each event processing invocation. There are some concurrency issues solved, as well a an optimization for larger chains for the ABC workflow. Each commit details them well. List: bitcoin#7917 (only fb8fad1) bitcoin#12988 bitcoin#13023 bitcoin#13247 bitcoin#13835 This is built on top of #2284 (and the reason that made me notice that problem). So, 2284 comes first, then this one. ACKs for top commit: Fuzzbawls: ACK 50dbec5 random-zebra: ACK 50dbec5 and merging... Tree-SHA512: 573144cc96d2caa49ed2f0887dc8c53b5aca0efd54b6a25626063ccb780da426f56b3c6b7491648e7abefc1960c82b1354a4ff2c51425bfe99adaa4394562608

skeees mentioned this pull request Apr 15, 2018

p2p_compactblocks.py failing occasionally on master #12978

Closed

promag reviewed Apr 16, 2018

View reviewed changes

skeees force-pushed the updatedblocktip-race branch from b6f486c to 713c066 Compare April 16, 2018 18:02

TheBlueMatt reviewed Apr 16, 2018

View reviewed changes

Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip

d86edd3

Ensures that callbacks are invoked in the order in which the chain is updated Resolves bitcoin#12978

skeees force-pushed the updatedblocktip-race branch from 713c066 to d86edd3 Compare April 16, 2018 22:04

laanwj merged commit d86edd3 into bitcoin:master Apr 17, 2018

Bushstar mentioned this pull request Apr 18, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#299

Merged

skeees deleted the updatedblocktip-race branch April 21, 2018 13:41

skeees mentioned this pull request Apr 23, 2018

[net] [validation] Call ProcessNewBlock() asynchronously #12934

Closed

laanwj added Validation Needs backport labels May 3, 2018

laanwj added this to the 0.16.1 milestone May 3, 2018

fanquake mentioned this pull request May 17, 2018

[0.16] Further Backports #13253

Merged

fanquake removed the Needs backport label May 17, 2018

furszy mentioned this pull request Apr 2, 2021

[Validation][Test] Fix concurrency issues in ActivateBestChain + optimization PIVX-Project/PIVX#2290

Merged

str4d mentioned this pull request Sep 23, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

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

Hold cs_main while calling UpdatedBlockTip() signal #12988

Hold cs_main while calling UpdatedBlockTip() signal #12988

Uh oh!

Conversation

skeees commented Apr 15, 2018

Uh oh!

skeees commented Apr 15, 2018

Uh oh!

ryanofsky commented Apr 15, 2018

Uh oh!

skeees commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 15, 2018

Uh oh!

ryanofsky commented Apr 15, 2018

Uh oh!

skeees commented Apr 15, 2018

Uh oh!

laanwj commented Apr 16, 2018

Uh oh!

jnewbery commented Apr 16, 2018

Uh oh!

TheBlueMatt commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 16, 2018

Uh oh!

promag Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

skeees commented Apr 16, 2018

Uh oh!

jnewbery commented Apr 16, 2018

Uh oh!

skeees commented Apr 16, 2018

Uh oh!

jnewbery commented Apr 16, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

sdaftuar Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

skeees Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

promag Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

sdaftuar Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Apr 17, 2018

Uh oh!

sdaftuar commented Apr 17, 2018

Uh oh!

Uh oh!

skeees commented Apr 15, 2018 •

edited

Loading

TheBlueMatt commented Apr 16, 2018 •

edited

Loading

promag Apr 16, 2018 •

edited

Loading