Fix block-connection performance regression

[TheBlueMatt]

@JeremyRubin found a performance regression introduced by b3b3c2a which causes us to make copies of every transaction we connect to tip a while back, but no one had gotten around to fixing it yet. By switching to using shared_ptrs for blocks in several places we not only fix the performance regression but open up the possibility of switching GetMainSignals() callbacks to background threads much more easily.

[sipa]

Don't you need a fallback for when the chainwork is identical?

[TheBlueMatt]

Since its only used in each ActivateBestChainStep call individually, there is only ever one thing per given height/total work, but the map came out of an older version of the code and is now useless...so I'm replacing it with a vector.

[JeremyRubin]

utACK

couple things that could be cleaned up, but I think this is the right approach for this and is OK as is, certainly better than txChanged.

[JeremyRubin]

nit: maybe do this as

it = blockTrace.mapBlocks.emplace(pindexNew, std::shared_ptr<const CBlock>(new CBlock())).first;

Or

auto pblockNew = std::make_shared<CBlock>();

and update following code accordingly

[TheBlueMatt]

The new version should be cleaner.

[JeremyRubin]

See other comment about using make_shared

[JeremyRubin]

make_shared

[JeremyRubin]

make_shared

[JeremyRubin]

make_shared

[JeremyRubin]

make_shared

[JeremyRubin]

Semantics of passing shared_ptr by reference are a bit wonky. I think it is correct in this use case though. Perhaps it would be better for maintainability to eat the performance cost of an atomic increment and pass by value, or pass by const reference (if possible?).

[sipa]

In my opinion, passing by reference is preferred at any time when you're sure the shared_ptr itself (not the object it points to) is only accessed from a single thread. The cost of the atomic increment is negligable here though, so I don't care in this particular case.

[JeremyRubin]

make_shared

[TheBlueMatt]

The shared_ptrs are to a const CBlock, and IIRC you cant blindly cast a shared_ptr to a shared_ptr, but you need to access the block as non-const, and I kinda prefer this over a const_cast.

[JeremyRubin]

implicit copy-cast should work...

#include <memory>
int main() {
    auto a = std::make_shared<int>(10);
    std::shared_ptr<const int> b = a;
}

[TheBlueMatt]

Cool, didnt realize that was a thing!

[JeremyRubin]

I think it may be worthwhile for you to make this a class and keep the implementation details hidden...

[TheBlueMatt]

I added txConflicted here to make diffs of individual commits easier, which now requires a lot of access to the contents directly :/ Not sure its worth it.

[TheBlueMatt]

OK, ended up rewriting the PR quite a bit, cleaning it up and resolving the nits folks had raised - it now no longer has this crazy map thing that can be used as both a paramter and a return value.

[JeremyRubin]

re-utack, much cleaner!

[sipa]

Can you use a doxygen compatible comment here?

[sipa]

This logic needs to be duplicated in the disconnect logic above. It's possible that an invalid block is received which triggers a reorg. So we start from chain A-B, and receive A-C-D, with D invalid. We'll first disconnect B, then successfully connect C, but fail when connecting D. We'll loop back then, disconnect C, and reconnect B. Your code will (I believe) report both C and B as newly connected, while it should be nothing at all.

Perhaps it's easier to do a filtering step in ActivateBestChain that removes the errorneously blocks in the trace.

[TheBlueMatt]

Hmm? I dont believe so? Note that the callbacks in question are called from ABC after each ABCS call, and might happen for blocks which will be reorged out in the next ABCS call (this is the existing behavior).

[sipa]

Ok (answered elsewhere).

[sipa]

Unintentional new lines?

[sipa]

Typo: deleted

[sipa]

No, cs_main is held during the entirety of the attempted reorg here, and the callbacks only fire after the whole thing (I think).

[TheBlueMatt]

I'm very confused. ActivateBestChain has always locked cs_main, called ActivateBestChainStep once, and then unlocked cs_main and made callbacks before repeating. In the old code we pushed transactions to update onto the callback queue at the end of ConnectTip and never removed them (since there will never be a DisconnectTip call within the same cs_main in ActivateBestChainStep since all DisconnectTips happen before ConnectTip).

[sipa]

It seems that failed reorgs are an exception to what I said before, so ignore what I said. I think we should fix this (it could lead to GBT working on the forking point of a failed reorg), but that's outside of the scope here.

[sipa]

Before headers first, reorgs were dealt with by accumulating the changes in
another cache level, which was only flushed if the reorg succeeded.

Now there is much less a concept of reorgs, and only attempts to improve
the tip. ABC finds what block we want and like to be the tip, and calls
ABCS to make progress towards that. In case of failure, we find what other
block we'd like to become the tip (which may be the previous one), and make
progress towards that. cs_main is only released once we're in a strictly
better state than what we started off in, as otherwise you'd briefly expose
the forking point to GBT during a reorg.

EDIT: see #9014 (comment) - I don't think there is an issue for this PR.

[sipa]

utACK 3f78bc176a3c612f320bce4516aadcd6234c0850. My nit is just a suggestion.

[sipa]

Ultranit: if you swap this line with the one above, you can pass std::move(pblockNew) to emplace_back, avoiding an atomic increment + decrement when pblockNew goes out of scope.

[TheBlueMatt]

No, because pblockNew is a std::shared_ptr not an std::shared_ptr, so you must call the std::shared_ptr constructor to convert it.

[sipa]

I see!

[JeremyRubin]

(if anyone else gets confused, Matt meant std::shared_ptr not std::atomic above)

[sipa]

Needs rebase.

[TheBlueMatt]

Rebased.

[fanquake]

Travis failures:

  CXX      libbitcoin_server_a-rest.o
../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());

  CXX      libbitcoin_server_a-torcontrol.o
../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());

../../src/main.cpp: In function ‘bool ProcessMessage(CNode*, std::string, CDataStream&, int64_t, const CChainParams&, CConnman&)’:
../../src/main.cpp:6094:52: error: ‘block’ was not declared in this scope
             forceProcessing |= MarkBlockAsReceived(block.GetHash());

[paveljanik]

Looks like this is not rebased correctly - GH shows 5 conflicting files (and they match my own tests).

[TheBlueMatt]

It just needs rebased again, closing for now and will rebase on top of #9075 after that gets merged.

[TheBlueMatt]

Rebased on #9075.

[TheBlueMatt]

Rebased after #9260.