Block Wallet RPCs until wallet is synced to our current chain

TheBlueMatt commented at 11:53 pm on January 17, 2017: member

This fixes #9148, though with a bit more code than I intended.

There were two issues: a) wallet could return data based on partial block processing, because there were no locks between SyncTransaction calls, which is fixed relatively easily. and b) you could be busy-waiting on getbestblockheight , and call getbalance immediately after that changed, getting your balance as of the previous block instead of the new one, which is a regression in master from 0.13.

To resolve the second, we introduce a primitive to block until the wallet has caught up either with the current chain, or the best chain (to resolve a simple race condition).

This should probably get a test case, because I know you can see a in simple tests, but I’m not 100% sure about b.

TheBlueMatt force-pushed on Jan 17, 2017

TheBlueMatt force-pushed on Jan 18, 2017

Make SyncTransaction provide an blocks instead of one transaction

This simplifies fixing #9148 as we can now hold cs_wallet across
an entire block instead of only per-tx.

This change also removes the NOT_IN_BLOCK constant in favor of only
passing the CBlockIndex* parameter to SyncTransactions when a new
block is being connected, instead of also when a block is being
disconnected.

bc4c4c6686

Make DisconnectBlock and ConnectBlock static in validation.cpp 4c70ae2606

TheBlueMatt force-pushed on Jan 18, 2017

Remove CValidationInterface::UpdatedTransaction

This removes another callback from block connection logic, one more
step towards fixing #9148.

Note that this does a full mapWallet loop after the first block
which is connected after restart. This is due to a previous bug
where a coinbase transaction from the current best tip which
existed in wallet immediately prior to restart would not be shown
in the GUI except because on-load -checkblocks would call
DisconnectBlock and ConnectBlock on the current tip. To avoid
making it worse (ie that such transactions would never be displayed
until restart), a scan to find such transactions was added.

997b578c46

Add ability to assert a lock is not held in DEBUG_LOCKORDER 05bd9d5ce3

TheBlueMatt force-pushed on Jan 18, 2017

Add CWallet::BlockUntilSyncedToCurrentChain()

This blocks until the wallet has synced up to the current height.

eda6afbac6

Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs

This resolves #9148

d632a99951

TheBlueMatt force-pushed on Jan 18, 2017

jonasschnelli added the label RPC/REST/ZMQ on Jan 18, 2017

TheBlueMatt commented at 7:57 pm on January 18, 2017: member

@morcos points out that the cs_main holding at for the block’s transaction’s processing loop might interfere with all the fancy new fast block-response networking stuff we have going in 0.14. Sadly, because cs_main must come before cs_wallet, this isnt trivial to fix.

One easy fix is to add another cs_wallet - cs_wallet_before_main which is locked here to emulate the behavior prior to the lock split in #7946.

TheBlueMatt commented at 8:16 pm on January 18, 2017: member

Thinking about it more, most of those things will not be running at that time anyway, because we only have one message processing thread. This may have adverse effects on miners who run with a wallet, but aside from that I dont think there is really all that much cs_main contention here (and certainly wouldn’t be a regression from 0.13). We can add a cs_wallet_before_main when we split message processing thread.

f "Add CWallet::BlockUntilSyncedToCurrentChain()" add recheck timeout d26a43ce52

f "Add CWallet::BlockUntilSyncedToCurrentChain()" Handle the case that we've moved passed the initial block without cs_main 6ddb874fbb

f "Remove CValidationInterface::UpdatedTransaction" fix for multiwallet ff9d9603c0

f "Add CWallet::BlockUntilSyncedToCurrentChain()" fix comment text da2861338f

TheBlueMatt force-pushed on Jan 18, 2017

laanwj added this to the milestone 0.14.0 on Jan 19, 2017

morcos commented at 1:47 pm on January 19, 2017: member

utACK?

I think the code is right, but I wish there were a bit more straightforward way of doing this.

TheBlueMatt commented at 4:28 pm on January 19, 2017: member

Closing in favor of #9583 for 0.14. Will open a new PR with a bigger set of changes that also addresses the concerns in #9584.

TheBlueMatt closed this on Jan 19, 2017

MarcoFalke locked this on Sep 8, 2021

Block Wallet RPCs until wallet is synced to our current chain #9570