Fix block-connection performance regression

TheBlueMatt commented at 3:32 pm on October 25, 2016: member

@JeremyRubin found a performance regression introduced by b3b3c2a5623d5c942d2b3565cc2d833c65105555 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.

in src/main.cpp: in d0f3b60785 outdated

2798@@ -2799,20 +2799,40 @@ static int64_t nTimeFlush = 0;
2799 static int64_t nTimeChainState = 0;
2800 static int64_t nTimePostConnect = 0;
2801 
2802+struct CompareBlockIndexByWork {
2803+    bool operator()(const CBlockIndex* a, const CBlockIndex* b) const {
2804+        return a->nChainWork < b->nChainWork;

sipa commented at 3:35 pm on October 25, 2016:

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

TheBlueMatt commented at 4:16 pm on October 25, 2016:

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.

in src/main.cpp: in d0f3b60785 outdated

2830-    if (!pblock) {
2831-        if (!ReadBlockFromDisk(block, pindexNew, chainparams.GetConsensus()))
2832+    const CBlock *pblock;
2833+    auto it = blockTrace.mapBlocks.find(pindexNew);
2834+    if (it == blockTrace.mapBlocks.end()) {
2835+        CBlock *pblockNew = new CBlock();

JeremyRubin commented at 4:15 pm on October 25, 2016:

nit: maybe do this as

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

Or

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

and update following code accordingly

TheBlueMatt commented at 6:29 pm on October 25, 2016:

The new version should be cleaner.

in src/main.cpp: in d0f3b60785 outdated

6049@@ -6023,23 +6050,24 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
6050 
6051     else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex) // Ignore blocks received while importing
6052     {
6053-        CBlock block;
6054-        vRecv >> block;
6055+        CBlock *pblock = new CBlock();

JeremyRubin commented at 4:50 pm on October 25, 2016:

See other comment about using make_shared

in src/rpc/mining.cpp: in d0f3b60785 outdated

131@@ -132,7 +132,8 @@ UniValue generateBlocks(boost::shared_ptr<CReserveScript> coinbaseScript, int nG
132             continue;
133         }
134         CValidationState state;
135-        if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL))
136+        std::shared_ptr<const CBlock> shared_pblock(new CBlock(*pblock));

JeremyRubin commented at 4:51 pm on October 25, 2016:

make_shared

in src/rpc/mining.cpp: in d0f3b60785 outdated

726@@ -726,7 +727,9 @@ UniValue submitblock(const JSONRPCRequest& request)
727             + HelpExampleRpc("submitblock", "\"mydata\"")
728         );
729 
730-    CBlock block;
731+    CBlock* blockptr = new CBlock();

JeremyRubin commented at 4:51 pm on October 25, 2016:

make_shared

in src/test/miner_tests.cpp: in d0f3b60785 outdated

222@@ -223,7 +223,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
223         pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
224         pblock->nNonce = blockinfo[i].nonce;
225         CValidationState state;
226-        BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL));
227+        std::shared_ptr<const CBlock> shared_pblock(new CBlock(*pblock));

JeremyRubin commented at 4:52 pm on October 25, 2016:

make_shared

in src/test/test_bitcoin.cpp: in d0f3b60785 outdated

126@@ -127,7 +127,8 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>&
127     while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
128 
129     CValidationState state;
130-    ProcessNewBlock(state, chainparams, NULL, &block, true, NULL);
131+    std::shared_ptr<const CBlock> shared_pblock(new CBlock(block));

JeremyRubin commented at 4:52 pm on October 25, 2016:

make_shared

in src/main.cpp: in d0f3b60785 outdated

3765@@ -3740,7 +3766,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha
3766     return true;
3767 }
3768 
3769-bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp)
3770+bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, std::shared_ptr<const CBlock>& pblock, bool fForceProcessing, const CDiskBlockPos* dbp)

JeremyRubin commented at 4:58 pm on October 25, 2016:

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 commented at 1:32 am on October 26, 2016:

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.

in src/main.cpp: in d0f3b60785 outdated

5854@@ -5829,7 +5855,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5855         BlockTransactions resp;
5856         vRecv >> resp;
5857 
5858-        CBlock block;
5859+        CBlock *pblock = new CBlock();

JeremyRubin commented at 4:58 pm on October 25, 2016:

make_shared

TheBlueMatt commented at 5:13 pm on October 25, 2016:

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 commented at 5:32 pm on October 25, 2016:

implicit copy-cast should work…

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

TheBlueMatt commented at 6:06 pm on October 25, 2016:

Cool, didnt realize that was a thing!

in src/main.cpp: in d0f3b60785 outdated

2808+// Used in ActivateBestChain/ConnectBlock to keep track of blocks connected
2809+// and notify listeners in the right order.
2810+// Note that since mapBlocks is sorted lowest-work to highest-work, iterating
2811+// over it will give you the order in which the blocks were connected
2812+struct BlockConnectTrace {
2813+    std::map<CBlockIndex*, std::shared_ptr<const CBlock>, CompareBlockIndexByWork> mapBlocks;

JeremyRubin commented at 5:06 pm on October 25, 2016:

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

TheBlueMatt commented at 6:29 pm on October 25, 2016:

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.

JeremyRubin commented at 5:10 pm on October 25, 2016: contributor

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.

TheBlueMatt force-pushed on Oct 25, 2016

TheBlueMatt commented at 6:30 pm on October 25, 2016: member

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.

TheBlueMatt force-pushed on Oct 25, 2016

JeremyRubin commented at 7:26 pm on October 25, 2016: contributor

re-utack, much cleaner!

TheBlueMatt force-pushed on Oct 25, 2016

in src/main.cpp: in 34d0c884d6 outdated

2798@@ -2799,11 +2799,18 @@ static int64_t nTimeFlush = 0;
2799 static int64_t nTimeChainState = 0;
2800 static int64_t nTimePostConnect = 0;
2801 
2802+// Used to track conflicted transactions removed from mempool and transactions

sipa commented at 1:06 am on October 26, 2016:

Can you use a doxygen compatible comment here?

in src/main.cpp: in a0d30f80f3 outdated

2974@@ -2974,6 +2975,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c
2975                     state = CValidationState();
2976                     fInvalidFound = true;
2977                     fContinue = false;
2978+                    // If we didn't actually connect the block, don't notify listeners about it

sipa commented at 1:17 am on October 26, 2016:

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 commented at 3:22 am on October 26, 2016:

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 commented at 4:44 am on November 23, 2016:

Ok (answered elsewhere).

in src/main.cpp: in 051b9968cf outdated

2805@@ -2806,11 +2806,13 @@ struct ConnectTrace {
2806     std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
2807 };
2808 
2809+

sipa commented at 1:22 am on October 26, 2016:

Unintentional new lines?

in src/main.cpp: in 5095970c97 outdated

5840@@ -5846,7 +5841,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5841         BlockTransactions resp;
5842         vRecv >> resp;
5843 
5844-        CBlock block;
5845+        CBlock *pblock = new CBlock();
5846+        std::shared_ptr<const CBlock> shared_pblock(pblock); // pblock will be delted by the shared_ptr

sipa commented at 3:07 am on October 26, 2016:

Typo: deleted

TheBlueMatt force-pushed on Oct 26, 2016

in src/main.cpp: in 3f78bc176a outdated

2975                     if (!state.CorruptionPossible())
2976                         InvalidChainFound(vpindexToConnect.back());
2977                     state = CValidationState();
2978                     fInvalidFound = true;
2979                     fContinue = false;
2980+                    // If we didn't actually connect the block, don't notify listeners about it

sipa commented at 4:27 am on October 26, 2016:

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 commented at 2:24 pm on October 26, 2016:

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 commented at 5:21 pm on October 26, 2016:

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.

laanwj added the label Validation on Oct 26, 2016

sipa commented at 3:40 pm on October 26, 2016: member

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 (review) - I don’t think there is an issue for this PR.

in src/main.cpp: in 3f78bc176a outdated

2821-    CBlock block;
2822     if (!pblock) {
2823-        if (!ReadBlockFromDisk(block, pindexNew, chainparams.GetConsensus()))
2824+        std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
2825+        connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew);
2826+        if (!ReadBlockFromDisk(*pblockNew, pindexNew, chainparams.GetConsensus()))

sipa commented at 5:29 pm on October 26, 2016:

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 commented at 6:20 pm on October 26, 2016:

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 commented at 6:28 pm on October 26, 2016:

I see!

JeremyRubin commented at 6:49 pm on October 26, 2016:

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

sipa commented at 5:57 pm on October 26, 2016: member

utACK 3f78bc176a3c612f320bce4516aadcd6234c0850. My nit is just a suggestion.

sipa commented at 10:26 pm on November 7, 2016: member

Needs rebase.

TheBlueMatt force-pushed on Nov 7, 2016

TheBlueMatt commented at 10:48 pm on November 7, 2016: member

Rebased.

fanquake commented at 0:29 am on November 8, 2016: member

Travis failures:

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

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

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

paveljanik commented at 7:14 am on November 8, 2016: contributor

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

TheBlueMatt commented at 1:19 am on November 9, 2016: member

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

TheBlueMatt closed this on Nov 9, 2016

TheBlueMatt reopened this on Nov 17, 2016

TheBlueMatt force-pushed on Nov 17, 2016

TheBlueMatt commented at 11:28 pm on November 17, 2016: member

Rebased on #9075.

TheBlueMatt force-pushed on Nov 22, 2016

TheBlueMatt commented at 2:51 am on November 22, 2016: member

Rebased after #9125 was merged. Note that #9125 should have fixed most of the performance regression here (and was likely the source of its major reorg speedup, though it should help in more real-world conditions as well), though this should still help some, and is a good step towards #9027 as well as improving callback concurrency in the future.

TheBlueMatt force-pushed on Nov 23, 2016

in src/main.cpp: in 7b717cdf7f outdated

3126         // ... and about transactions that got confirmed:
3127-        for (unsigned int i = 0; i < txChanged.size(); i++)
3128-            GetMainSignals().SyncTransaction(*std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i]));
3129+        for (const auto& pair : connectTrace.blocksConnected) {
3130+            assert(pair.second);
3131+            const CBlock& block = *(pair.second);

sipa commented at 5:01 am on November 23, 2016:

This loop could be rewritten as

0for (const auto& ptx : pair.second->vtx)
1    GetMainSignals().SyncTransaction(*ptx, pair.first, i);

sipa commented at 5:02 am on November 23, 2016: member

utACK

gmaxwell commented at 8:59 pm on November 24, 2016: contributor

utACK

in src/main.cpp: in eb33aa1412 outdated

3018@@ -3018,6 +3019,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c
3019                     state = CValidationState();
3020                     fInvalidFound = true;
3021                     fContinue = false;
3022+                    // If we didn't actually connect the block, don't notify listeners about it
3023+                    connectTrace.blocksConnected.pop_back();

sdaftuar commented at 2:13 am on November 29, 2016:

nit: slightly weird that the caller of ConnectTip() has to know to do this on failures. Perhaps document this somewhere?

TheBlueMatt commented at 5:26 am on November 30, 2016:

Fixed

sdaftuar commented at 2:21 am on November 29, 2016: member

Code review ACK, will test.

sdaftuar commented at 10:33 pm on November 29, 2016: member

ACK 7b717cd

Add struct to track block-connect-time-generated info for callbacks 6fdd43b968

Keep blocks as shared_ptrs, instead of copying txn in ConnectTip fd9d89070a

Create a shared_ptr for the block we're connecting in ActivateBCS ae4db44d03

Make the optional pblock in ActivateBestChain a shared_ptr 2736c44c8e

Switch pblock in ProcessNewBlock to a shared_ptr

This (finally) fixes a performance regression in
b3b3c2a5623d5c942d2b3565cc2d833c65105555

2d6e5619af

Document ConnectBlock connectTrace postconditions dd0df81ebd

TheBlueMatt force-pushed on Dec 4, 2016

TheBlueMatt commented at 8:21 am on December 4, 2016: member

Rebased after #9260.

reinier1991 approved

laanwj merged this on Dec 5, 2016

laanwj closed this on Dec 5, 2016

laanwj referenced this in commit d04aebaec7 on Dec 5, 2016

codablock referenced this in commit c4d15c113a on Jan 16, 2018

codablock referenced this in commit fdbb9b6501 on Jan 16, 2018

codablock referenced this in commit b5389c09b3 on Jan 17, 2018

gladcow referenced this in commit 59ec8eafa9 on Mar 5, 2018

gladcow referenced this in commit ad83a65b02 on Mar 8, 2018

gladcow referenced this in commit 60f18b7806 on Mar 13, 2018

gladcow referenced this in commit aea4ba0e85 on Mar 14, 2018

gladcow referenced this in commit 9fe17f4666 on Mar 15, 2018

gladcow referenced this in commit a14a312856 on Mar 15, 2018

gladcow referenced this in commit 37aa7dc04f on Mar 15, 2018

gladcow referenced this in commit 1f5b5c562a on Mar 15, 2018

gladcow referenced this in commit f3a4879534 on Mar 24, 2018

gladcow referenced this in commit ca694e861a on Apr 4, 2018

UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018

andvgal referenced this in commit c8b7bc1a78 on Jan 6, 2019

andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019

CryptoCentric referenced this in commit b13b4f0860 on Feb 25, 2019

CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019

furszy referenced this in commit 8a9b92c0ed on Oct 17, 2020

DrahtBot locked this on Sep 8, 2021

Fix block-connection performance regression #9014