Call wallet notify callbacks in scheduler thread (without cs_main) #10286

In commit "Add a CValidationInterface::TransactionRemovedFromMempool"

What does this imply? Just that if there are any new calls to addUnchecked, the caller also needs to signal TransactionAddedToMempool not to break the wallet? Would say this in the comment explicitly if this is the case.

Updated the comment to mention that addUnchecked is only called from ATMP outside of tests period. I think the implication is that we need to fix the strong-coupling here.

In commit "Add a CValidationInterface::TransactionRemovedFromMempool"

Unclear to me what a normal operation is. Comment might be clearer mentioning a not normal counterexample.

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

Does "some things might block forever" just mean this wait might block forever? If so, maybe be more concrete and say something like "lastBlockProcessed will not be rewound back to chainActive.Tip()." Otherwise it would be good to clarify what some things is referring to.

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

Stray tab here

In commit "Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs"

Can you give an example of specific bug that could occur without these BlockUntilSynced calls and is prevented by adding them? I looked at some of the old issues (#9584, #9148, etc), but they're confusing and I don't know how much of the information is up to date.

It would be great if BlockUntilSyncedToCurrentChain had a comment that made it clearer when it does and doesn't need to be called, and what consistency issues it is and isn't supposed to solve.

Maybe there should also be a bullet point in the new RPC interface guidelines about what kind of consistency wallet RPCs are expected to have.

In commit "Fix zmq tests now that txn/blocks are unordered"

Maybe assert msg[0] != topic above this line to confirm actually receive distinct hashtx and hashblock messages (not two hashblocks).

These changes do not appear to be related to the rest. Am I missing something or should this be in its own PR? I believe @sipa made a similar comment on #10179

Without them test_bitcoin-qt segfaults.

This would be safer/faster/cleaner with : internals(new CMainSignalsInstance()) {} instead of the body.

Initializer lists guarantee proper cross-thread visibility, otherwise you might init twice and have sharing issues.

Does that compile? CMainSignalsInstance() is not defined at that time, only declared.

Since it is only a declaration I believe it should have been fine. I see this code isn't present in the final commit so I suppose it doesn't matter either way.

Ahh, yes, since the rebase on the latest version of #10179 this code no longer exists, as the scheduler has to be passed into the creation of the internals object.

You can accomplish this entire commit by changing this line to void schedule(Function f, boost::chrono::system_clock::time_point t = boost::chrono::system_clock::now());

Took this on #10179, will be here when I next rebase.

This comment and issue can be avoided entirely if you move line 56 up to 46. After that lines 54 and 55 (which will be 55 and 56) can be removed.

This comment and issue can be avoided entirely if you move line 56 up to 46. After that lines 54 and 55 (which will be 55 and 56) can be removed.

I think this is right (line numbers apply to commit 8daf2439796dfdee41c1a32787e0ec9726daf6be). It also seems like you could eliminate the fCallbacksRunning variable if you change ProcessQueue to call pop_front after running the callback and condition the AddToProcessQueue schedule() call on the queue being previously empty.

Looks like you commented on an outdated version and github wont show me full context, so I have no idea what those line numbers refer to :/

@ryanofsky I believe the point is to avoid duplicate calls to the scheduler since it may be multi-threaded. So a call to AddToProcessQueue should not schedule() anything if we're already scheduled; it should only schedule() if our previously scheduled function has completed execution (at least beyond the point of it calling schedule() again). Can't see implementing that without knowing if fCallbacksRunning is true.

Still, the code does not guarantee single threaded execution in its current form due to the fCallbacksRunning state being set AFTER the call to schedule(). Being that all the locks are placed appropriately we might as well make this guarantee or else it seems like a bug because the SingleThreadedClient won't be.

I'll see if I can't get a test showing this behavior.

@ryanofsky I believe the point is to avoid duplicate calls to the scheduler since it may be multi-threaded.

I know, this is why the second half of my suggestion was "condition the AddToProcessQueue schedule() call on the queue being previously empty." Anyway, I don't think Matt's particularly interested in these simplifications, and it's easier to communicate these changes as patches rather than english descriptions, so I'd rather just leave any simplifications to followup PRs.

Still, the code does not guarantee single threaded execution in its current form due to the fCallbacksRunning state being set AFTER the call to schedule()

The reason it works in its current form is because of the if (fCallbacksRunning) return; line at the top of ProcessQueue()

Again, I don't think the code in it's current form is the simplest it could be, but it seems safe and easy to clean up later in a followup PR. Also this whole discussion really should be moved to #10179. #10286 is only building on the changes in #10179.

@ryanofsky I see that now, the extra check does prevent the execution.

Being that this is new code I wouldn't call it a simplification. Here's a patch of the proposed change, less logic with the same function: scheduler.patch.txt

which reads better if you rename fCallbacksRunning to fCallbacksScheduled and this patch: scheduler.patch2.txt which can be argued reduces code reuse but I think the readability is improved.

@TheBlueMatt If you're open to these changes in a PR to your branch I can do that, I assume they'll be squashed so either way works.

@mchrostowski hmm, really, I find that it decreases readability (though that may be NIH). It looks harder to reason about whether some callbacks might accidentally get missed to me.

(Other random note, we dont use tabs in our codebase, which your patch added).

@TheBlueMatt Well, in that case I feel like either patch gets funky, especially since the use of fCallbacksRunning becomes inconsistent if you apply the first patch without the second (unless some alternative name for fCallbacksRunning works).

The extra safety check and inconsistency of scheduling bothers me but I wouldn't expect it to actually cause issues so I have no grounds for objection.

I think my inquiry stemmed from it not being immediately apparent that ProcessQueue() only runs once and the extra check is just an extra check. Perhaps the "not a big deal" part of the comment could be "because ProcessQueue() already checks" for clarity, I would not have looked so deeply into the code except that I thought "not a big deal" meant "sometimes interweaving calls is okay."

RAII is great and all but exists for the acquisition of resources. Why not try{} catch{}? try { callback(); } catch(...) { { LOCK(cs_callbacksPending); fCallbacksRunning = false; } MaybeScheduleProcessQueue(); }

Why not try{} catch{}?

My guess about this was that it allows the processqueue to take advantage of whatever error handling or reporting cscheduler provides, and to not have to repeat the finalization logic both inside and after the catch clause. Either approach seems fine to me, though.

@laanwj previously requested that any exceptions be thrown all the way up, so this was an easier way to do that. That request seemed reasonable.

Understood, makes perfect sense. Didn't occur to me we'd have to duplicate the logic, spoiled by finally.

Is this really what we want? A scheduler call for each callback? It does prevent starving any other scheduled tasks in case of a long queue, but it also generates a lot of lock contention which can be a performance killer.

Unless there is evidence of this queue processing messing with other scheduling I feel strongly we should avoid this design. It will be much harder to detect performance issues from lock contention than performance issues from the processing of a long queue.

I recommend replacing line 45 with a size() query, -- the size (to account for your pop), and put the entire thing in a do {} while (size > 0);. You can then avoid calling MaybeScheduleProcessQueue() inside of ProcessQueue() itself.

Is this really what we want? A scheduler call for each callback?

I'm guessing the work done in any of these callbacks far outweighs the cost of scheduling one of them but I could be wrong.

Right now we only have one scheduler thread, so there should be limited lock contention, not to mention anything running is gonna take much longer than any locks held (locks are only held to push/pop/whatever, which should be very, very fast).

Okay, sounds like blocking/starving is the biggest risk. Can't disagree with that.

Feels like a bit of wasted work to call schedule on things you know are intended to execute now, but that's just a performance (not important) concern. A bit surprising there is only one scheduler thread when the scheduler counts threads, but indeed it is only one.

This is no longer a scheduler. It has one public method, void AddToProcessQueue(std::function<void (void)> func);, which does not take any 'schedule' information.

This class is neat, more of a SingleThreadedExecutor that happens to use a scheduler to execute. Really its treating the scheduler as a thread pool.

I'm all for keeping this if it's not named 'scheduler' and if a thread pool abstraction can be extracted from CScheduler then both this class and CScheduler can use that pool for execution. Also to consider, is this used anywhere else yet or is it expected to be used?

Its unlikely to be used elsewhere, but as it is more intimate with the CScheduler than the validation interface, it was abstracted out and put here. We should probably tweak up how it all works in a later PR (as we move off of the big boost threadGroup in init), but for now I'll leave it.

Odd comment, "Obviously... may..." is concerning enough that we should have a comment explaining how to avoid a deadlock rather than this vagueness or perhaps removing the statement altogether?

Any suggestions? The statement indicates that "holding cs_main/cs_wallet may cause deadlock", this is true, deadlock is not guaranteed, but may appear, thus you should obviously never call with cs_main or cs_wallet held.

I'd just drop "Obviously" now that I'm more familiar with the method.

This method is concerning. It may be that it is being used in a safe manner but the method itself is quite dangerous. Preliminary observation suggests this can be called from both the command line RPC and JSON RPC at the same time but I don't know how true this is.

Calling it from two different threads appears to be not okay, so it is "Not thread safe" and should likely be labeled as such (though I don't see this as a standard in the project codebase so maybe that's going a bit far).

I'm not sure how it is not thread safe? It blocks the current thread, not any other work.

I thought unsafe due to the "AssertLockNotHeld" that I misunderstood. My above comment is totally wrong and can be disregarded.

If these assertions need to be held for this method to execute correctly then the method cannot be thread safe as itself being called twice, in two threads, is enough to cause a failure.

The AssertLockNotHeld call only fails if the current thread holds the lock, not any thread.

boost::thread_specific_ptr<LockStack> lockstack; Didn't see that, makes perfect sense then.

This is a fine solution. All these lock inversion concerns make me wonder if there isn't a more serious issue regarding lack of proper encapsulation with some of these locks. I'm sure global locks (cs_main) don't help either, can't imagine actually needing a global lock.

This is being run in a background thread, so there are no possible lock inversions? We've had great success with DEBUG_LOCKORDER and havent had any serious deadlock issues afair since the 0.3.X era.

In commit "Add a dev notes document describing the new wallet RPC blocking"

s/who's/whose

In commit "Add a dev notes document describing the new wallet RPC blocking"

s/restults/results, s/best-known-block/best-known block,/

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

I don't understand why we would wait forever without this check. Does InvalidateBlock not trigger notifications that would lead to lastBlockProcessed being updated? And if it doesn't, shouldn't this just be fixed so the right notifications are sent?

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

Maybe consider dropping the try-lock and replacing it with lastBlockProcessedMutex.unlock(); LOCK(cs_main); lastBlockProcessedMutex.lock();. Maybe this would be a little slower in the average case where this code runs (which is rare to begin with), but it would avoid the 100ms worst case, and make the code simpler because you could also replace the while loop and timeout with a plain cv.wait(lock, pred) call.

In commit "Add CallFunctionInQueue to wait on validation interface queue drain"

Would std::move(func)

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

Maybe drop this check. Seems to be a special case of the check below which isn't actually more expensive.

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

I think all the mutex/cv/lambda/looping stuff below could be replaced by:

std::promise<void> promise;
CallFunctionInValidationInterfaceQueue([&promise]() { promise.set_value(); });
promise.get_future().wait();

In commit "Fix wallet RPC race by informing wallet of tx in sendrawtransaction"

Seems like this will result in the wallet getting two TransactionAddedToMempool notifications, which is fine but might be worth noting in the comment.

Also, not asking for this change, but would another way to do this without referencing the wallet here be to release cs_main and then wait for the other notification to be processed? (Maybe using CallFunctionInValidationInterfaceQueue like in BlockUntilSyncedToCurrentChain?)

Yes, much better to CallFunctionInValidatioInterface. Done.

In commit "Add ability to assert a lock is not held in DEBUG_LOCKORDER"

Looks like there are travis errors compiling this code (undefined FOREACH/PAIRTYPE).

In commit "Fix wallet RPC race by waiting for callbacks in sendrawtransaction"

I think you might need to release cs_main before waiting for the promise, because the wallet handler in the notification thread will want to acquire it.

FWIW, the way I implemented this in my wallet ipc branch was to call CallFunctionInValidationInterfaceQueue the same way you are doing here, but release cs_main before the PushInventory calls, and wait for the promise after the PushInventory calls. I also updated the comment.

--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -29,6 +29,7 @@
 #include "wallet/wallet.h"
 #endif
 
+#include <future>
 #include <stdint.h>
 
 #include <univalue.h>
@@ -846,7 +847,6 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
             + HelpExampleRpc("sendrawtransaction", "\"signedhex\"")
         );
 
-    LOCK(cs_main);
     RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VBOOL});
 
     // parse hex string from parameter
@@ -860,6 +860,9 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
     if (request.params.size() > 1 && request.params[1].get_bool())
         nMaxRawTxFee = 0;
 
+    std::promise<void> sent_notifications;
+    {
+    LOCK(cs_main);
     CCoinsViewCache &view = *pcoinsTip;
     bool fHaveChain = false;
     for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) {
@@ -881,15 +884,6 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
                 }
                 throw JSONRPCError(RPC_TRANSACTION_ERROR, state.GetRejectReason());
             }
-#ifdef ENABLE_WALLET
-        } else {
-            // If wallet is enabled, ensure that the wallet has been made aware
-            // of the new transaction prior to returning. This prevents a race
-            // where a user might call sendrawtransaction with a transaction
-            // to/from their wallet, immediately call some wallet RPC, and get
-            // a stale result because callbacks have not yet been processed.
-            pwalletMain->TransactionAddedToMempool(tx);
-#endif
         }
     } else if (fHaveChain) {
         throw JSONRPCError(RPC_TRANSACTION_ALREADY_IN_CHAIN, "transaction already in block chain");
@@ -897,11 +891,22 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
     if(!g_connman)
         throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
 
+    CallFunctionInValidationInterfaceQueue([&sent_notifications] { sent_notifications.set_value(); });
+    } // LOCK(cs_main)
+
     CInv inv(MSG_TX, hashTx);
     g_connman->ForEachNode([&inv](CNode* pnode)
     {
         pnode->PushInventory(inv);
     });
+
+    // Wait for any TransactionAddedToMempool notifications sent above to be
+    // processed by thw wallet. This prevents a race where a user might call
+    // sendrawtransaction with a transaction to/from their wallet, immediately
+    // call some wallet RPC, and get a stale result because callbacks have not
+    // yet been processed.
+    sent_notifications.get_future().wait();
+
     return hashTx.GetHex();
 }

Heh, beat you to it.

In commit "Fix wallet RPC race by waiting for callbacks in sendrawtransaction"

This seems right. Possible tweaks:

• should_wait_on_validationinterface seems like it is always true at this point, maybe the variable is not needed.
• Might be better to call CallFunctionInValidationInterfaceQueue before releasing cs_main, to avoid waiting for notifications that might be queued in the meantime that we don't care about.
• Might be cheaper / simpler just to hold on to the tx reference (remove std::move) instead of copying the hash to a temporary variable.
• Maybe release cs_main before the PushInventory loop.

In commit "Fix wallet RPC race by waiting for callbacks in sendrawtransaction"

Might be more efficient to wait for the promise after the PushInventory calls so they aren't blocked waiting for wallets.

I figured put it above the !g_connman check to make sure we block even if at some point in the future we support running without net/connman. Dont feel strongly either way, though

Without net/connman, I'd expect this RPC to fail immediately anyway.

const CTransactionRef& ptx, like TransactionAddedToMempool?

This isnt a new function, it would have to be changed in txmempool.h

Why filtering?

Because BLOCK and CONFLICT are passed out in BlockConnectedDisconnected (see comment in validationinterface.h which describes in what cases this is called).

This function is only introduced in a later commit.

I think you've been bitten by GitHub's obnoxious order-commits-by-date-instead-of-git-ordering bug?

Move up for better packing?

Instead of fInMempool for each wallet transaction, maybe a std::set<WalletTx*> with the mempool transactions takes less memory?

Moved it up. At some point someone can go through and mark all the bools as 1 bit, I'd think.

Move this to EnsureWalletIsAvailable?

Not all functions which want wallet need to do this, though.

In commit "Use callbacks to cache whether wallet transactions are in mempool"

Looks like you accidentally resurrected this CalculateEstimateType function in the rebase. Should remove.

Oops, fixed.

Since you're adding system includes, might as well move them to their correct place, after the user includes. Also, better if they're sorted

are used->is used?

Missing space after //

I think a better interface would be std::future<void> SyncWithValidationInterfaceQueue();

Whose implementation sets up the delayed call to a promise.set_value(), instead of this being the responsibility of the caller as it is now.

The sync functionality is the only purpose, so rather than have an overly-generic interface which allows calling any function, it's better to have a more specific interface which simplifies the interface as well as the setup work on the caller side.

Hmm..I have a few more branches which use this function for other stuff (eg one that will set a "allowed to continue processing" boolean on a CNodeState then call WakeMessageHandler). I think most of the use-cases for now could almost do fine with a future-like return value if std::future allowed you to query whether the future had yet completed. Still, since the promise-create-future-wait pattern isnt too many LoC, I suppose I'll leave it for now instead of adding a bunch of machinery just to create a special future-like return type.

I think most of the use-cases for now could almost do fine with a future-like return value if std::future allowed you to query whether the future had yet completed.

Wouldn't std::future::wait_for() with argument provide that functionality?

Orthogonally, if the intended interface really is to allow running arbitrary functions on the queue, than the current interface is fine. If it's just to sync with it, I think the interface I suggested is better.

Yea, could pull some hack like wait_for(1ns), but that's pretty gross. I think the purpose is somewhat of both - really it shouldn't be to do much more than sync, but sync may require rather arbitrary things (like calling WakeMessageHandler to wake up another thread which is waiting on some other unrelated condition in the example above). For now probably best to just leave it IMO.

Ok. btw I meant to write "with argument zero" above, so it's not hacky - it just checks if the std::future is done, without waiting.

This should come after assert(pwallet != nullptr);

Removed that line instead (we dont have similar checks anywhere else and the case is already covered by EnsureWalletIsAvailable).

I think this introduces a bug. CMainSignals::SetBestChain() might be called with a CBlockLocator object argument which goes out of scope and gets destructed before the queued m_internals->SetBestChain() is called, causing an invalid memory access. For example, this can happen in the walletInstance->SetBestChain(chainActive.GetLocator()); call in wallet.cpp).

On second thought I'm probably wrong because locator is captured by the lambda by value, not reference.

sort

This is explained here: #10286 (comment)

it now just tests if it is caught up, and if it is not, it puts a callback into the CValidationInterface queue and waits for it to trigger

I think this should be a code comment rather than a github comment!

New comment looks good. Thanks.

There's a global variable in net_processing called nTimeBestReceived which gets updated to the current time in this callback. Now that the callback is asynchronous, it won't necessarily be updated immediately. Does that cause a problem when nTimeBestReceived is used in the Broadcast callback?

I believe this to be fine. nTimeBestReceived is passed to wallet as the first parameter to ResendWalletTransactions, and is effectively just called on a loop to resend wallet transactions when we need to. If it gets called with an nTimeBestReceived that is further back than we are, that should be fine, it just won't rebroadcast until the next time its called.

Yep, sounds good. I thought this was ok, but I wasn't certain and wanted to check.

Is there a reason that you've named the validation interface callback TransactionRemovedFromMempool() but the signal is named MempoolEntryRemoved. For all other callbacks in the validation interface, the callback name matches the signal name.

For symmetry with TransactionAddedToMempool. I'd rather rename the MempoolEntryRemoved signal in CTxMempool isntead, but, really, I cant say I have hugely strong feelings. Considering I have a PR lined up after this one to rework some of the mempool parts of this interface, maybe lets leave that for the next PR?

I don't understand why this is protected by cs_main. Can you explain?

The code in BlockUntilSyncedToCurrentChain accesses this in a place that's going to need cs_main anyway (for chainActive.Tip()), could add a cs_wallet and call it protected by cs_wallet, but that just means an extra lock in BlockUntilSyncedToCurrentChain.

I added comments to explain this.

This will be set incorrectly if the rescan was aborted.

I just removed this and set it to chainActive.Tip() in CreateWalletFromFile. It doesnt make sense to try to use this to block wallet until we're caught up with the current tip in reorg, it isnt a regression so we should just leave it. When/if we expose last_block_processed via RPC (and allow wallet to return data that is stale) we should change that, I suppose.

Yes - I agree. m_last_block_processed would be a really nice value to expose in the getwalletinfo RPC.

I have a weak preference to make m_last_block_processed work as you'd expect (tracking the last block processed by the wallet), but this PR is already doing a lot, so I think it's also fine to keep this as is. Please update the comment in wallet.h to say that it's not actually tracking how far the wallet is behind chain sync and shouldn't be publicly exposed until that's changed.

OK, added more info to the wallet.h comment.

Can you move this cs_main scope further down (to avoid having to declare the CTransactionRef above and then recompute the GetHash() at the end of this function?

CTransactionRef->GetHash() is (virtually) free because we cache the hash in the CTransaction. Honestly we should probably just go the other way and remove the hashTx local. Additionally, if I'm correctly understanding your request, that would result in a deadlock as the validation interface queue may not be able to make progress as long as cs_main is held in the RPC thread. I've added a comment to validationinterface.h above CallFunctionInValidationInterfaceQueue to this effect.

Misunderstood your point, moved the scope start down.

Yes, sorry - my comment was unclear.

Change looks good.

Isn't the scheduler queue abandoned at shutdown? What keeps this from blocking forever if timed just right?

No, init.cpp calls CMainSignals().FlushBackgroundCallbacks() which will empty the CValidationInterfaceQueue. The scheduler itself is abandoned, but we shouldn't generate any new notifications by that point.

Doesn't this entire function need cs_main ?

It would appear so (though that isn't introduced here)....I'll let @practicalswift take a look since that appears to be his project atm.

Yes, according to my annotations (#11226) calling CreateWalletFromFile requires holding cs_main:

src/wallet/wallet.h:    static CWallet* CreateWalletFromFile(const std::string walletFile)
                            EXCLUSIVE_LOCKS_REQUIRED(cs_main);

This is due to the following underlying locking requirements:

• CreateWalletFromFile is reading the variable chainActive which requires holding the mutex cs_main.
• CreateWalletFromFile calls FindForkInGlobalIndex which reads the variable mapBlockIndex. Reading the variable mapBlockIndex requires holding the mutex cs_main.

Somewhat unrelated nit: AddToWallet's return value should be checked before using it here, but that was already the case.

Indeed, though in such cases not much we can do - bdb got upset, we're probably just going to shutdown soon anyway.

@TheBlueMatt Shouldn't this line be before the UnregisterBackgroundSignalScheduler?

I suppose it would be more correct, yes, though I do not believe this is a bug - mempool should absolutely, absolutely not be generating any events by this point, so if it does (which I believe would result in an assert(false) or so) it would represent some other shutdown-order bug.