Commit e545dedf72bff2bd41c93c93eb576929fce37112 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.
Yeah. Maybe an alternative would be to always catch up with the queue?
0--- a/src/wallet/wallet.cpp
1+++ b/src/wallet/wallet.cpp
2@@ -1279,23 +1279,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() {
3 AssertLockNotHeld(cs_main);
4 AssertLockNotHeld(cs_wallet);
5
6- {
7- // Skip the queue-draining stuff if we know we're caught up with
8- // chainActive.Tip()...
9- // We could also take cs_wallet here, and call m_last_block_processed
10- // protected by cs_wallet instead of cs_main, but as long as we need
11- // cs_main here anyway, its easier to just call it cs_main-protected.
12- LOCK(cs_main);
13- const CBlockIndex* initialChainTip = chainActive.Tip();
14-
15- if (m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) {
16- return;
17- }
18- }
19-
20- // ...otherwise put a callback in the validation interface queue and wait
21- // for the queue to drain enough to execute it (indicating we are caught up
22- // at least with the time we entered this function).
23+ // Wait for the queue to catch up on everything that was there when we entered this function
24 SyncWithValidationInterfaceQueue();
25 }
Though, I am not sure how useful that is outside of functional tests.
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 https://github.com/bitcoin/bitcoin/pull/12206/commits/fadc3f6249a9eddaa3078e570a4465162032195e
I was suggesting adding a RPC to just do SyncWithValidationInterfaceQueue or whatever.
Feels weird to replace assertions with poll.
1643@@ -1628,6 +1644,7 @@ static const CRPCCommand commands[] =
1644 { "hidden", "waitfornewblock", &waitfornewblock, {"timeout"} },
1645 { "hidden", "waitforblock", &waitforblock, {"blockhash","timeout"} },
1646 { "hidden", "waitforblockheight", &waitforblockheight, {"height","timeout"} },
1647+ { "hidden", "syncwithvalidationinterfacequeue", &syncwithvalidationinterfacequeue, {} },
323@@ -323,6 +324,21 @@ UniValue waitforblockheight(const JSONRPCRequest& request)
324 return ret;
325 }
326
327+UniValue syncwithvalidationinterfacequeue(const JSONRPCRequest& request)
src/rpc/misc.cpp is a better place?
blockchain.cpp also contains mempool rpcs. And this one is about mempool, no?
389@@ -390,7 +390,7 @@ def sync_chain(rpc_connections, *, wait=1, timeout=60):
390 timeout -= wait
391 raise AssertionError("Chain sync failed: Best block hashes don't match")
392
393-def sync_mempools(rpc_connections, *, wait=1, timeout=60):
394+def sync_mempools(rpc_connections, *, wait=1, timeout=60, flush_scheduler=True):
utACK fa1e69e52bf8de08b1ce7a774416aa7a8d20068b
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.
@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.
since every release adds new rpcs
indeed, but not all RPCs are called in the mainline setup_nodes() method.
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.
Labels
Tests
RPC/REST/ZMQ
Milestone
0.16.0