Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

CValidationInterface Cleanups #9725

Merged
merged 12 commits into from Apr 10, 2017

Conversation

TheBlueMatt
Copy link
Contributor

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.

Copy link
Member

@sdaftuar sdaftuar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment nit in the zmq file, utACK f3ef07215b

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, added a few comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another comment.

@TheBlueMatt TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from f3ef072 to e16f6b0 Compare February 23, 2017 20:10
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true))
if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, with a lot more code :(.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pindex can no longer be NULL here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@theuni
Copy link
Member

theuni commented Mar 6, 2017

Concept ACK.

@TheBlueMatt 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
Copy link
Contributor Author

TheBlueMatt commented Mar 6, 2017

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 TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from c79226b to d9c8399 Compare March 7, 2017 15:12
@TheBlueMatt
Copy link
Contributor Author

Rebased for trivial conflict from #9605.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK d9c8399716c6688689f02da41105588ae0d72c16.

Looks good, but suggested possible improvements.

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added more to the commit message.

std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;

public:
void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

blocksConnected.emplace_back(pindex, pblock);
}

std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > GetBlocksConnected() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, done.


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, OK, split the commit.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done.

@TheBlueMatt 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
Copy link
Contributor Author

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 TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-1 branch from 6d38a85 to 80e7290 Compare March 7, 2017 23:18
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 80e7290f5b7c030c5bb2f2f108783d64732bbd10

Thanks for making changes, especially like the new comments.

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

struct PerBlockConnectTrace {
CBlockIndex* pindex = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;

public:
void BlockConnected(CBlockIndex* pindex, const std::shared_ptr<const CBlock>& pblock) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Handle SyncTransaction in ActivateBestChain instead of ConnectTrace"

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the class use-once instead.

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

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 96370a99bc3d3728df11a4dc289514dfa6ccfcfc.

Thanks for the updates.

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Rebased to fix trivial conflict in rpc/mining.cpp

Copy link
Contributor

@JeremyRubin JeremyRubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we shouldn't rely on indirect includes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth doing a shrink_to_fit/swap on empty vector here?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.