CBlockStore

TheBlueMatt commented at 10:59 pm on January 19, 2012: member

From the commitmsg: “Change Bitcoin’s flow to use a CBlockStore class which net/wallets send/recieve blocks to/from.

This commit is designed to not change any bitcoin functionality or operation.

Largest changes:

ProcessMessage(s)/SendMessages moved to protocol.cpp
Many globals removed from main.h and abstracted to CBlockStore
Calls to CWallet::AddToWalletIfInvolvingMe no longer block Block verification, resulting in a small, but measureable block verification speed increase.

There is still a lot of abstraction to go, but this is a step in the right direction.”

This commit is not intended to be merged for 0.6, its targeted for for 0.7. I just wanted to post it to start getting eyeballs on it as early as possible. In addition, there is a nasty performance bug somewhere that is causing an almost 25% slowdown in block download when downloading a lot of blocks (when downloading from a local tmpfs node to another local tmpfs node, but it is often quicker for smaller sets, say < 5000 blocks). Ive looked at it in valgrind and gprofile and tried changing some possible culprits and havent had any luck tracking it down. If anyone tracks it down, Ill owe you big time. Additionally, please mind the removal of cs_main around stuff thats not in main.cpp (esp rpc) and tell me Im wrong to do that.

TheBlueMatt commented at 0:10 am on January 20, 2012: member

Oh and as usual thanks to @sipa and @gmaxwell for comments/suggestions/general help for my C++ noobishness along the way.

luke-jr commented at 4:01 pm on February 17, 2012: member

This seems to need rebasing… also, at least fedb422 and bc98084 don’t belong here (and create regressions).

TheBlueMatt commented at 7:51 am on February 18, 2012: member

Rebased, should be pretty mergeable now (aside from the elusive performance issue).

TheBlueMatt commented at 8:35 pm on February 20, 2012: member

Fixed a few minor issues and rebased. Still nothing new on the performance issue(s).

TheBlueMatt commented at 3:52 am on February 23, 2012: member

So, good news on the performance front. While I was benchmarking bitcoin built in debug vs release (for gitian stuff), I noticed there was a bit of a performance decrease when built in debug mode. As it turns out, I had always been benchmarking CBlockStore in debug mode. When ff66202 is removed and CBlockStore is benchmarked, it has the slight performance improvement over master that would be expected.

TheBlueMatt commented at 7:36 pm on February 29, 2012: member

Rebased.

TheBlueMatt commented at 4:08 am on March 14, 2012: member

Ridiculously minor fixes to make valgrind happy and rebased.

TheBlueMatt closed this on Mar 26, 2012

TheBlueMatt reopened this on Apr 1, 2012

TheBlueMatt commented at 1:50 am on April 1, 2012: member

Rebased, added a simple unit test, and abstracted one more thing.

TheBlueMatt commented at 1:53 am on April 1, 2012: member

Not sure why github is showing a few of gavin’s commits that are on master in the commitlist, but they have the same commitid, and this appears to be rebased fine in git…

TheBlueMatt commented at 2:29 am on April 6, 2012: member

Closing this since after some thought I really dont trust myself to touch this much code without introducing one or two fatal bugs. If anyone does want to look at this, @sipa spent some time cleaning up the internals to look nicer/run a bit smoother at https://github.com/sipa/bitcoin/tree/ooifiedbs

TheBlueMatt closed this on Apr 6, 2012

sipa commented at 12:13 pm on April 6, 2012: member

@TheBlueMatt large refactorings always carry some risk, but imho this is a refactor that needs to happen anyway. It can probably use a few more eyes, but it seems to run without problem, so it seems a waste not trying to get it merged.

By the way, ooifiedbs is rebased against master.

TheBlueMatt reopened this on May 5, 2012

TheBlueMatt commented at 8:06 am on May 5, 2012: member

Slowly working on rebasing this.

TheBlueMatt commented at 7:23 am on May 6, 2012: member

Rebased against master.

sipa commented at 2:23 pm on May 6, 2012: member

I get one potential deadlock: in net.cpp, AskForBlocks sends messages while holding the cs_vNodes lock. It’s better to make a copy of the node you need to contact, ->AddRef() it, send the message, and ->Release().

TheBlueMatt commented at 6:55 am on May 7, 2012: member

Fixed.

in src/bitcoinrpc.cpp: in 8be11f904a outdated

1414@@ -1408,7 +1415,7 @@ Value listsinceblock(const Array& params, bool fHelp)
1415             "listsinceblock [blockhash] [target-confirmations]\n"
1416             "Get all transactions in blocks since block [blockhash], or all transactions if omitted");
1417 
1418-    CBlockIndex *pindex = NULL;
1419+    const CBlockIndex *pindex = NULL;

Diapolo commented at 9:21 am on May 7, 2012:

If this is a constant now, how can you write to it in line 1426?

sipa commented at 10:27 am on May 7, 2012:

“const CBlockIndex *pindex” means “declare *pindex as a const CBlockIndex”, or more commonly “declare pindex as a pointer to a const CBlockIndex”. This means that the pointer can change, but it can only point to constant objects. Line 1426 makes it point to a constant object.

Diapolo commented at 11:23 am on May 7, 2012:

Thanks for the clarification.

TheBlueMatt commented at 9:25 am on May 7, 2012: member

Spent some more time double checking and triple checking the latest rebase…found one more potential deadlock, fixed one potential race…more checking to be done @Diapolo Ill check in the morning, its almost 6am…

TheBlueMatt commented at 0:17 am on May 8, 2012: member

Fixed the one other potential deadlock I’m aware of…its kinda an ugly fix, but it works, and since the transaction copying only happens in the cblockstore callback thread(s), it shouldn’t effect performance either way. Gonna do more testing in the next few days, but this should be pretty good right now.

TheBlueMatt commented at 10:22 pm on May 8, 2012: member

Rebased onto current master.

TheBlueMatt commented at 7:44 am on May 9, 2012: member

Rebased and have been testing…looking largely good to me, so any additional eyeballs at this stage would be appreciated.

Add comment for constant pubkey. f994ceaa5f

Change Bitcoin's flow to use a CBlockStore class

which net/wallets send/recieve blocks to/from.

This commit is designed to not change any bitcoin functionality or operation.

Largest changes:
* ProcessMessage(s)/SendMessages moved to protocol.cpp
* Many globals removed from main.h and abstracted to CBlockStore
* Calls to CWallet::AddToWalletIfInvolvingMe no longer block Block verification,
  resulting in a small, but measureable block verification speed increase.

There is still a lot of abstraction to go, but this is a step in the right direction.

000f04150d

A start on SPV mode for a CBlockStore-based bitcoin.

You can enable "SPV Mode" by changing HasFullBlocks() to return
false in src/blockstore.h.

In "SPV Mode", blocks are still written to disk, but if any code
attempts to read blocks, bitcoin will assert fail.

This is not a complete implementation of non-block-reading
"SPV Mode", it is just the fixing of areas of code which read
blocks when I tested it.

b421ce6fcb

Potentially unsafe optimization. a422455c48

Clean up getmininginfo's API. c482016a01

Add basic unit test support and abstract a bit more. b0233aa94f

CBlockStoreCallback and derived classes 1af7ffcc36

Use condition waiting instead of polling for callbacks

Modified to use CSemaphore instead of CConditionVariable by Matt
- blame Matt if this breaks something.

e99b0bc435

Fix coding style after 865a0c1674dd13563498608dfff27fa6da7b8357 de08772730

Allow setting -callbackconcurrency, default 1 thread.

Purposefully not documented in -? as it is only useful
for debugging.

4ac3a19b4b

Use callbacks for RelayMessage in wallet.cpp to avoid a deadlock. 3bb9fee812

Remove AcceptToMemoryPool entirely. 201617ef32

Fix DEBUG_LOCKORDER. 83148eadbb

Add debug stuff - REMOVE BEFORE MERGE cb16f39e6f

TheBlueMatt commented at 2:51 pm on May 29, 2012: member

Closing to redo from the ground up (again). Continuing rebasing this would simply cause issues and no doubt create odd interactions with recent commits.

TheBlueMatt closed this on May 29, 2012

destenson referenced this in commit 641f464d44 on Jun 26, 2016

ptschip referenced this in commit 57f7c57873 on Dec 27, 2017

DrahtBot locked this on Sep 8, 2021

CBlockStore #771