Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Nov 15, 2017

Conversation

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Apr 27, 2017

Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946.

This concludes the work of #9725, #10178, and #10179.

See individual commit messages for more information.

Copy link
Contributor

ryanofsky left a comment

utACK 6fb571977d9cc41793a594688e5071dd5bbd864d.

Code changes mostly seem great, though as you can tell from my comments I have a somewhat hazy understanding of the semantics and assumptions being made. A little more documentation would make everything clear, I think.

src/txmempool.h Outdated
@@ -511,6 +511,9 @@ class CTxMemPool
// to track size/count of descendant transactions. First version of
// addUnchecked can be used to have it call CalculateMemPoolAncestors(), and
// then invoke the second version.
// Note that addUnchecked is ONLY called from ATMP during normal operation,
// and any other callers may break wallet's in-mempool tracking (due to
// lack of CValidationInterface::TransactionAddedToMempool callbacks).

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 3, 2017

Author Contributor

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.

src/txmempool.h Outdated
@@ -511,6 +511,9 @@ class CTxMemPool
// to track size/count of descendant transactions. First version of
// addUnchecked can be used to have it call CalculateMemPoolAncestors(), and
// then invoke the second version.
// Note that addUnchecked is ONLY called from ATMP during normal operation,

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

In commit "Add a CValidationInterface::TransactionRemovedFromMempool"

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

src/wallet/wallet.cpp Outdated
if (this->lastBlockProcessed == chainActive.Tip()) {
return true;
}
// If the user called invalidatechain some things might block

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

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.

src/wallet/wallet.h Outdated
* Blocks until the wallet state is up-to-date to /at least/ the current
* chain at the time this function is entered
* Obviously holding cs_main/cs_wallet when going into this call may cause
* deadlock

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

In commit "Add CWallet::BlockUntilSyncedToCurrentChain()"

Stray tab here

src/wallet/rpcwallet.cpp Outdated
@@ -2648,6 +2712,10 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)

RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR));

// Make sure the results are valid at least up to the most recent block

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

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.

test/functional/zmq_test.py Outdated
if topic == b"hashblock":
blkhash = bytes_to_hex_str(body)
else:
assert_equal(topic, b"hashtx")

msg = self.zmqSubSocket.recv_multipart()
topic = msg[0]

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 2, 2017

Contributor

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).

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch May 3, 2017
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented May 3, 2017

Rebased and fixed @ryanofsky's mostly-comment nits :).

Copy link
Contributor

mchrostowski left a comment

Overall appears to be on a good track. It looks to me like the global lock (cs_main) is causing some serious confusion/issues and there is some general mistake in the pattern of locks or their encapsulation that makes this all difficult.

I reviewed everything pretty closely aside from the ZMQ test changes, that went over my head.

Please don't overlook the outdated validationinterface.cpp comments, those took some time to put together.

src/qt/test/rpcnestedtests.cpp Outdated
@@ -12,6 +12,7 @@
#include "rpc/server.h"

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

Without them test_bitcoin-qt segfaults.

src/validationinterface.cpp Outdated
static CMainSignals g_signals;

CMainSignals::CMainSignals() {

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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.

src/scheduler.h Outdated
@@ -43,6 +43,9 @@ class CScheduler
// Call func at/after time t
void schedule(Function f, boost::chrono::system_clock::time_point t);

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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());

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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

src/validationinterface.cpp Outdated
void MaybeScheduleProcessQueue() {
{
LOCK(cs_callbacksPending);
// Try to avoid scheduling too many copies here, but if we

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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 has been minimized.

Copy link
@ryanofsky

ryanofsky May 4, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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 :/

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

@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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 4, 2017

Contributor

@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.

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

@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.

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

@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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

@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).

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

@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."

src/validationinterface.cpp Outdated
callbacksPending.pop_front();
}

// RAII the setting of fCallbacksRunning and calling MaybeScheduleProcessQueue

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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(); }

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 4, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

@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.

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

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

src/scheduler.h Outdated
* to be executed on the same thread, but no two jobs will be executed
* at the same time.
*/
class CSingleThreadedSchedulerClient {

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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.

/**
* Blocks until the wallet state is up-to-date to /at least/ the current
* chain at the time this function is entered
* Obviously holding cs_main/cs_wallet when going into this call may cause

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

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

@@ -1147,6 +1153,50 @@ void CWallet::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) {



void CWallet::BlockUntilSyncedToCurrentChain() {

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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).

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

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

src/wallet/wallet.cpp Outdated
initialChainTip = chainActive.Tip();
}
AssertLockNotHeld(cs_main);
AssertLockNotHeld(cs_wallet);

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 4, 2017

Contributor

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

src/validationinterface.cpp Outdated
@@ -104,7 +104,9 @@ void UnregisterAllValidationInterfaces() {

void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason reason) {
if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
internals->TransactionRemovedFromMempool(ptx);
internals->schedulerClient.AddToProcessQueue([ptx, this] {

This comment has been minimized.

Copy link
@mchrostowski

mchrostowski May 3, 2017

Contributor

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 comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 4, 2017

Author Contributor

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.

@mchrostowski

This comment has been minimized.

Copy link
Contributor

mchrostowski commented May 4, 2017

So far looks good to me, I'm going to poke around wallet<->blockchain interaction so I can better understand the wallet.cpp changes you made before I comment further.

That said I feel like there is something fundamentally wrong with the interaction between CWallet and the blockchain (I don't even know where that code lives yet). This feels like a solution to current issues but I would hope a refactor prevents needing such frequently occurring checks and thread specific execution.

That said, it would be interesting if a single "blockchain operations" thread is needed to get the code working in a reasonable manner. GUI systems tend to need such threads because of the nature of problem which is events coming from both ends of the system (rendering thread vs input thread) which would have to, in any logical design, invert lock ordering or else excessively block.

Copy link
Contributor

ryanofsky left a comment

utACK 943217460bc527c4003868415c264e4a77a6e55a.

Changes since previous ACK were rebasing, adding developer notes, tweaking some comments, adding a check to the zmq test

doc/developer-notes.md Outdated

- Wallet RPCs call BlockUntilSyncedToCurrentChain to maintain consistency with
`getblockchaininfo`'s state immediately prior to the call's execution. Wallet
RPCs who's behavior does *not* depend on the current chainstate may omit this

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 4, 2017

Contributor

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

s/who's/whose

doc/developer-notes.md Outdated
- *Rationale*: In previous versions of Bitcoin Core, the wallet was always
in-sync with the chainstate (by virtue of them all being updated in the
same cs_main lock). In order to maintain the behavior that wallet RPCs
return restults as of at least the highest best-known-block an RPC

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 4, 2017

Contributor

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

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented May 4, 2017

@mchrostowski thanks for the review! Generally, wallet and blockchain (essentially validation.cpp's stuff) have historically been pretty tightly coupled (updated all under the same cs_main lock). This PR is a step, however small, towards decoupling that a bit. Because the wallet still relies on "is it in our mempool?" as a proxy for "is this possibly going to get confirmed/is it spendable with the result making it into my mempool", the chainstate and mempool still need to be updated in-sync and the wallet notified of the updates in a single notification, which come in in the order they happened. Still, this is much better than the wallet actually querying the mempool/validation logic directly instead of tracking the stuff it cares about out of them.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch May 4, 2017
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented May 4, 2017

Rebased on latest #10179, current master, and fixed @ryanofsky's english corrections.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch May 5, 2017
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented May 5, 2017

utACK 2c306d7876fb57ff26d217f97415a79942094002. Changes since previous were some documentation tweaks and new block calls in "Add calls to CWallet::BlockUntilSyncedToCurrentChain()", along with the rebase.

@mchrostowski

This comment has been minimized.

Copy link
Contributor

mchrostowski commented May 13, 2017

@TheBlueMatt The stated purpose of this PR is to reduce locking on cs_main so as to reducing code coupling. I see one change in this PR that actually deletes a LOCK(cs_main) which is in CWallet::InMempool(). This looks like a step in the right direction.

That said, the remaining changes seem to be all about getting the signals into a background thread. What does this gain us for decoupling?

If the purpose is to not hold cs_main from whatever call sites hit GetMainSignals() then I don't see a benefit in using new threads when we can release the lock before making the call. That is, running in another thread is not decoupling synchronization or interface dependencies. The goal of "move wallet updates out of cs_main into a background thread" seems unrelated to decoupling because "using a background thread" and "not holding cs_main" are not dependent on each other, at least in the cases I observed.

Of particular concern is the re-ordering of calls, which you're avoiding with the single threaded scheduler, but if this isn't required for decoupling then it is just added complexity and overhead.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented May 30, 2017

This needs rebase due to a minor conflict in listunspent.

src/wallet/wallet.cpp Outdated
// If the user called invalidateblock our wait here might block
// forever, so we check if we're ahead of the tip (a state
// which should otherwise never be exposed outside of validation)
return this->lastBlockProcessed->nChainWork > chainActive.Tip()->nChainWork;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 31, 2017

Contributor

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?

src/wallet/wallet.cpp Outdated
// This should be exceedingly rare in regular usage, so potentially
// eating 100ms to retry this lock should be fine (not TRY_LOCKing
// here would be a lock inversion against lastBlockProcessedMutex)
TRY_LOCK(cs_main, mainLocked);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 31, 2017

Contributor

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.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch 3 times, most recently Jun 8, 2017
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

TheBlueMatt commented Jun 8, 2017

Rebased and rewrote CWallet::BlockUntilSyncedToCurrentChain(). Instead of the complicated fallback logic, 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 wanted to avoid having this function previously, but I ended up needing it for a different branch which moves more CValidationInterface callbacks to the background and the logic is so simple, that I went ahead with it.

Copy link
Contributor

ryanofsky left a comment

utACK 91aad9fa3b33ae387145e4b84a14c5f61cbd2494. Changes since last review were rebase, style guide fixes, BlockUntilSyncedToCurrentChain rewrite.

src/validationinterface.cpp Outdated
@@ -102,6 +102,10 @@ void UnregisterAllValidationInterfaces() {
g_signals.m_internals->NewPoWValidBlock.disconnect_all_slots();
}

void CallFunctionInValidationInterfaceQueue(std::function<void ()> func) {
g_signals.m_internals->m_schedulerClient.AddToProcessQueue(func);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 8, 2017

Contributor

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

Would std::move(func)

src/wallet/wallet.cpp Outdated
LOCK(cs_main);
const CBlockIndex* initialChainTip = chainActive.Tip();

if (m_last_block_processed == initialChainTip) {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 8, 2017

Contributor

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.

src/wallet/wallet.cpp Outdated
}
}

std::condition_variable callbacks_done_cv;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 8, 2017

Contributor

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();
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch 2 times, most recently Jun 9, 2017
Copy link
Contributor

ryanofsky left a comment

utACK ae0c83326bdae2b61c1bcffd122f78468aa2aefb. Same as previous except the last 3 review suggestions are now incorporated. There is a minor conflict now with master, but since this depends on #10179 anyway, there's probably no hurry to rebase.

src/rpc/rawtransaction.cpp Outdated
// 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);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jun 19, 2017

Contributor

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?)

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jun 21, 2017

Author Contributor

Yes, much better to CallFunctionInValidatioInterface. Done.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2017-01-wallet-cache-inmempool-4 branch 4 times, most recently Jun 21, 2017
laanwj added a commit to laanwj/bitcoin that referenced this pull request Nov 28, 2017
PR bitcoin#10286 introduced a few steps which are not robust to early shutdown
in initialization.

Stumbled upon this with bitcoin#11781, not sure if there are other scenarios
that can trigger it, but it's harden against this in any case.
laanwj added a commit to laanwj/bitcoin that referenced this pull request Nov 30, 2017
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Dec 4, 2017

According to #11822 (comment), this may be causing a memory leak

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 14, 2018
PR bitcoin#10286 introduced a few steps which are not robust to early shutdown
in initialization.

Stumbled upon this with bitcoin#11781, not sure if there are other scenarios
that can trigger it, but it's harden against this in any case.
jkczyz pushed a commit to jkczyz/bitcoin that referenced this pull request Aug 22, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Oct 31, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Oct 31, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Oct 31, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jnewbery added a commit to jnewbery/bitcoin that referenced this pull request Nov 8, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Nov 8, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Nov 11, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Nov 11, 2019
…pool

This renaming was suggested in bitcoin#10286. Additionally, it makes logging in
a proceeding commit symmetric with TransactionAddedToMempool when using
__func__.
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
…tion

d31e5c1 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan)

Pull request description:

  PR bitcoin#10286 introduced a few steps which are not robust to early shutdown in initialization.

  Stumbled upon this with bitcoin#11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

  E.g.
  ```
  $ src/bitcoind  -debuglogfile=/dfdf
  Error: Could not open debug log file /dfdf
  Program received signal SIGSEGV, Segmentation fault.
  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
  (gdb) bt
  #0  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
  #1  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
  #2  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
  #3  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19
  ```

Tree-SHA512: 7dd9570a9803514a17781bfadf1edde47e96df4e852cce2f423cab422e005fb94d44e777af1a6ea5167b04a4d889e848ae7a61a7e0e94232247ddea32ee70fc8
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
… (without cs_main)

89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)

Pull request description:

  Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946.

  This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179.

  See individual commit messages for more information.

Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
… (without cs_main)

89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)

Pull request description:

  Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946.

  This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179.

  See individual commit messages for more information.

Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 29, 2020
… (without cs_main)

89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)

Pull request description:

  Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946.

  This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179.

  See individual commit messages for more information.

Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 29, 2020
… (without cs_main)

89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)

Pull request description:

  Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946.

  This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179.

  See individual commit messages for more information.

Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
@PastaPastaPasta PastaPastaPasta mentioned this pull request Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.