CValidationInterface Cleanups

TheBlueMatt commented at 9:21 pm on February 8, 2017: member

This + #9605 + about three more and wallet updates from blocks will be in a background thread so that they do not block block connection. This PR is just some general cleanup that happened along the way or otherwise makes future changes easier. See commit messages for more details.

fanquake added the label Validation on Feb 8, 2017

fanquake added the label Wallet on Feb 8, 2017

TheBlueMatt force-pushed on Feb 18, 2017

in src/zmq/zmqnotificationinterface.cpp: in 9765f81d76 outdated

143@@ -144,8 +144,10 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co
144     }
145 }
146 
147-void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
148+void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)

sdaftuar commented at 3:22 pm on February 23, 2017:

Perhaps we should have a comment here explaining that this function is used for all transaction notifications, and not just when things are added to the mempool? I can imagine a future reader being confused about why BlockConnected and BlockDisconnected both call this function…

TheBlueMatt commented at 8:10 pm on February 23, 2017:

OK, added a few comments.

ryanofsky commented at 6:03 pm on March 7, 2017:

In commit “SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected”

Perhaps we should have a comment here

OK, added a few comments.

FWIW, not seeing a comment here. Maybe they were added elsewhere, but it would seem good to mention right in the top of the function body that this is not only called when a transaction is added to the mempool like the name would suggest. Alternately, you could leave this function named SyncTransaction and have TransactionAddedToMempool call SyncTransaction to avoid the misleading title.

TheBlueMatt commented at 8:03 pm on March 7, 2017:

Added another comment.

sdaftuar approved

sdaftuar commented at 5:18 pm on February 23, 2017: member

One comment nit in the zmq file, utACK f3ef07215b

TheBlueMatt force-pushed on Feb 23, 2017

in src/zmq/zmqnotificationinterface.cpp: in 942c45aa63 outdated

161@@ -160,3 +162,19 @@ void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CB
162         }
163     }
164 }
165+
166+void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted)

theuni commented at 8:18 pm on March 6, 2017:

To help others review, these two functions seem to mesh with current behavior. I don’t see how they could possibly be usable by any client, but there ya go.

in src/wallet/wallet.cpp: in 942c45aa63 outdated

1186-    LOCK2(cs_main, cs_wallet);
1187+void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) {
1188+    const CTransaction& tx = *ptx;
1189 
1190-    if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true))
1191+    if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true))

theuni commented at 8:29 pm on March 6, 2017:

Mind changing AddToWalletIfInvolvingMe to take a CTransactionRef here? Looks like we end up keeping 2 copies otherwise.

TheBlueMatt commented at 11:18 pm on March 6, 2017:

Fixed.

in src/validation.cpp: in 942c45aa63 outdated

2517             for (const auto& pair : connectTrace.blocksConnected) {
2518                 assert(pair.second);
2519-                const CBlock& block = *(pair.second);
2520-                for (unsigned int i = 0; i < block.vtx.size(); i++)
2521-                    GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i);
2522+                GetMainSignals().BlockConnected(pair.second, pair.first, *mrt.GetConflictedTransactions());

theuni commented at 9:18 pm on March 6, 2017:

The same set of conflicted transactions is sent here for each block.

TheBlueMatt commented at 11:13 pm on March 6, 2017:

Fixed, with a lot more code :(.

in src/wallet/wallet.cpp: in 942c45aa63 outdated

1214+
1215+    for (const CTransactionRef& ptx : vtxConflicted) {
1216+        SyncTransaction(ptx, NULL, -1);
1217+    }
1218+    for (size_t i = 0; i < pblock->vtx.size(); i++) {
1219+        SyncTransaction(pblock->vtx[i], pindex, pindex ? i : -1);

theuni commented at 10:22 pm on March 6, 2017:

pindex can no longer be NULL here.

TheBlueMatt commented at 11:15 pm on March 6, 2017:

Fixed.

in src/validation.cpp: in 942c45aa63 outdated

2510@@ -2518,14 +2511,10 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2511 
2512             // throw all transactions though the signal-interface
2513 
2514-            } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified
2515-
2516             // Transactions in the connected block are notified
2517             for (const auto& pair : connectTrace.blocksConnected) {
2518                 assert(pair.second);

theuni commented at 10:28 pm on March 6, 2017:

Assert pair.first here too, I think. It’d be nice to doc that the BlockConnected() params are now always non-null.

TheBlueMatt commented at 11:14 pm on March 6, 2017:

Fixed.

theuni commented at 10:30 pm on March 6, 2017: member

Concept ACK.

TheBlueMatt force-pushed on Mar 6, 2017

TheBlueMatt commented at 11:24 pm on March 6, 2017: member

To fix the issue @theuni found (“The same set of conflicted transactions is sent here for each block.”), I had to go rewrite a big chunk of how the mempool removal callback logic works. Now what was previously a standalone class which listens to mempool for transactions removed has gone into the ConnectTrace class, which is accessed in ActivateBestChain to generate callbacks. Also, had to rebase.

in src/validation.cpp: in 5130d7cdcc outdated

2239@@ -2240,24 +2240,23 @@ struct ConnectTrace {
2240  * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock
2241  * corresponding to pindexNew, to bypass loading it again from disk.
2242  *
2243- * The block is always added to connectTrace (either after loading from disk or by copying
2244- * pblock) - if that is not intended, care must be taken to remove the last entry in
2245- * blocksConnected in case of failure.
2246+ * The block is always added to connectTrace in the case connection succeeds.

ryanofsky commented at 2:37 pm on March 7, 2017:

In commit “Add pblock to connectTrace at the end of ConnectTip, not start”:

Maybe write “The block is added to connectTrace only if the connection succeeds.” ? “Always” and “in the case” imply contradictory things to me.

TheBlueMatt commented at 7:30 pm on March 7, 2017:

Tweaked, carryover from too-little-diff-change.

in src/validation.cpp: in 5130d7cdcc outdated

2289@@ -2291,6 +2290,8 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2290     int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
2291     LogPrint("bench", "  - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001);
2292     LogPrint("bench", "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001);
2293+
2294+    connectTrace.blocksConnected.emplace_back(pindexNew, pthisBlock);

ryanofsky commented at 2:46 pm on March 7, 2017:

In commit “Add pblock to connectTrace at the end of ConnectTip, not start”:

This does appear to be doing what the commit message says. But could you add a line like “Cleanup, no change in behavior” to the commit message if that is the intention here?

As someone interested in being able to contribute reviews, and in being able to understand the direction we’re taking the code, it’s really helpful to me when I can read a commit message that not only tells me what change the commit is making, but also provides a little hint about why the change is being made.

TheBlueMatt commented at 7:39 pm on March 7, 2017:

Sure, added more to the commit message.

TheBlueMatt force-pushed on Mar 7, 2017

TheBlueMatt commented at 3:12 pm on March 7, 2017: member

Rebased for trivial conflict from #9605.

in src/validation.cpp: in 99634d420b outdated

2232@@ -2233,7 +2233,17 @@ static int64_t nTimePostConnect = 0;
2233  * part of a single ActivateBestChainStep call.
2234  */
2235 struct ConnectTrace {
2236+private:
2237     std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
2238+
2239+public:
2240+    void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {

ryanofsky commented at 3:17 pm on March 7, 2017:

In commit “Make ConnectTrace::blocksConnected private, hide behind accessors”:

Doesn’t really matter because this is only called in a single place, and that place happens to require a copy. But if you wanted to write this in a move-compatible way (giving callers flexibility to either move or copy), you could write:

0void BlockConnected(CBlockIndex* pindex, std::shared_ptr<const CBlock> pblock) {
1    blocksConnected.emplace_back(pindex, std::move(pblock));
2}

TheBlueMatt commented at 7:31 pm on March 7, 2017:

I prefer to force peoplt to not take the atomic increment hit without changing the code :).

ryanofsky commented at 3:51 pm on March 8, 2017:

I prefer to force peoplt to not take the atomic increment hit without changing the code :).

Your version forces the atomic increment hit in every case, my version lets callers avoid it in the case where they are moving the shared pointer into the container.

TheBlueMatt commented at 5:22 pm on March 8, 2017:

Ahh, indeed, I hate this C++11 stuff, anyway, I took the proposed change.

in src/validation.cpp: in 99634d420b outdated

2239+public:
2240+    void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {
2241+        blocksConnected.emplace_back(pindex, pblock);
2242+    }
2243+
2244+    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > GetBlocksConnected() {

ryanofsky commented at 3:18 pm on March 7, 2017:

In commit “Make ConnectTrace::blocksConnected private, hide behind accessors”:

Now we are copying the vector (as well as the shared_ptrs inside) whenever it is accessed, which we weren’t doing before. Any reason for this not to return a const reference to the vector to avoid this?

Also the method itself should be marked const.

TheBlueMatt commented at 7:35 pm on March 7, 2017:

Oops, indeed. Now it just returns a reference, which should also be fine. The method is not technically const later (without making things mutable), so I’ll leave the second part.

in src/validation.cpp: in 91dc381aa7 outdated

2227         return blocksConnected;
2228     }
2229+
2230+    void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) {
2231+        if (reason == MemPoolRemovalReason::CONFLICT) {
2232+            conflictedTxs.push_back(txRemoved);

ryanofsky commented at 3:21 pm on March 7, 2017:

In commit “Handle conflicted transactions directly in ConnectTrace”:

Using std::move and emplace would be slightly more efficient. Would also make this code have a more internally consistent style.

TheBlueMatt commented at 7:53 pm on March 7, 2017:

Indeed, done.

in src/validation.cpp: in 91dc381aa7 outdated

2511@@ -2526,8 +2512,15 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2512             fInitialDownload = IsInitialBlockDownload();
2513 
2514             // throw all transactions though the signal-interface
2515-
2516-            } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified
2517+            connectTrace.CallSyncTransactionOnConflictedTransactions();

ryanofsky commented at 3:23 pm on March 7, 2017:

In commit “Handle conflicted transactions directly in ConnectTrace”:

Again, I would find it really helpful if the commit message gave a hint about why the change is being made instead of only saying what the change is.

Is this just a cleanup not intended to change behavior? Is it important that signal connect/disconnect no longer happen under cs_main? Please mention in the commit message.

TheBlueMatt commented at 7:59 pm on March 7, 2017:

It all still happens under cs_main? There are no behavior changes there. I added another line to the commit message.

ryanofsky commented at 3:56 pm on March 8, 2017:

Is it important that signal connect/disconnect no longer happen under cs_main?

It all still happens under cs_main? There are no behavior changes there. I added another line to the commit message.

The NotifyEntryRemoved.connect and disconnect calls happen in the ConnectTrace constructor/destructor above the LOCK(cs_main) call, instead of below it like before. Guessing this probably isn’t significant, but I wanted to make sure it was intentional and thought it might be worth mentioning in the commit message as a possible change in behavior.

TheBlueMatt commented at 5:51 pm on March 8, 2017:

Argh, I believe that could be a race which triggers a reachable assertion if there are two ActivateBestChain’s going at once, moved the class into the cs_main lock, which simplified the whole thing a bit.

in src/validation.cpp: in 5459aca910 outdated

2212+    std::vector<std::vector<CTransactionRef> > conflictedTxs;
2213     CTxMemPool &pool;
2214 
2215 public:
2216     ConnectTrace(CTxMemPool &_pool) : pool(_pool) {
2217+        conflictedTxs.emplace_back();

ryanofsky commented at 3:27 pm on March 7, 2017:

In commit “Keep conflictedTxs in ConnectTrace per-block”:

Could construct the vector in the desired state initially (just pass 1 or {{}} to the constructor) instead of creating it empty and then pushing.

TheBlueMatt commented at 7:43 pm on March 7, 2017:

OK, did that for the whole patch set.

in src/validation.cpp: in 01f38bf2c2 outdated

2194@@ -2195,6 +2195,12 @@ static int64_t nTimeFlush = 0;
2195 static int64_t nTimeChainState = 0;
2196 static int64_t nTimePostConnect = 0;
2197 
2198+struct PerBlockConnectTrace {
2199+    CBlockIndex* pindex = NULL;

ryanofsky commented at 3:39 pm on March 7, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”:

Maybe s/NULL/nullptr/.

TheBlueMatt commented at 7:28 pm on March 7, 2017:

Is there actually a difference between nullptr and NULL to the compiler (it overrides differently, sometimes, if you’re calling a function, no?)

ryanofsky commented at 3:41 pm on March 8, 2017:

Is there actually a difference between nullptr and NULL to the compiler (it overrides differently, sometimes, if you’re calling a function, no?)

Yes, but I only suggested nullptr because I thought it might be an oversight. (NULL seems retro to me, but if you prefer it, please keep.)

bulldozer00 commented at 8:36 am on April 10, 2017:

From Cpp Core Guidelines (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-nullptr)

ES.47: Use nullptr rather than 0 or NULL ()

Readability. Minimize surprises: nullptr cannot be confused with an int. nullptr also has a well-specified (very restrictive) type, and thus works in more scenarios where type deduction might do the wrong thing on NULL or 0.

in src/validation.cpp: in 01f38bf2c2 outdated

2225@@ -2220,28 +2226,33 @@ class ConnectTrace {
2226     }
2227 
2228     void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {
2229-        blocksConnected.emplace_back(pindex, pblock);
2230-        conflictedTxs.emplace_back();
2231+        assert(!awaitingClear);
2232+        assert(pindex && pblock);

ryanofsky commented at 3:52 pm on March 7, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”:

Maybe break assert into two lines so it’s easier to know which condition failed.

TheBlueMatt commented at 7:47 pm on March 7, 2017:

Done.

in src/validation.cpp: in 01f38bf2c2 outdated

2194@@ -2195,6 +2195,12 @@ static int64_t nTimeFlush = 0;
2195 static int64_t nTimeChainState = 0;
2196 static int64_t nTimePostConnect = 0;
2197 
2198+struct PerBlockConnectTrace {
2199+    CBlockIndex* pindex = NULL;
2200+    std::shared_ptr<const CBlock> pblock;
2201+    std::shared_ptr<std::vector<CTransactionRef> > conflictedTxs;

ryanofsky commented at 4:00 pm on March 7, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”:

Maybe s/> > /»/.

TheBlueMatt commented at 7:28 pm on March 7, 2017:

Heh, I still find the pre-C++11 version to be easier to read :P

in src/validation.cpp: in 01f38bf2c2 outdated

2237+
2238+    std::vector<PerBlockConnectTrace> GetBlocksConnected() {
2239+        if (!blocksConnected.back().pindex) {
2240+            blocksConnected.pop_back();
2241+            assert(!awaitingClear);
2242+            awaitingClear = true;

ryanofsky commented at 4:28 pm on March 7, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”:

This awaitingClear variable never seems to be used anywhere except in asserts. It would definitely be worth mentioning this in a comment on the awaitingClear declaration, because otherwise the state this class is tracking appears to be more complicated than it really is.

Also, if possible, I think it would be better to make the interface less fragile and get rid of the need for any awaitingClear variable to detect misuse. It seems you could replace the GetBlocksConnected/ClearBlocksConnected function pair with a single function:

0std::vector<PerBlockConnectTrace> ClearBlocksConnected() {
1    if (!blocksConnected.back().pindex) {
2        blocksConnected.pop_back();
3    }
4    std::vector<PerBlockConnectTrace> ret(1);
5    ret.swap(blocksConnected);
6    return ret;
7}

If you did this, another advantage would be that you could make conflictedTxs a plain vector instead of a shared_ptr to a vector, because you would no longer be creating copies of it.

TheBlueMatt commented at 7:52 pm on March 7, 2017:

Sorry, the intention of making conflictedTxs a shared_ptr is so that it can be easily passed to CValidationInterface in a later patchset, not so that we can avoid copy here, forgot to mention that when I split up the patch set.

in src/validation.cpp: in 01f38bf2c2 outdated

2235+        blocksConnected.emplace_back();
2236+    }
2237+
2238+    std::vector<PerBlockConnectTrace> GetBlocksConnected() {
2239+        if (!blocksConnected.back().pindex) {
2240+            blocksConnected.pop_back();

ryanofsky commented at 4:38 pm on March 7, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”:

Could you add a comment explaining this pop_back behavior? I thought the whole point of adding an initial null block to the blocksConnected vector was to support NotifyEntryRemoved calls prior to any BlockConnected call. But if this will now throw away the results of those calls, why not just avoid creating initial null block and have NotifyEntryRemoved do nothing if called when blocksConnected.empty()?

I’m probably missing something, so a comment here mentioning the real purpose of the pop could be helpful.

TheBlueMatt commented at 7:47 pm on March 7, 2017:

Done.

in src/wallet/wallet.cpp: in ffbac04438 outdated

1176+    SyncTransaction(ptx, NULL, -1);
1177+}
1178+
1179+void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) {
1180+    LOCK2(cs_main, cs_wallet);
1181+    // TODO: Tempoarily ensure that mempool removals are notified before

ryanofsky commented at 4:52 pm on March 7, 2017:

In commit “SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected”:

Realize you are only moving this, but this TODO comment seems to be describing what the current behavior is, not what the TODO is. Maybe remove TODO at the beginning of this comment, and add “TODO: <thing to be done>” at the end, where I’m guessing <thing to be done> is somehow fixing the race condition.

TheBlueMatt commented at 7:54 pm on March 7, 2017:

Its removed later in the PR, so probably easier to just leave it

in src/wallet/wallet.cpp: in ffbac04438 outdated

1154@@ -1155,11 +1155,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
1155     }
1156 }
1157 
1158-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
1159-{
1160-    LOCK2(cs_main, cs_wallet);

ryanofsky commented at 5:50 pm on March 7, 2017:

In commit “SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected”

Could you update the commit message to explicitly say which of the changes in the commit are changes in behavior as opposed to just internal code reorganization? Maybe the following would be accurate? “This commit doesn’t change the behavior of ZMQ notifications or network message processing in any way. It does slightly change the way the wallet processes transactions. The wallet now no longer releases cs_main and cs_wallet between transactions when it is processing multiple transactions from the same block.”

Also if this is accurate, maybe consider changing the wallet locking in a separate commit (or even a separate PR), instead of mixing code cleanup and a locking improvement in the same change.

TheBlueMatt commented at 8:00 pm on March 7, 2017:

There are no behavior changes here? Everything is still under cs_main from the previous commit.

ryanofsky commented at 4:05 pm on March 8, 2017:

The wallet now no longer releases cs_main and cs_wallet between transactions when it is processing multiple transactions from the same block.

There are no behavior changes here? Everything is still under cs_main from the previous commit.

Everything is still under cs_main, but previously cs_main and cs_wallet were released between processing individual transactions in the block, while now they are held until all transactions in the block are processed. You already mention this in your commit message, I was just suggesting moving the locking change to a separate commit so this one can be a pure refactoring, and the locking part can be understood on its own (and more easily identified and reverted if it causes problems).

TheBlueMatt commented at 5:58 pm on March 8, 2017:

Ahh, OK, split the commit.

ryanofsky commented at 2:15 pm on March 9, 2017:

In commit “SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected”:

This commit is currently removing LOCK2 at the top of SyncTransaction and then copying it 4 times over each place where SyncTransaction is called. Not a big deal, I guess, since the locks move again in the next commit. But just to be clear, what I was trying to suggest was only moving the locks in the next commit, and leaving them completely alone in this commit.

TheBlueMatt commented at 3:00 pm on March 9, 2017:

Ahh, oops, whatever, I’ll just leave it.

in src/validationinterface.h: in ffbac04438 outdated

33@@ -33,7 +34,9 @@ void UnregisterAllValidationInterfaces();
34 class CValidationInterface {

ryanofsky commented at 5:57 pm on March 7, 2017:

In commit “SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected”:

What does the part of your commit message that says “CValidationInterface starts listening to mempool directly, placing it in the middle” mean?

Wasn’t CValidationInterface always in the middle between validation code and the various listeners? And aren’t the calls to the CValidationInterface still happening in the exact same places as before, just with updated function names and arguments?

TheBlueMatt commented at 8:01 pm on March 7, 2017:

CValidationInterface has never listened to mempool, it was just an interface between the things in validation.cpp and other callers, now its also listening to mempool, putting it in the middle of the various callbacks we have going.

ryanofsky commented at 4:10 pm on March 8, 2017:

CValidationInterface has never listened to mempool, it was just an interface between the things in validation.cpp and other callers, now its also listening to mempool, putting it in the middle of the various callbacks we have going.

I guess I don’t understand what “listening to mempool” means. CValidationInterface is a class with methods, and the methods seem to be called from the same places as before. I don’t think it’s an important point, I just don’t get what this is trying to say.

TheBlueMatt commented at 6:04 pm on March 8, 2017:

Ahh, I see the issue. Yes, this is actually referring to something further in the change-set, but not in this PR (the full changeset is at https://github.com/TheBlueMatt/bitcoin/commits/2017-01-wallet-cache-inmempool, this is referring to https://github.com/TheBlueMatt/bitcoin/commit/71683dc0d8a00e16211194aaa66bfea1ae265f45).

in src/net_processing.h: in 2897190e78 outdated

29@@ -30,10 +30,10 @@ class PeerLogicValidation : public CValidationInterface {
30 public:
31     PeerLogicValidation(CConnman* connmanIn);
32 
33-    virtual void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted);
34-    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
35-    virtual void BlockChecked(const CBlock& block, const CValidationState& state);
36-    virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock);
37+    virtual void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted) override;

ryanofsky commented at 6:09 pm on March 7, 2017:

In commit “Add override to functions using CValidationInterface methods”

I think normally you drop “virtual” when you add “override” to make the two different types of method declarations more distinct. At least that’s the style I’m used to, and we seem to follow it everywhere else we’re currently using override.

TheBlueMatt commented at 8:08 pm on March 7, 2017:

OK, done.

ryanofsky commented at 6:29 pm on March 7, 2017: member

utACK d9c8399716c6688689f02da41105588ae0d72c16.

Looks good, but suggested possible improvements.

TheBlueMatt force-pushed on Mar 7, 2017

TheBlueMatt commented at 8:10 pm on March 7, 2017: member

Added a bunch of comments and additional commit description at @ryanofsky’s request. Also, note that the reason the std::vector of conflicted transactions is passed back to ActivateBestChain as a shared_ptr is to make it easy to add that to a queue of events shared between threads later, not for anything in this function, forgot to make that clear when I mentioned this is part of a much longer patchset.

TheBlueMatt force-pushed on Mar 7, 2017

in src/validation.cpp: in c36422e883 outdated

2248+        // been filled in. Thus, the last entry should always be an empty
2249+        // one waiting for the transactions from the next block. We use
2250+        // awaitingClear to keep track of this state, and pop the last
2251+        // entry here to make sure the list we return is sane.
2252+        if (!blocksConnected.back().pindex) {
2253+            blocksConnected.pop_back();

ryanofsky commented at 4:29 pm on March 8, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”

Could / should you assert(blocksConnected.back().conflictedTxs.empty()) here?

TheBlueMatt commented at 5:47 pm on March 8, 2017:

Took this change (as a part of larger changes).

in src/validation.cpp: in c36422e883 outdated

2249+        // one waiting for the transactions from the next block. We use
2250+        // awaitingClear to keep track of this state, and pop the last
2251+        // entry here to make sure the list we return is sane.
2252+        if (!blocksConnected.back().pindex) {
2253+            blocksConnected.pop_back();
2254+            assert(!awaitingClear);

ryanofsky commented at 4:36 pm on March 8, 2017:

In commit “Handle SyncTransaction in ActivateBestChain instead of ConnectTrace”

What do you think of my suggestion to get merge the ClearBlocksConnected and GetBlocksConnected methods into a single method so you can get rid of the awaitingClear variable and all the asserts?

Link: #9725 (review)

Seems better to prevent the interface misuse instead of detecting it at runtime and crashing.

TheBlueMatt commented at 5:49 pm on March 8, 2017:

Made the class use-once instead.

ryanofsky commented at 4:37 pm on March 8, 2017: member

utACK 80e7290f5b7c030c5bb2f2f108783d64732bbd10

Thanks for making changes, especially like the new comments.

TheBlueMatt force-pushed on Mar 8, 2017

ryanofsky commented at 2:19 pm on March 9, 2017: member

utACK 96370a99bc3d3728df11a4dc289514dfa6ccfcfc.

Thanks for the updates.

TheBlueMatt force-pushed on Mar 9, 2017

TheBlueMatt commented at 3:05 pm on March 9, 2017: member

Rebased to fix trivial conflict in rpc/mining.cpp

laanwj added this to the "Blockers" column in a project

in src/zmq/zmqnotificationinterface.h:11 in 892ddf12e1 outdated

 7@@ -8,6 +8,7 @@
 8 #include "validationinterface.h"
 9 #include <string>
10 #include <map>
11+#include <list>

JeremyRubin commented at 6:35 pm on March 28, 2017:

Is this really missing?

TheBlueMatt commented at 8:25 pm on March 28, 2017:

Yes, there is an std::list in CZMQNotificationInterface. It might not fail build due to current header organization, but it should be included if its used (and I believe it failed build when I was making changes).

sipa commented at 1:24 am on March 29, 2017:

Agree, we shouldn’t rely on indirect includes.

JeremyRubin commented at 2:46 am on March 29, 2017:

Ah, I was viewing the diff and didn’t realize it was truncated, so I didn’t see the list :) ACK the include.

in src/validation.cpp:2269 in bd60e24d27 outdated

2264@@ -2266,6 +2265,8 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2265     int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
2266     LogPrint("bench", "  - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001);
2267     LogPrint("bench", "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001);
2268+
2269+    connectTrace.blocksConnected.emplace_back(pindexNew, pthisBlock);

JeremyRubin commented at 6:43 pm on March 28, 2017:

Maybe use std::move on the shared_ptr because we don’t seem to use it after this point.

TheBlueMatt commented at 8:29 pm on March 28, 2017:

Done.

in src/validation.cpp:2247 in bd60e24d27 outdated

2214@@ -2215,24 +2215,23 @@ struct ConnectTrace {
2215  * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock
2216  * corresponding to pindexNew, to bypass loading it again from disk.
2217  *
2218- * The block is always added to connectTrace (either after loading from disk or by copying
2219- * pblock) - if that is not intended, care must be taken to remove the last entry in
2220- * blocksConnected in case of failure.
2221+ * The block is added to connectTrace if connection succeeds.
2222  */
2223 bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace)

JeremyRubin commented at 6:59 pm on March 28, 2017:

This is an API change probably outside the scope of this PR, but:

It might be nice to refactor ConnectTip to return an rvalue reference to a shared_ptr (and std::move pthisBlock) and have the caller decide what to do with it, because a connecttrace isn’t really relevant to ConnectTip. Failing ConnectTip can return nullptr. (Could also keep the as-is ConnectTip around, calling the suggested version, if you’re worried about any downstream API stuff).

TheBlueMatt commented at 8:27 pm on March 28, 2017:

Well the other thing is I’d like to move the new mempool disconnect cache stuff into ConnectTrace, at which point ConnectTip again will end up needing to know about the connecttrace object. Anyway, agreed, lets table this for future work.

in src/validation.cpp:2184 in a9c02d029a outdated

2182  */
2183 class ConnectTrace {
2184 private:
2185     std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
2186-    std::vector<CTransactionRef> conflictedTxs;
2187+    std::vector<std::vector<CTransactionRef> > conflictedTxs;

JeremyRubin commented at 7:08 pm on March 28, 2017:

nit: extra whitespace

TheBlueMatt commented at 8:48 pm on March 28, 2017:

Done

in src/validation.cpp:2217 in a9c02d029a outdated

2215+        for (const auto& txRemovedForBlock : conflictedTxs) {
2216+            for (const auto& tx : txRemovedForBlock) {
2217+                GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
2218+            }
2219         }
2220         conflictedTxs.clear();

JeremyRubin commented at 7:11 pm on March 28, 2017:

Worth doing a shrink_to_fit/swap on empty vector here?

0
1std::vector<std::vector<CTransactionRef>>(1).swap(conflictedTxs);

TheBlueMatt commented at 8:32 pm on March 28, 2017:

Hmm, its removed by the end and there is no equivalent vector on which to do so, so I think its fine.

in src/validation.cpp:2217 in b275002930 outdated

2214-    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > >& GetBlocksConnected() {
2215+        assert(!blocksConnected.back().pindex);
2216+        assert(pindex);
2217+        assert(pblock);
2218+        blocksConnected.back().pindex = pindex;
2219+        blocksConnected.back().pblock = std::move(pblock);

JeremyRubin commented at 7:20 pm on March 28, 2017:

I think given where we call BlockConnected you could make pblock be an rvalue and move from the caller, saving the pass by copy.

TheBlueMatt commented at 8:36 pm on March 28, 2017:

Likely, but the copy-from-rvalue is mostly free, and as you previously pointed out the caller now does std::move, so I dont think it matters.

in src/validation.cpp:2230 in b275002930 outdated

2227+        // one waiting for the transactions from the next block. We pop
2228+        // the last entry here to make sure the list we return is sane.
2229+        assert(!blocksConnected.back().pindex);
2230+        assert(blocksConnected.back().conflictedTxs->empty());
2231+        blocksConnected.pop_back();
2232         return blocksConnected;

JeremyRubin commented at 7:21 pm on March 28, 2017:

Can you set a flag so that doing things with the instance after calling GetBlocksConnected is an assert(blocks_not_gotten)?

Otherwise, could you return a pair of iterators instead?

TheBlueMatt commented at 8:39 pm on March 28, 2017:

Its already there implicitly, and I was trying to pair back the million asserts I had in this code (because if you have NotifyEntryRemoved(), you will assert the pindex on the back is empty, and BlockConnected will assert that its setting a pindex, so if you try to call either it will fail as the back().pindex is no longer null).

JeremyRubin commented at 6:43 pm on March 30, 2017:

Maybe worth explicitly differentiating the cause though – back() being null could be caused for two reasons, and if you call twice with 0 blocksconnected it will cause UB on the second call. Also, that @sdaftuar found a double call so it’s worth having this.

TheBlueMatt commented at 8:55 pm on March 30, 2017:

Wait, where does it UB? I think it always asserts if you misuse it. Anyway, no need to make asserts overly complicated, the preconditions here are well-documented, the asserts are probably just as useful to remove completely as leave there.

in src/rpc/mining.cpp:209 in c359a52e04 outdated

205@@ -207,7 +206,7 @@ UniValue generatetoaddress(const JSONRPCRequest& request)
206     if (!address.IsValid())
207         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address");
208 
209-    boost::shared_ptr<CReserveScript> coinbaseScript(new CReserveScript());
210+    std::shared_ptr<CReserveScript> coinbaseScript(new CReserveScript());

JeremyRubin commented at 7:38 pm on March 28, 2017:

please use make_shared

TheBlueMatt commented at 8:39 pm on March 28, 2017:

Done.

in src/wallet/wallet.cpp:3301 in c359a52e04 outdated

3295@@ -3296,9 +3296,9 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx)
3296     }
3297 }
3298 
3299-void CWallet::GetScriptForMining(boost::shared_ptr<CReserveScript> &script)
3300+void CWallet::GetScriptForMining(std::shared_ptr<CReserveScript> &script)
3301 {
3302-    boost::shared_ptr<CReserveKey> rKey(new CReserveKey(this));
3303+    std::shared_ptr<CReserveKey> rKey(new CReserveKey(this));

JeremyRubin commented at 7:38 pm on March 28, 2017:

make_shared

TheBlueMatt commented at 8:44 pm on March 28, 2017:

Done

in src/validationinterface.h:44 in c359a52e04 outdated

40@@ -42,7 +41,7 @@ class CValidationInterface {
41     virtual void Inventory(const uint256 &hash) {}
42     virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {}
43     virtual void BlockChecked(const CBlock&, const CValidationState&) {}
44-    virtual void GetScriptForMining(boost::shared_ptr<CReserveScript>&) {};
45+    virtual void GetScriptForMining(std::shared_ptr<CReserveScript>&) {};

JeremyRubin commented at 7:41 pm on March 28, 2017:

nit: could refactor API to pass up an rvalue reference rather than take an arg.

TheBlueMatt commented at 8:44 pm on March 28, 2017:

I’ll leave that for someone else in a later PR.

in src/wallet/wallet.cpp:969 in 3f677fe533 outdated

957@@ -958,8 +958,9 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
958  * Abandoned state should probably be more carefully tracked via different
959  * posInBlock signals or by checking mempool presence when necessary.
960  */
961-bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate)
962+bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate)

JeremyRubin commented at 7:48 pm on March 28, 2017:

You probably don’t need to use a ref to a CTransactionRef because CWalletTx is going to make a copy anyways – you could just move it from the CWalletTx constructor.

TheBlueMatt commented at 8:43 pm on March 28, 2017:

CWalletTx now copies the ref that is passed in, so using a ref here should avoid a complete copy?

JeremyRubin commented at 6:53 pm on March 30, 2017:

The CWalletTx constructor is:

0CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) : CMerkleTx(std::move(arg))

I was suggesting to add a constructor

0CWalletTx(const CWallet* pwalletIn, CTransactionRef&& arg) : CMerkleTx(arg)

and then update L984 to

0            CWalletTx wtx(this, std::move(ptx));

and update this line to

0bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef ptx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate)

There are two reasons:

slightly more clear intent (AddToWalletIfInvolvingMe does get ownership)
Eliminates an indirection

TheBlueMatt commented at 8:53 pm on March 30, 2017:

Ahh, OK, I missed your original point. Anyway, I would still prefer to leave it because it is a smaller diff to not change CWalletTx, and the way it is you dont end up with a variable lying around which has already been std::move()d prior to the end of the function, which I dont super like doing. Anyway, feel free to clean it up in a future PR (also maybe we need a general “when to && vs const& vs copy-and-move style guide)

JeremyRubin commented at 7:50 pm on March 28, 2017: contributor

I’ve reviewed most of this and it seems reasonable. I left some feedback on some minor ways to improve it, but I don’t think anything is a hold up.

TheBlueMatt force-pushed on Mar 28, 2017

sipa commented at 2:13 am on March 29, 2017: member

utACK acac363e574470be7ca26ab43b7c32c2beac9d9c.

in src/validation.cpp:2178 in ff3d20571b outdated

2172@@ -2206,19 +2173,46 @@ static int64_t nTimePostConnect = 0;
2173 /**
2174  * Used to track blocks whose transactions were applied to the UTXO state as a
2175  * part of a single ActivateBestChainStep call.
2176+ *
2177+ * This class also tracks transactions that are removed from the mempool as
2178+ * conflicts and and can be used to pass all those transactions through

sdaftuar commented at 7:11 pm on March 29, 2017:

nit: “and and”

TheBlueMatt commented at 1:07 am on March 30, 2017:

Fixed.

in src/validation.cpp:2513 in 98ed4e0c9c outdated

2525+
2526             // Transactions in the connected block are notified
2527-            for (const auto& pair : connectTrace.GetBlocksConnected()) {
2528-                assert(pair.second);
2529-                const CBlock& block = *(pair.second);
2530+            for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {

sdaftuar commented at 7:41 pm on March 29, 2017:

This is a second call to GetBlocksConnected(), which violates the single-use requirement.

TheBlueMatt commented at 1:11 am on March 30, 2017:

Fixed.

sdaftuar commented at 7:48 pm on March 29, 2017: member

Looks like one of the intermediate commits is broken (but the issue goes away in a later commit).

TheBlueMatt force-pushed on Mar 30, 2017

kallewoof commented at 1:39 am on April 3, 2017: member

utACK 312683b23752bd9bdae365d0695f1ed73d3c1f3b

sipa commented at 8:19 am on April 6, 2017: member

Needs rebäse.

Include missing #include in zmqnotificationinterface.h f5e9a019a4

Add pblock to connectTrace at the end of ConnectTip, not start

This makes ConnectTip responsible for the ConnectTrace instead
of splitting the logic between ActivateBestChainStep and ConnectTip

822000cf82

Make ConnectTrace::blocksConnected private, hide behind accessors 29e6e231c8

Handle conflicted transactions directly in ConnectTrace d3167ba9bb

Keep conflictedTxs in ConnectTrace per-block a1476877ce

Handle SyncTransaction in ActivateBestChain instead of ConnectTrace

This makes a later change to move it all into one per-block callback
simpler.

f404334910

SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected

This simplifies fixing the wallet-returns-stale-info issue as we
can now hold cs_wallet across an entire block instead of only
per-tx (though we only actually do so in the next commit).

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.

This change adds a parameter to BlockConnectedDisconnected which
lists the transactions which were removed from mempool due to
confliction as a result of this operation. While its somewhat of a
shame to make block-validation-logic generate a list of mempool
changes to be included in its generated callbacks, fixing this isnt
too hard.

Further in this change-set, CValidationInterface starts listening
to mempool directly, placing it in the middle and giving it a bit
of logic to know how to route notifications from block-validation,
mempool, etc (though not listening for conflicted-removals yet).

461e49fee2

Hold cs_wallet for whole block [dis]connection processing

This simplifies fixing the wallet-returns-stale-info issue as we
now hold cs_wallet across an entire block instead of only per-tx.

e6d5e6cbbe

Add override to functions using CValidationInterface methods acad82f375

Remove dead-code tracking of requests for blocks we generated 91f1e6ce5e

Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining 1c95e2f9c9

Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy b1a6d4cd56

TheBlueMatt force-pushed on Apr 7, 2017

TheBlueMatt commented at 9:54 am on April 7, 2017: member

Rebased trivially.

JeremyRubin commented at 2:53 pm on April 10, 2017: contributor

utack 461e49f

theuni commented at 4:37 pm on April 10, 2017: member

cautious utACK b1a6d4cd560fbdb66506841860db03c08ea4bbbc.

laanwj merged this on Apr 10, 2017

laanwj closed this on Apr 10, 2017

laanwj referenced this in commit 67023e9004 on Apr 10, 2017

in src/wallet/wallet.cpp:969 in b1a6d4cd56

965@@ -966,8 +966,9 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
966  * Abandoned state should probably be more carefully tracked via different
967  * posInBlock signals or by checking mempool presence when necessary.
968  */
969-bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate)
970+bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate)

jnewbery commented at 8:39 pm on April 10, 2017:

Now that AddToWalletIfInvolvingMe() is called with pindexBlockConnected set to NULL when the transaction isn’t in a block, can we remove the magic -1 value for posInBlock (and change the if (posInBlock != -1) into if (pindexBlockConnected != NULL))?

TheBlueMatt commented at 8:48 pm on April 10, 2017:

Sure, go for it. Still gonna have to pass in two parameters either way, sadly :/.

jnewbery commented at 8:40 pm on April 10, 2017: member

concept ACK, and weak utACK (although I’m not familiar enough with the interface to be confident that I haven’t missed anything).

One nit that can be addressed in a future PR.

jtimon commented at 11:40 pm on April 12, 2017: contributor

Nice to learn about ‘override’

fanquake moved this from the "Blockers" to the "For backport" column in a project

fanquake moved this from the "For backport" to the "Blockers" column in a project

fanquake removed this from the "Blockers" column in a project

laanwj referenced this in commit 927a1d7d08 on Nov 15, 2017

PastaPastaPasta referenced this in commit 1883727159 on May 20, 2019

PastaPastaPasta referenced this in commit bc29d651ba on May 20, 2019

PastaPastaPasta referenced this in commit ca843143f9 on May 21, 2019

PastaPastaPasta referenced this in commit 11facdfa8a on May 22, 2019

PastaPastaPasta referenced this in commit b050707d14 on May 22, 2019

PastaPastaPasta referenced this in commit c6564e4cca on May 22, 2019

PastaPastaPasta referenced this in commit 2496ec5914 on May 22, 2019

PastaPastaPasta referenced this in commit faf502fd14 on May 23, 2019

UdjinM6 referenced this in commit 321a93611c on May 23, 2019

PastaPastaPasta referenced this in commit fec5433e47 on May 23, 2019

PastaPastaPasta referenced this in commit 60438257a5 on May 27, 2019

barrystyle referenced this in commit b6b5c39f2c on Jan 22, 2020

PastaPastaPasta referenced this in commit 61758874b8 on Feb 13, 2020

PastaPastaPasta referenced this in commit 76ed385c03 on Feb 13, 2020

PastaPastaPasta referenced this in commit 3eb80fdcd5 on Feb 29, 2020

PastaPastaPasta referenced this in commit 803530c83d on Feb 29, 2020

furszy referenced this in commit 6143f803b2 on Jan 30, 2021

ckti referenced this in commit 38a9ad2b41 on Mar 28, 2021

gades referenced this in commit 1459aa5a72 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021

CValidationInterface Cleanups #9725