CBlockStore #771
CBlockStore #771
Conversation
|
This seems to need rebasing... also, at least fedb422 and bc98084 don't belong here (and create regressions). |
|
Rebased, should be pretty mergeable now (aside from the elusive performance issue). |
|
Fixed a few minor issues and rebased. Still nothing new on the performance issue(s). |
|
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. |
|
Rebased. |
|
Ridiculously minor fixes to make valgrind happy and rebased. |
|
Rebased, added a simple unit test, and abstracted one more thing. |
|
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... |
|
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 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. |
|
Slowly working on rebasing this. |
|
Rebased against master. |
|
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(). |
|
Fixed. |
| @@ -1408,7 +1415,7 @@ Value listsinceblock(const Array& params, bool fHelp) | |||
| "listsinceblock [blockhash] [target-confirmations]\n" | |||
| "Get all transactions in blocks since block [blockhash], or all transactions if omitted"); | |||
|
|
|||
| CBlockIndex *pindex = NULL; | |||
| const CBlockIndex *pindex = NULL; | |||
There was a problem hiding this comment.
If this is a constant now, how can you write to it in line 1426?
There was a problem hiding this comment.
"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.
|
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 |
|
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. |
|
Rebased onto current master. |
|
Rebased and have been testing...looking largely good to me, so any additional eyeballs at this stage would be appreciated. |
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.
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.
Modified to use CSemaphore instead of CConditionVariable by Matt - blame Matt if this breaks something.
Purposefully not documented in -? as it is only useful for debugging.
|
Closing to redo from the ground up (again). Continuing rebasing this would simply cause issues and no doubt create odd interactions with recent commits. |
Some houskeeping
Fix a typo in BUcash 1.1.1.1 rel notes
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:
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.