CValidationInterface Cleanups #9725

TheBlueMatt · 2017-02-08T21:21:49Z

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.

sdaftuar

One comment nit in the zmq file, utACK f3ef07215b

sdaftuar · 2017-02-23T15:22:53Z

src/zmq/zmqnotificationinterface.cpp

@@ -144,8 +144,10 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co
    }
 }

-void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
+void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)


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

OK, added a few comments.

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.

Added another comment.

theuni · 2017-03-06T20:18:37Z

src/zmq/zmqnotificationinterface.cpp

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


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.

theuni · 2017-03-06T20:29:10Z

src/wallet/wallet.cpp


-    if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true))
+    if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true))


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

theuni · 2017-03-06T21:18:04Z

src/validation.cpp

-                const CBlock& block = *(pair.second);
-                for (unsigned int i = 0; i < block.vtx.size(); i++)
-                    GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i);
+                GetMainSignals().BlockConnected(pair.second, pair.first, *mrt.GetConflictedTransactions());


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

Fixed, with a lot more code :(.

theuni · 2017-03-06T22:22:29Z

src/wallet/wallet.cpp

+        SyncTransaction(ptx, NULL, -1);
+    }
+    for (size_t i = 0; i < pblock->vtx.size(); i++) {
+        SyncTransaction(pblock->vtx[i], pindex, pindex ? i : -1);


pindex can no longer be NULL here.

theuni · 2017-03-06T22:28:00Z

src/validation.cpp

@@ -2518,14 +2511,10 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,

            // throw all transactions though the signal-interface

-            } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified
-
            // Transactions in the connected block are notified
            for (const auto& pair : connectTrace.blocksConnected) {
                assert(pair.second);


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

theuni · 2017-03-06T22:30:59Z

Concept ACK.

TheBlueMatt · 2017-03-06T23:24:52Z

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.

TheBlueMatt · 2017-03-07T15:12:50Z

Rebased for trivial conflict from #9605.

ryanofsky

utACK d9c8399716c6688689f02da41105588ae0d72c16.

Looks good, but suggested possible improvements.

ryanofsky · 2017-03-07T14:37:06Z

src/validation.cpp

- * The block is always added to connectTrace (either after loading from disk or by copying
- * pblock) - if that is not intended, care must be taken to remove the last entry in
- * blocksConnected in case of failure.
+ * The block is always added to connectTrace in the case connection succeeds.


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.

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

ryanofsky · 2017-03-07T14:46:27Z

src/validation.cpp

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


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.

Sure, added more to the commit message.

ryanofsky · 2017-03-07T15:17:47Z

src/validation.cpp

    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
+
+public:
+    void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {


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:

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

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

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.

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

ryanofsky · 2017-03-07T15:18:01Z

src/validation.cpp

+        blocksConnected.emplace_back(pindex, pblock);
+    }
+
+    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > GetBlocksConnected() {


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.

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.

ryanofsky · 2017-03-07T15:21:33Z

src/validation.cpp

+
+    void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) {
+        if (reason == MemPoolRemovalReason::CONFLICT) {
+            conflictedTxs.push_back(txRemoved);


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.

Indeed, done.

ryanofsky · 2017-03-07T16:52:29Z

src/wallet/wallet.cpp

+
+void CWallet::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) {
+    LOCK2(cs_main, cs_wallet);
+    // TODO: Tempoarily ensure that mempool removals are notified before


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.

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

ryanofsky · 2017-03-07T17:50:26Z

src/wallet/wallet.cpp

@@ -1155,11 +1155,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
    }
 }

-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
-{
-    LOCK2(cs_main, cs_wallet);


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.

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

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

Ahh, OK, split the commit.

ryanofsky · 2017-03-07T17:57:53Z

src/validationinterface.h

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


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?

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.

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.

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 TheBlueMatt@71683dc).

ryanofsky · 2017-03-07T18:03:49Z

src/zmq/zmqnotificationinterface.cpp

@@ -144,8 +144,10 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co
    }
 }

-void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
+void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)


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.

ryanofsky · 2017-03-07T18:09:31Z

src/net_processing.h

-    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
-    virtual void BlockChecked(const CBlock& block, const CValidationState& state);
-    virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock);
+    virtual void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& vtxConflicted) override;


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 · 2017-03-07T20:10:00Z

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.

ryanofsky

utACK 80e7290f5b7c030c5bb2f2f108783d64732bbd10

Thanks for making changes, especially like the new comments.

ryanofsky · 2017-03-08T15:41:11Z

src/validation.cpp

@@ -2195,6 +2195,12 @@ static int64_t nTimeFlush = 0;
 static int64_t nTimeChainState = 0;
 static int64_t nTimePostConnect = 0;

+struct PerBlockConnectTrace {
+    CBlockIndex* pindex = NULL;


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

ryanofsky · 2017-03-08T15:51:02Z

src/validation.cpp

    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
+
+public:
+    void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {


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.

ryanofsky · 2017-03-08T15:56:02Z

src/validation.cpp

@@ -2526,8 +2512,15 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
            fInitialDownload = IsInitialBlockDownload();

            // throw all transactions though the signal-interface
-
-            } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified
+            connectTrace.CallSyncTransactionOnConflictedTransactions();


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.

ryanofsky · 2017-03-08T16:05:28Z

src/wallet/wallet.cpp

@@ -1155,11 +1155,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
    }
 }

-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
-{
-    LOCK2(cs_main, cs_wallet);


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

ryanofsky · 2017-03-08T16:10:49Z

src/validationinterface.h

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


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.

ryanofsky · 2017-03-08T16:29:06Z

src/validation.cpp

+        // awaitingClear to keep track of this state, and pop the last
+        // entry here to make sure the list we return is sane.
+        if (!blocksConnected.back().pindex) {
+            blocksConnected.pop_back();


In commit "Handle SyncTransaction in ActivateBestChain instead of ConnectTrace"

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

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

ryanofsky · 2017-03-08T16:36:39Z

src/validation.cpp

+        // entry here to make sure the list we return is sane.
+        if (!blocksConnected.back().pindex) {
+            blocksConnected.pop_back();
+            assert(!awaitingClear);


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

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

Made the class use-once instead.

ryanofsky

utACK 96370a99bc3d3728df11a4dc289514dfa6ccfcfc.

Thanks for the updates.

ryanofsky · 2017-03-09T14:15:20Z

src/wallet/wallet.cpp

@@ -1155,11 +1155,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx)
    }
 }

-void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock)
-{
-    LOCK2(cs_main, cs_wallet);


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.

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

TheBlueMatt · 2017-03-09T15:05:22Z

Rebased to fix trivial conflict in rpc/mining.cpp

JeremyRubin

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.

JeremyRubin · 2017-03-28T18:35:34Z

src/zmq/zmqnotificationinterface.h

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


Is this really missing?

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

Agree, we shouldn't rely on indirect includes.

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

JeremyRubin · 2017-03-28T18:43:38Z

src/validation.cpp

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


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

JeremyRubin · 2017-03-28T18:59:09Z

src/validation.cpp

- * The block is always added to connectTrace (either after loading from disk or by copying
- * pblock) - if that is not intended, care must be taken to remove the last entry in
- * blocksConnected in case of failure.
+ * The block is added to connectTrace if connection succeeds.
 */
 bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace)


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

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.

JeremyRubin · 2017-03-28T19:08:37Z

src/validation.cpp

 */
 class ConnectTrace {
 private:
    std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
-    std::vector<CTransactionRef> conflictedTxs;
+    std::vector<std::vector<CTransactionRef> > conflictedTxs;


nit: extra whitespace

JeremyRubin · 2017-03-28T19:11:26Z

src/validation.cpp

+        for (const auto& txRemovedForBlock : conflictedTxs) {
+            for (const auto& tx : txRemovedForBlock) {
+                GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
+            }
        }
        conflictedTxs.clear();


Worth doing a shrink_to_fit/swap on empty vector here?

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

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

JeremyRubin · 2017-03-28T19:21:41Z

src/validation.cpp

+        // the last entry here to make sure the list we return is sane.
+        assert(!blocksConnected.back().pindex);
+        assert(blocksConnected.back().conflictedTxs->empty());
+        blocksConnected.pop_back();
        return blocksConnected;


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?

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

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.

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.

JeremyRubin · 2017-03-28T19:38:23Z

src/rpc/mining.cpp

@@ -207,7 +206,7 @@ UniValue generatetoaddress(const JSONRPCRequest& request)
    if (!address.IsValid())
        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address");

-    boost::shared_ptr<CReserveScript> coinbaseScript(new CReserveScript());
+    std::shared_ptr<CReserveScript> coinbaseScript(new CReserveScript());


please use make_shared

JeremyRubin · 2017-03-28T19:38:46Z

src/wallet/wallet.cpp

 {
-    boost::shared_ptr<CReserveKey> rKey(new CReserveKey(this));
+    std::shared_ptr<CReserveKey> rKey(new CReserveKey(this));


make_shared

JeremyRubin · 2017-03-28T19:41:30Z

src/validationinterface.h

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


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

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

JeremyRubin · 2017-03-28T19:48:55Z

src/wallet/wallet.cpp

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


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.

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

The CWalletTx constructor is:

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

I was suggesting to add a constructor

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

and then update L984 to

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

and update this line to

bool 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

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)

b1a6d4c Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo) 1c95e2f Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining (Matt Corallo) 91f1e6c Remove dead-code tracking of requests for blocks we generated (Matt Corallo) acad82f Add override to functions using CValidationInterface methods (Matt Corallo) e6d5e6c Hold cs_wallet for whole block [dis]connection processing (Matt Corallo) 461e49f SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected (Matt Corallo) f404334 Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo) a147687 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo) d3167ba Handle conflicted transactions directly in ConnectTrace (Matt Corallo) 29e6e23 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo) 822000c Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo) f5e9a01 Include missing #include in zmqnotificationinterface.h (Matt Corallo) Tree-SHA512: 8893d47559da3b28d2ef7359768547cba8a4b43b6f891d80f5848f995a84b1517bfb0f706fdc8cd43f09a1350349eb440d9724a59363ab517dfcc4fcb31b2018

jnewbery

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.

jnewbery · 2017-04-10T20:39:24Z

src/wallet/wallet.cpp

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


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

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

jtimon · 2017-04-12T23:40:44Z

Nice to learn about 'override'

…ut 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 #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. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

b1a6d4c Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo) 1c95e2f Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining (Matt Corallo) 91f1e6c Remove dead-code tracking of requests for blocks we generated (Matt Corallo) acad82f Add override to functions using CValidationInterface methods (Matt Corallo) e6d5e6c Hold cs_wallet for whole block [dis]connection processing (Matt Corallo) 461e49f SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected (Matt Corallo) f404334 Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo) a147687 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo) d3167ba Handle conflicted transactions directly in ConnectTrace (Matt Corallo) 29e6e23 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo) 822000c Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo) f5e9a01 Include missing #include in zmqnotificationinterface.h (Matt Corallo) Tree-SHA512: 8893d47559da3b28d2ef7359768547cba8a4b43b6f891d80f5848f995a84b1517bfb0f706fdc8cd43f09a1350349eb440d9724a59363ab517dfcc4fcb31b2018

… (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

…llet 98d770f CScheduler boost->std::function, use millisecs for times, not secs (Matt Corallo) 67e068c Remove now unneeded ChainTip signal (furszy) bcdd3e9 Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. (furszy) b799070 Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo) d77244c Remove dead-code tracking of requests for blocks we generated (Matt Corallo) 10ccbbf Hold cs_wallet for whole block [dis]connection processing (Matt Corallo) 1a45036 fix compiler warning member functions not marked as 'override' (furszy) d3867a7 An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 (Matt Corallo) f5ac648 [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure (furszy) 8704d9d Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo) 6dcb6b0 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo) 50d3e0e Handle conflicted transactions directly in ConnectTrace (furszy) 27fb897 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo) 60329da Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo) e7c2789 Include missing #include in zmqnotificationinterface.h (Matt Corallo) 1b396b8 Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler (furszy) 4cb5820 Better document usage of SyncTransaction (Alex Morcos) 21be4e2 Introduce MemPoolConflictRemovalTracker (Alex Morcos) 7326acb mempool: add notification for added/removed entries (Wladimir J. van der Laan) a8605d2 Clean up tx prioritization when conflict mined (Casey Rodarmor) e7db9ff remove internal tracking of mempool conflicts for reporting to wallet (Alex Morcos) 76c72c6 remove external usage of mempool conflict tracking (Alex Morcos) ef429af tests: Stop node before removing the notification file (furszy) 15a21c2 tests: write the notification to different files to avoid race condition (Chun Kuan Lee) 466e97a [Wallet] Simplify InMempool (furszy) 85e18f0 Rename FlushWalletDB -> CompactWalletDB, add function description (Matt Corallo) 00f36ea Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (Matt Corallo) Pull request description: Revamped the validation interface interaction with the wallet, encapsulating and improving the mempool and block signaling and each of the wallet signals handler. Restructured the Sapling witnesses and nullifiers update under the new signals. Solved several bugs that found on the way as well (Check each commit description). This PR concludes with #1726 long road :). Pushing our repository around 2 years ahead in btc time in the mempool and validation interface areas (without including RBF). The new validation -> wallet interaction architecture will enable further, and much more user facing important, improvements for the syncing process, overall software responsiveness and multithreading locking issues. Adapted backports: dashpay#6464 --> Always clean up manual transaction prioritization (mempool) bitcoin#9240 --> Remove txConflicted (mempool) bitcoin#9371 --> Notify on removal (mempool) bitcoin#9605 --> Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (walletdb) bitcoin#9725 --> CValidationInterface Cleanups (wallet, validation and validation interface) ACKs for top commit: random-zebra: utACK 98d770f Fuzzbawls: utACK 98d770f Tree-SHA512: 84c86567c2bff36b859b2ae73c558956a70dff0fffb4f73208708d92165b851bf42d35246410238c66c7a4b77e5bf51ec93885234a75fa48901fd182d2f70a28

… (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

fanquake added Validation Wallet labels Feb 8, 2017

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from ece78cc to f3ef072 Compare February 18, 2017 18:40

sdaftuar approved these changes Feb 23, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from f3ef072 to e16f6b0 Compare February 23, 2017 20:10

theuni reviewed Mar 6, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch 3 times, most recently from 769e709 to c79226b Compare March 6, 2017 23:21

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from c79226b to d9c8399 Compare March 7, 2017 15:12

ryanofsky reviewed Mar 7, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch 3 times, most recently from 61ba00d to 6d38a85 Compare March 7, 2017 20:08

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from 6d38a85 to 80e7290 Compare March 7, 2017 23:18

ryanofsky reviewed Mar 8, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from 80e7290 to 96370a9 Compare March 8, 2017 17:58

ryanofsky reviewed Mar 9, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from 96370a9 to 3f677fe Compare March 9, 2017 15:05

JeremyRubin reviewed Mar 28, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from 3f677fe to acac363 Compare March 28, 2017 20:48

TheBlueMatt mentioned this pull request Apr 10, 2017

Remove CValidationInterface::UpdatedTransaction #10178

Merged

laanwj merged commit b1a6d4c into bitcoin:master Apr 10, 2017

jnewbery reviewed Apr 10, 2017

View reviewed changes

practicalswift mentioned this pull request Apr 10, 2017

[trivial] Fix typos (tempoarily → temporarily, inadvertantly → inadvertently) #10180

Merged

jnewbery mentioned this pull request Apr 11, 2017

Remove SYNC_TRANSACTION_NOT_IN_BLOCK magic number #10186

Merged

TheBlueMatt mentioned this pull request Apr 27, 2017

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

Merged

furszy mentioned this pull request Aug 5, 2020

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

21 tasks

furszy mentioned this pull request Jan 21, 2021

Revamping block validation interface interaction with wallet PIVX-Project/PIVX#2150

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true))
		if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true))

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

CValidationInterface Cleanups #9725

CValidationInterface Cleanups #9725

Uh oh!

Conversation

TheBlueMatt commented Feb 8, 2017

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Mar 6, 2017

Uh oh!

TheBlueMatt commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Mar 7, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TheBlueMatt commented Mar 6, 2017 •

edited

Loading