Buffer block downloading and commit/check blocks in parallel #1233

TheBlueMatt commented at 6:28 AM on May 9, 2012: member

This is based on cblockstore and commits blocks in a separate thread from downloading/initial checking (in CheckBlock). It offers a nice performance gain over cblockstore alone.

pfrom->Misbehaving is now called in a callback instead of after EmitBlock as nDoS is not known until the block is committed. This could give us a bit more work to do from a bad node's blocks, but should be pretty safe as the buffer is limited to 20 blocks by default (-blockbuffersize).
A setBlocksSeen is added which holds a set of blocks which have passed basic DOS checks (CheckBlock and the minwork computations), ie is the list of orphans + accepted blocks. This allows us to avoid cs_main locking when doing the duplicate detection in the initial checking
FinishEmitBlock callbacks are done in series and, therefore, CBlockStore had to be changed to allow for InOrder callback threads. Though FinishEmitBlock callbacks could be called out of order, this would result in a significant number of "orphan" blocks being committed before being merged into the main block tree, which is pretty ugly and inefficiency.
GetLastCheckpoint now returns a cached value instead of scanning mapBlockIndex each time. This is primarily to avoid a cs_main lock in the initial checking, but is also a micro-optimization.

TheBlueMatt commented at 1:36 AM on May 10, 2012: member

For the record, cblockstore's download times from local nodes comes in reliably under master, but only by a very tiny margin. This, on the other hand, comes in around 20% lower on tmpfs chain sync.

in src/init.cpp:None in c37ab81ce1 outdated

 505 | @@ -463,8 +506,8 @@ bool AppInit2(int argc, char* argv[])
 506 |      printf("Done loading\n");
 507 |  
 508 |      //// debug print
 509 | -    printf("mapBlockIndex.size() = %d\n",   mapBlockIndex.size());
 510 | -    printf("nBestHeight = %d\n",            nBestHeight);
 511 | +    //printf("mapBlockIndex.size() = %d\n",   mapBlockIndex.size());
 512 | +    printf("BestBlockHeight = %d\n",        pblockstore->GetBestBlockIndex()->nHeight);

rebroad commented at 2:22 PM on May 12, 2012:

Could comment out this line too, since the height is mentioned in debug.log just few lines before this line.

rebroad commented at 2:28 PM on May 12, 2012: contributor

can this pull request be done in such a way that there isn't so much of a diff output? Did so much code need to move from main.cpp to protocol.cpp, for example?

TheBlueMatt commented at 4:51 PM on May 12, 2012: member

Note that this pull is based on #771, which has radically different design goals. This pull is fairly small on its own. Re: the move from main -> protocol. One of the primary design goals of #771 is to remove the number of globals we export from main, especially the block index/chain storage stuff. As a part of this, the net code in main.cpp really no longer belongs there, and was moved to protocol.cpp.

laanwj commented at 7:58 AM on May 13, 2012: member

I think those are very sensible design goals.

That said, in that case we should definitely pull #771 first, to prevent unrelated changes being merged into one pull request. Github seems to almost lock up when I try to view the diff.

rebroad commented at 6:09 PM on May 14, 2012: contributor

Ah, ok. Well, I'd certainly find the diffs easier to view if they were kept small. Probably would be better therefore to base this commit from a post-#771 commit then, in order not to effectively include that commit in its entirety within this commit, then it can be reviewed without having to manually save the files and manually diff them.

TheBlueMatt commented at 6:19 PM on May 14, 2012: member

Github always shows total diffs from master, so the only reasonable way to do it is to just base on #711, pulling in its huge diff...however, you can always compare the commit list to #711 and view the diffs of individual commits.

rebroad commented at 6:21 PM on May 14, 2012: contributor

@TheBlueMatt, do you mean within github? I'm not sure how to do that. Could you provide a URL, perhaps?

TheBlueMatt commented at 6:26 PM on May 14, 2012: member

You have to manually compare the list of commits, and then you can just open each commit from the commits list in the pull...if you feel like doing some URL hacking, you will notice git style ...s in diff URLs which you can replace manually using any branch like: https://github.com/TheBlueMatt/bitcoin/compare/cblockstore...parallelcheck

TheBlueMatt closed this on May 29, 2012

TheBlueMatt reopened this on Jun 6, 2012

TheBlueMatt commented at 11:57 PM on June 6, 2012: member

Rebased onto #1429

Add a CHub for communication from p2p/wallet to blockstore.

The goal is for p2p code/wallet to only get information/communicate
information about the blockchain through CHub, giving Bitcoin a
much more clearly-defined structure and allowing for the removal
of a ton of the current global mess.

877e3af8ba

Add a basic CHubListener that can be extended. c42d0f15a2

Add EmitBlock/CommitBlock functionality to CHub.

Replacing ProcessBlock with EmitBlock, and creating callbacks for
CommitBlock.

dd3593c500

Fix typo e6d39a8873

Add HandleCommitBlock to net.cpp & move stuff from main.cpp to it 159c528cec

Remove uiInterface.NotifyBlocksChanged and replace with CommitBlock

Also rename NotifyBlocksChanged to NotifyNewBlock and remove from
the uiInterface signals list.

This removes some functionality, but NofiyBlocksChanged was not
used anyway, so it shouldn't matter.  That said, if it is ever
needed, it would be fairly trivial to add a new callback for it
in CHub.

94a440ae79

Add EmitAlert/CommitAlert functionality to CHub.

Replace ProcessAlert with calls to EmitAlert and create callbacks
for CommitAlert.

1aad2e99bb

Use CommitAlert in qt/clientmodel.cpp 5365eca78c

Add RegisterRemoveAlert functionality to CHub. 7da0bed1f2

Replace NotifyAlertChanged with RegisterRemoveAlert. 8f6c206561

Convert Orphan Tx storage to CTransactions from CDataStreams.

There was no reason to use CDataStream as the transaction was
already being serialized/deserialized several times, with this
change, transactions coming in over network are deserialized once
when received, and then only reserialized in the call to
RelayMessage, which will be called in a callback thread, not
blocking cs_main.

c6c30dca1a

Add a cs around mapAlreadyAskedFor. 9dc89066a8

Add basic EmitTransaction/CommitTransaction functionality to CHub. ac33eeca2c

Use EmitTransaction instead of AcceptToMemoryPool in sendrawtx. 3bfb60f64d

Add CWallet support for registering with a CHub. c277c93f23

Use HandleCommitTransactionToMemoryPool instead of SyncWithWallets. 916ffbfe9d

Remove AcceptToMemoryPool and replace with EmitTransaction.

 * This removes not only CTransaction::AcceptToMemoryPool, but
    also CMerkleTx::AcceptToMemoryPool. It also moves
    CWalletTx::AcceptWalletTransaction to wallet.cpp

 * This adds a fCheckInputs flag to EmitTransaction, which is
    similar to the fCheckInputs flag to AcceptToMemoryPool,
    however, it has stricter guidlines that it should only be set
    "when transaction is a supporting tx for one of our own."
    Additionally, "fCheckInputs is ignored (and set to true)
    if !IsInitialBlockDownload() && !fClient"

    As a part of these guidelines,
    CWalletTx::AcceptWalletTransaction calls EmitTransaction with
    fCheckInputs set to true (the default) on the final
    transaction, whereas it used to call with fCheckInputs set to
    false. This has the important side-effect of allowing wallet-
    generated transactions to end up getting AddOrphanTx'd.
    However, if a supporting transaction to one of our own had
    previously been AddOrphanTx'd, it would immediately be added
    to memory pool as it is "a supporting tx for one of our own"
    and thus is re-added with fCheckInputs=false.

    Note that the possibility of a wallet transaction getting
    AddOrphanTx'd is very low, and should only happen if
    a) a transaction's input is a generate and we are missing that
       block (note that no transactions should be generated with a
       generation input if we don't have that block anyway).
    b) We match the !IsInitialBlockDownload() && !fClient check,
       are not caught up to the latest block, and an input is in a
       block we do not yet have (possible after the last
       checkpoint). This situation is temporary and should resolve
       itself once we catch up (though AddOrphanTx'd transactions
       may be permanently orphaned).

    Largely, these guidelines are there because there is no reason
    to add a transaction without checking its inputs, as we have
    those inputs available, and checking them as any other
    transaction would provides additional sanity-checks.

 * A second EmitTransaction was added with tx of type CMerkleTx.
    This keeps behavior of CMerkleTx::AcceptToMemoryPool the same
    in fClient mode. Note that new behavior was invented for
    CHub::EmitTransaction(CTransaction&...) in fClient mode,
    namely that ClientConnectInputs is only checked if
    fCheckInputs is true. This was chosen to make emitting a
    transaction possible in fClient mode even if its inputs are
    not available, but could be changed if support for that is not
    needed when fClient mode is actually implemented.

e60084cae3

Add a CBlockStore class to hold the blockstore. 41ff3cf816

Add support for registering a CBlockStore with CHub. 9a268f437b

Move EmitBlock handling from CHub to CBlockStore. 60ba767b3e

Move block checking from CTxDB::LoadBlockIndex->LoadBlockIndex. 9b926c8d34

Add CWallet::HandleCommitBlock and move junk from main.cpp to it.

Note that this changes the way the GUI shows coinbases of users:
it now shows them only after the first non-orphan block based on
the generating block, instead of after the first block based on
the generating one period.

9a8cd1de9b

in src/main.cpp:None in 669a5d4c9e outdated

1861 |      {
1862 | -        printf("ProcessBlock: ORPHAN BLOCK, prev=%s\n", pblock->hashPrevBlock.ToString().substr(0,20).c_str());
1863 | -        CBlock* pblock2 = new CBlock(*pblock);
1864 | -        mapOrphanBlocks.insert(make_pair(hash, pblock2));
1865 | -        mapOrphanBlocksByPrev.insert(make_pair(pblock2->hashPrevBlock, pblock2));
1866 | +        printf("CBlockStore::FinishEmitBlock: ORPHAN BLOCK, prev=%s\n", block.hashPrevBlock.ToString().substr(0,20).c_str());

rebroad commented at 11:28 PM on June 14, 2012:

Why make this line longer than it already was?

sipa commented at 11:31 PM on June 14, 2012:

Because the name of the function it is in, is being renamed.

in src/main.cpp:None in 669a5d4c9e outdated

1912 |          }
1913 |          mapOrphanBlocksByPrev.erase(hashPrev);
1914 |      }
1915 |  
1916 | -    printf("ProcessBlock: ACCEPTED\n");
1917 | +    printf("CBlockStore::FinishEmitBlock: ACCEPTED\n");

rebroad commented at 11:29 PM on June 14, 2012:

Not sure why this line is being changed....

TheBlueMatt commented at 12:01 AM on June 15, 2012:

Because the function name has changed.

rebroad commented at 7:29 PM on July 2, 2012:

ProcessBlock is more visually appealing IMHO.

TheBlueMatt commented at 7:31 PM on July 2, 2012:

But if it were ProcessBlock, it would be unclear and searching to code for where the printf was called would be harder (which is the point of prefixing the print with the function that called it)

in src/main.cpp:None in 669a5d4c9e outdated

2695 | @@ -2551,6 +2696,13 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
2696 |                      printf("force request: %s\n", inv.ToString().c_str());
2697 |              }
2698 |  
2699 | +            if (inv.type == MSG_BLOCK)
2700 | +                nBlockCount++;
2701 | +
2702 | +            // Don't set hashLastInvLastBlock if we are getting a hashContinue inv
2703 | +            if (nInv == nLastBlock && nBlockCount > 5)

rebroad commented at 11:31 PM on June 14, 2012:

why 5?

TheBlueMatt commented at 12:05 AM on June 15, 2012:

Its to guess if we have another getblocks to go after this one. Its MAX_BLOCK_SIZE = 1000000 / default -maxsendbuffer = 10000000 = 10 / 2 = 5 See https://github.com/TheBlueMatt/bitcoin/blob/669a5d4c9e49fb3027ec7110664faa42615698bc/src/main.cpp#L2798

rebroad commented at 7:43 PM on July 2, 2012:

I'm not sure what I'm looking at re the URL above. Why 5 rather than 4 or 6?

TheBlueMatt commented at 7:44 PM on July 2, 2012:

MAX_BLOCK_SIZE / (default -maxsendbuffer / 2) The link simply explains why its maxsendbuffer/2.

Add ability to post_all() in CSemaphore. 03aaa47487

Move AlreadyHave to CHub::NeedInv c54072bf68

Add a setBlocksSeen and use it for duplicate checks. b8eecbe69d

Add support for calling CNode->Misbehaving from CHub & CBlockStore. bf46b9fb0b

Add support for emitting DoS callbacks from EmitBlock. 296f60d53f

Lock cs_main in ProcessMessage instead of ProcessMessages.

This removes the locking of cs_main before calling Emit* and should
allow multiple ProcessMessage calls at once.

It no doubt locks cs_main in places where it isnt neccessary, but
to avoid complicated and unforseen interactions, cs_main is locked
for nearly every message in ProcessMessage.

8d006b6c78

Add a CBlockStore::GetBlockIndex to encapsulate mapBlockIndex.

It has a fBlocking flag to wait for the block being requested to
be committed after having been emitted (for use in the
"force request" block request in the "inv" message handler).

This fixes a potential segfault if -blockbuffersize is overly big.

235a0277cf

Add support for EmitBlock concurrency. 2514150e66

Use EmitBlock concurrency in ProcessMessage. c3782838fc

Cache last checkpoint. 7522cc8dc1

Move getdata handling out of cs_main in SendMessages.

This resolves a bug where the block buffer is allowed to deplete
because the getdata that is required to continue is not sent.

0a6ab15771

Add -blockbuffersize, default 20. fb20713497

Call PushGetBlocks after last block, instead of just on orphans.

This keeps the block buffer from running out until the final
block's FinishEmitBlock is run.

9c33625524

Use EmitBlock concurrency in LoadExternalBlockFile. 7176054d23

Fix improper use of STL Containers. 1e2aff6a68

in src/main.cpp:None in 669a5d4c9e outdated

2876 | +
2877 | +        // Though we request duplicates, Satoshi nodes will not return any,
2878 | +        // thanks to setInventoryKnown, however they will still count them
2879 | +        // towards the block size in the inv result, its still better to
2880 | +        // request blocks now, but #973 will optimize this further.
2881 | +        if (pfrom->hashLastInvLastBlock == block.GetHash())

rebroad commented at 11:33 PM on June 14, 2012:

won't inv.hash work here?

TheBlueMatt commented at 12:06 AM on June 15, 2012:

Yep, changed.

TheBlueMatt commented at 8:15 PM on July 5, 2012: member

This needs rebasing, and Im not going to keep rebasing this stuff without any interest in eventually merging. If it ever gets interest, I may reopen.

TheBlueMatt closed this on Jul 5, 2012

lateminer referenced this in commit dc27b03ad0 on Jan 22, 2019

lateminer referenced this in commit 2e4d9142fb on Jan 10, 2020

dexX7 referenced this in commit 1c0ae8ae01 on Aug 10, 2021

DrahtBot locked this on Feb 15, 2022