qa: Sync with validationinterface queue in sync_mempools #12206

maflcko · 2018-01-17T01:18:14Z

Commit e545ded moved TransactionAddedToMempool to the background scheduler thread. Thus, adding a transaction to the mempool will no longer add it to the wallet immediately. Functional tests, that sync_mempools and then call into wallet rpcs will race against the scheduler thread.

Fix that race by flushing the scheduler queue.

Fixes #12205; Fixes #12171;
References #9584;

maflcko · 2018-01-17T01:33:18Z

Obviously all other functional tests are affected as well. I will try to fix them in a later commit, but the above fix is needed the most as it is almost always failing on travis.

promag · 2018-01-17T01:35:05Z

Concept ACK. Not sure the test should handle this way. IMO some sync_mempool(flush_scheduler=True) would be better?

maflcko · 2018-01-17T01:50:13Z

Yeah. Maybe an alternative would be to always catch up with the queue?

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1279,23 +1279,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() {
     AssertLockNotHeld(cs_main);
     AssertLockNotHeld(cs_wallet);
 
-    {
-        // Skip the queue-draining stuff if we know we're caught up with
-        // chainActive.Tip()...
-        // We could also take cs_wallet here, and call m_last_block_processed
-        // protected by cs_wallet instead of cs_main, but as long as we need
-        // cs_main here anyway, its easier to just call it cs_main-protected.
-        LOCK(cs_main);
-        const CBlockIndex* initialChainTip = chainActive.Tip();
-
-        if (m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) {
-            return;
-        }
-    }
-
-    // ...otherwise put a callback in the validation interface queue and wait
-    // for the queue to drain enough to execute it (indicating we are caught up
-    // at least with the time we entered this function).
+    // Wait for the queue to catch up on everything that was there when we entered this function
     SyncWithValidationInterfaceQueue();
 }

Though, I am not sure how useful that is outside of functional tests.

meshcollider · 2018-01-17T11:38:00Z

I think @promag was suggesting a different way for the tests to handle it, not to change the core code btw.

Current approach seems fine to me at least for now,
utACK fadc3f6

maflcko · 2018-01-17T13:29:16Z

I am not aware of a way to flush the scheduler from outside the code without modifying the code or mining a block. Though, I might be misssing something.

promag · 2018-01-17T13:54:52Z

I was suggesting adding a RPC to just do SyncWithValidationInterfaceQueue or whatever.

Feels weird to replace assertions with poll.

maflcko · 2018-01-17T15:43:57Z

Yes, I plan to do something like that after branch off of 0.16. At this point I'd feel bad to delay 0.16 by spawning unnecessary review-load on the queue.

TheBlueMatt · 2018-01-17T21:05:50Z

I agree with @promag here, and that fix should be just as/more simple/obvious than even this fix. Just add an RPC that calls SyncWithValidationInterfaceQueue() and call that at the end of sync_mempools(), at least then its much more obvious the fix is complete, and should require a lower review burden.

ryanofsky

utACK fadc3f6249a9eddaa3078e570a4465162032195e. I agree with everybody else that this should definitely be followed up on, but this fix is small and simple, and the test is failing constantly on travis, so it seems odd to object to merging this as a workaround.

maflcko · 2018-01-17T21:50:47Z

Introduced the new syncwithvalidationinterfacequeue rpc as requested by @TheBlueMatt and @promag

promag

Sweet utACK fadc3f6.

promag · 2018-01-17T21:58:51Z

src/rpc/blockchain.cpp

@@ -1628,6 +1644,7 @@ static const CRPCCommand commands[] =
    { "hidden",             "waitfornewblock",        &waitfornewblock,        {"timeout"} },
    { "hidden",             "waitforblock",           &waitforblock,           {"blockhash","timeout"} },
    { "hidden",             "waitforblockheight",     &waitforblockheight,     {"height","timeout"} },
+    { "hidden",             "syncwithvalidationinterfacequeue", &syncwithvalidationinterfacequeue, {} },


Sort, alignment...

ryanofsky

utACK fa1e69e. @TheBlueMatt was right, the new fix is more simple and obvious than the old one.

promag · 2018-01-18T00:45:40Z

src/rpc/blockchain.cpp

@@ -323,6 +324,21 @@ UniValue waitforblockheight(const JSONRPCRequest& request)
    return ret;
 }

+UniValue syncwithvalidationinterfacequeue(const JSONRPCRequest& request)


Not sure but src/rpc/misc.cpp is a better place?

My understanding is that blockchain.cpp also contains mempool rpcs. And this one is about mempool, no?

promag · 2018-01-18T00:47:35Z

test/functional/test_framework/util.py

@@ -390,7 +390,7 @@ def sync_chain(rpc_connections, *, wait=1, timeout=60):
        timeout -= wait
    raise AssertionError("Chain sync failed: Best block hashes don't match")

-def sync_mempools(rpc_connections, *, wait=1, timeout=60):
+def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True):


How about not making it optional? If there is such need in the future, add it then?

Right now it is overkilling, i.e. always flushing. But that is not necessary, as for non-wallet functional tests and also most wallet functional test it can be turned off. I will leave the "turning it off" for later.

Ok, so minimal diff for now.

fa1e69e qa: Sync with validationinterface queue in sync_mempools (MarcoFalke) Pull request description: Commit e545ded moved `TransactionAddedToMempool` to the background scheduler thread. Thus, adding a transaction to the mempool will no longer add it to the wallet immediately. Functional tests, that `sync_mempools` and then call into wallet rpcs will race against the scheduler thread. Fix that race by flushing the scheduler queue. Fixes #12205; Fixes #12171; References #9584; Tree-SHA512: 14d99cff9c4756de9fad412f04e6d8e25bb9a0938f24ed8348de79df5b4ee67763dac5214b1a69e77e60787d81ee642976d1482b1b5637edfc4892a238ed22af

laanwj · 2018-01-18T14:06:54Z

utACK fa1e69e

jnewbery · 2018-01-22T17:01:37Z

utACK fa1e69e

One comment: this places a brand new RPC in the mainline test setup path, which will make efforts like #12134 more difficult. I think binaries from commits prior to this will fail running any test scripts from this commit onwards, because the test code will try to call a non-existent RPC.

Not a reason not to merge, but something to keep in mind.

maflcko · 2018-01-22T18:11:36Z

@jnewbery This is true for every release, since every release adds new rpcs.

Also it should be trivial to work around in python with a one or two-line change.

jnewbery · 2018-01-22T18:23:29Z

since every release adds new rpcs

indeed, but not all RPCs are called in the mainline setup_nodes() method.

maflcko · 2018-01-22T18:53:00Z

That looks like an overkill. Currently setup_network is only called after the chain was set up and I don't think there are any plans to change this in the near future. Syncing the (empty) mempools just after a "fresh" chain was set up is probably just wasting time on travis.

…mempools fa1e69e qa: Sync with validationinterface queue in sync_mempools (MarcoFalke) Pull request description: Commit e545ded moved `TransactionAddedToMempool` to the background scheduler thread. Thus, adding a transaction to the mempool will no longer add it to the wallet immediately. Functional tests, that `sync_mempools` and then call into wallet rpcs will race against the scheduler thread. Fix that race by flushing the scheduler queue. Fixes bitcoin#12205; Fixes bitcoin#12171; References bitcoin#9584; Tree-SHA512: 14d99cff9c4756de9fad412f04e6d8e25bb9a0938f24ed8348de79df5b4ee67763dac5214b1a69e77e60787d81ee642976d1482b1b5637edfc4892a238ed22af

…ead (without cs_main) 3d11027 wallet:CreateCoinStake, solving IsSpent() missing cs_main lock. (furszy) 046386b IsNoteSaplingChange: Add missing cs_wallet lock. (furszy) ded2e8e feature_dbcrash.py using blockmaxsize instead of blockmaxweight that we currently don't support. (furszy) 00cc6ec dumpwallet: Add missing BlockUntilSyncedToCurrentChain (furszy) bfd9a15 test: sapling_fillblock.py sync mempool every 200 transactions instead of only at the end. (furszy) 53497f0 Validation: DisconnectTip doesn't need to force a flush to disk. (furszy) f8cd371 [Miner] Sync wallet state before try to solve proof of stake. (furszy) 3ace13b qa: Fix some tests to work on native windows (furszy) 65cf7e1 don't attempt mempool entry for wallet transactions on startup if already in mempool (instagibbs) 756d0fa Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift) 1423dba [bugfix] save feeDelta instead of priorityDelta in DumpMempool (Alex Morcos) d97ace9 [Test] notes_double_spend: sync wallet before check balances. (furszy) 1ed753f Fix wallet_tests.cpp, missing fInMempool flag set. (furszy) 815667d unit test framework: missing scheduler service loop start added. (furszy) de3c7ae fix wallet_upgrade.py test, wasn't counting the coinbase script. (furszy) e6770c8 fixing invalid wallet_dump.py, generated PoW blocks use a P2PKH coinbase script that now is properly marked as used inside the wallet. (furszy) 4ed7024 fix invalid numbers in wallet_labels.py (furszy) b9249c5 Miner: generate RPC, fix coinbase script key not marked as used (furszy) 296c956 wallet: guard null m_last_block_processed (furszy) 0dfebf4 sapling_rpc_wallet_tests: remove unneeded cs_main and cs_wallet locks. (furszy) c3a281c fix mempool_persist.py dump issue, missing sync with validation interface. (furszy) 67c754a qa: Sync with validationinterface queue in sync_mempools (MarcoFalke) 596056c [validation] Do not check for double spent zerocoins. (furszy) 0c4642c Add helper to wait for validation interface queue to catch up (Matt Corallo) cc91d44 Block ActivateBestChain to empty validationinterface queue (Matt Corallo) 0c68e2f Add an interface to get the queue depth out of CValidationInterface (Matt Corallo) 31c7974 Decouple block processing cs_main lock from the rest of inv get data requests (furszy) da7c0f7 Refactor ProcessGetData avoiding to lock cs_main for its entire time. (furszy) 10efbe5 net_processing: making PushTierTwoGetDataRequest return a bool in case of pushing the message. (furszy) 51dea23 net_processing move-only: decouple tier two get data request into its own function. (furszy) 1c9fe10 RPC: listunspent remove redundant wallet check (furszy) 4d927b0 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 5f521fd Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) 7d05997 Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) c7ab490 Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 31a8790 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) f6df6e4 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (furszy) 24a3ce4 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 40ed4c4 Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 268be9c Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) 1fa0d70 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Concluding with the validation <--> wallet asynchronous signal processing work started in #2082, #2118, #2150, #2192, #2195. Effectively moving every validation interface callback to a background thread without locking `cs_main` for its entire process (each handler can now request `cs_main` lock only when/if they need it). This has a direct performance improvement on the synchronization time (which i haven't measured yet because there is one/two more PRs over the wallet and GUI areas, probably large as well, on top of this one and #2201 that should boost the sync time a lot more). Containing the following changes: * Adaptations coming from bitcoin#10286. * Adaptations coming from bitcoin#11824 (this one is different for us, take the base idea when you review it). Essentially solves a severe memory leak introduced previously in 10286 and improves `cs_main` lock acquisitions as well. * net_processing: decouple and refactor tier two inv data request processing. * bitcoin#12206 ACKs for top commit: random-zebra: Great job! 🍻 ACK 3d11027 Fuzzbawls: ACK 3d11027 Tree-SHA512: 60a25604fb8a3ad0553ccb074aed99c1b3c6f8a765b40c1b43f25412373cbd2a9e4f0f413d45cf694bd62e48512c936099ffb7a0d23a1b97576cb33283ca05ac

maflcko added the Tests label Jan 17, 2018

maflcko added this to the 0.16.0 milestone Jan 17, 2018

ryanofsky reviewed Jan 17, 2018

View reviewed changes

qa: Sync with validationinterface queue in sync_mempools

fa1e69e

maflcko force-pushed the Mf1801-qaWalletMempoolAsync branch from fadc3f6 to fa1e69e Compare January 17, 2018 21:45

maflcko added the RPC/REST/ZMQ label Jan 17, 2018

maflcko changed the title ~~qa: Have wallet wait for TransactionAddedToMempool~~ qa: Sync with validationinterface queue in sync_mempools Jan 17, 2018

promag reviewed Jan 17, 2018

View reviewed changes

ryanofsky reviewed Jan 17, 2018

View reviewed changes

promag reviewed Jan 18, 2018

View reviewed changes

maflcko assigned laanwj Jan 18, 2018

laanwj merged commit fa1e69e into bitcoin:master Jan 18, 2018

maflcko deleted the Mf1801-qaWalletMempoolAsync branch January 18, 2018 14:21

AkioNak mentioned this pull request Jan 19, 2018

Reduce redundant code of prevector and speed it up #11988

Closed

furszy mentioned this pull request Feb 10, 2021

Call wallet/validation notify callbacks in scheduler thread (without cs_main) PIVX-Project/PIVX#2203

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

qa: Sync with validationinterface queue in sync_mempools #12206

qa: Sync with validationinterface queue in sync_mempools #12206

Uh oh!

Conversation

maflcko commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 17, 2018

Uh oh!

maflcko commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 17, 2018

Uh oh!

promag commented Jan 17, 2018

Uh oh!

maflcko commented Jan 17, 2018

Uh oh!

TheBlueMatt commented Jan 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 17, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 18, 2018

Uh oh!

jnewbery commented Jan 22, 2018

Uh oh!

maflcko commented Jan 22, 2018

Uh oh!

jnewbery commented Jan 22, 2018

Uh oh!

maflcko commented Jan 22, 2018

Uh oh!

Uh oh!

maflcko commented Jan 17, 2018 •

edited

Loading

maflcko commented Jan 17, 2018 •

edited

Loading

maflcko commented Jan 17, 2018 •

edited

Loading

meshcollider commented Jan 17, 2018 •

edited

Loading