Clean up code dependencies #2154

CodeShark · 2013-01-07T07:18:51Z

I realize this introduction is a bit long, so if you don't feel like reading skip to the bullet points below under "strategy".

The codebase as it exists right now has a number of unnecessary dependencies which makes code modularization much more difficult. In particular, the satoshi client was built to handle all bitcoin-related tasks, but its value as a "reference implementation" lies primarily in doing verification and acting as a relay agent as these are the most essential tasks that participating nodes must perform to keeping the network in operation. Things like wallets, historical databases, mining, and notification agents could be written as entirely separate third-party applications without risk to the network's fundamental integrity.

The basic architecture of a bitcoin node is as follows:

At the core there exist fundamental bitcoin message structures, along with the code necessary for serialization/deserialization. These structures belong in their own source files with minimal dependencies so they can be reused for applications that needn't perform verification and relay - for instance, filtering and notification agents. Unfortunately, these core structures currently reside for the most part in main.h/main.cpp, one of the central problems this pull request attempts to fix.

On top of these core structures sits a network component that manages sockets, does peer discovery, and handles queueing and dispatching of messages. This component is clearly dependent on the core message structures but does not depend on the specific logic used to verify blocks and transactions nor to identify misbehaving peers nor sign transactions nor maintain a block chain database.

Then we have a scripting engine, signature verification component, and a signing component. Historical database applications do not need signature verification/signing functionality at all. Filtering messages and sending alerts generally does not even require a scripting engine and does fine with basic pattern matching.

The most critical high-level operations needed by a verification/relay node such as the satoshi client are transaction verification; block chain and memory pool management; and detection/management of misbehaving peers. These things are currently primarily implemented in main.h/main.cpp. These are indeed the main operations of the satoshi client - but the core low-level structures should not depend at all on this logic.

Then there's the UI, but let's leave that aside for a moment.

Finally there's init.h/init.cpp, which sets up the particular environment in which the satoshi client runs.

This branch takes the following strategy:

Remove source file dependencies on main.h and init.h by only including necessary headers wherever possible.
If source files depend on definitions in main or init, either move the dependent portions into main/init or move the depended-upon portions into separate files.
If the dependent source files use global variables or functions that clearly belong in either main or init, copy the value over to a class member or a variable with file scope in the dependent source or expose a registration function to set a callback.
If moving a core class out of main is impossible because its methods depend on variables or functions defined in main, isolate the methods that depend on main and either move them to another class that does belong in main or convert them into regular functions in main.

It is important that all modifications made in this branch are easy to review and to test. This branch does not encourage rewriting things from scratch - only moving them and rearranging them in easily identifiable chunks. Furthermore, the focus of the branch is not so much on coding style and style consistency - but on isolation of functional units and elimination of unnecessary dependencies.

gavinandresen · 2013-01-07T14:53:59Z

Why namespace db_cpp ? Seems to me the database copy of pchMessageStart could be a static member of the DbEnv.

CodeShark · 2013-01-07T15:19:41Z

I'd rather avoid adding more dependencies in CAddrDB for something that isn't really used elsewhere.

sipa · 2013-01-07T15:34:54Z

@gavinandresen That doesn't make sense. This is for peers.dat, which doesn't use BDB at all, and I suppose CDbEnv will be gone as soon as we kick out BDB-based wallets.

CodeShark · 2013-01-07T16:38:33Z

I'm still not clear on exactly where in the code it's best to:

set the magic bytes for CAddrDB
Set the call handlers for net.cpp

I tried running and got this error: http://pastebin.com/tJU9Gsi2
But tried restarting and it initialized correctly, and now it seems to be running fine.

BitcoinPullTester · 2013-01-07T17:23:28Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/03014e2ff26d4ab6195f8b0bfc29c712dec509de for binaries and test log.

gavinandresen · 2013-01-07T22:07:49Z

RE: namespace db_cpp:

Fine, make it static in CAddrDB, I forgot that peers.dat is not a BDB file (and pass it into CAddrDB constructors? or is CAddrDB a singleton class? )

CodeShark · 2013-01-08T04:05:34Z

@gavinandresen Noted and done.

CodeShark · 2013-01-08T05:21:36Z

cp: writing out/bitcoind.exe': No space left on device cp: writingout/test_bitcoin.exe': No space left on device

WTF?!?!?!

Shall we all chip in and get BlueMatt some more hard disk space?

CodeShark · 2013-01-08T12:42:22Z

I am going to keep pushing commits as an extra backup despite BitcoinPullTester being out of disk space.

Diapolo · 2013-01-08T13:49:21Z

I think you are doing good work, but I'm sure you will get faster ACKs or merges, if you try to keep pulls smaller. Perhaps @sipa or other core devs can comment.

gavinandresen · 2013-01-08T14:02:56Z

Our bottleneck is code review and testing.

If you want your pulls to get merged, then you need to help fix that bottleneck-- either recruit reviewers/testers to review/test your own code, or volunteer to help review/test other people's pulls.

Or only refactor code that has good unit tests, so we can be more confident that nothing breaks on any of the three platforms we support.

I am generally against refactoring just to improve the code, unless the refactoring comes with some significant benefit or unit tests.

gavinandresen · 2013-01-08T14:06:20Z

Oh, also: you might want to help test or update https://github.com/libcoin/libcoin which is a fully refactored version of the core code, that, last I checked, nobody used because nobody trusts it (because so many changes were made without thorough testing).

CodeShark · 2013-01-08T16:30:06Z

The changes I'm making will be well-documented and possible to rigorously review. And if more unit tests are needed, I'd be glad to help write some up.

I'm making incremental changes, hopefully each change easy to understand and track. I haven't changed any actual logic in the code nor implementation details - just moved code around - and in large chunks, not small pieces.

As most of the significant changes thus far are things like moving methods from one class to another or turning them into regular functions, it should be possible to do a fairly comprehensive assessment by doing pattern matches on the codebase and making sure each usage was appropriately modified. i.e. tx.CheckTransaction() becomes CheckTransaction(tx); or tx.GetOutputFor(txin, inputs) becomes inputs.GetOutputFor(txin); etc...

CodeShark · 2013-01-08T16:46:57Z

Also, I'm willing to do comprehensive testing on earlier commits with the hope of getting them merged without having to merge everything at once.

sipa · 2013-01-08T19:27:30Z

src/net.cpp

+//
+ProcessMessagesHandler fnProcessMessages = NULL;
+SendMessagesHandler fnSendMessages = NULL;
+StartShutdownHandler fnStartShutdown = NULL;


Can these be static?

BitcoinPullTester · 2013-01-08T20:48:43Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4af9a6c7e0d1246852195b47c200a974dff47ab1 for binaries and test log.

sipa · 2013-01-08T21:05:49Z

Just for the record: I've been discussing these changes extensively with @CodeShark the past few days, and I think they are very valuable. They should make the code easier to understand and reuse.

Getting 0.8 out now certainly has priority over refactorings, but as these are almost entirely just code-movement changes, merging them shouldn't be too hard.

sipa · 2013-01-08T21:15:16Z

src/main.cpp

-    for (unsigned int i = 0; i < vin.size(); i++)
-        nResult += GetOutputFor(vin[i], inputs).nValue;
+    for (unsigned int i = 0; i < tx.vin.size(); i++)
+        nResult += this->GetOutputFor(tx.vin[i]).nValue;


Is this "this->" necessary?

nah, it's mostly a stylistic thing, I guess. I can get rid of it if you don't like it.

BitcoinPullTester · 2013-01-09T00:03:29Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/aea31289a59166195d2d7270eacb0905b14d476a for binaries and test log.

BitcoinPullTester · 2013-01-09T06:33:36Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/366fb8aa64a581c22064dae33a3d08769375b9f3 for binaries and test log.

CodeShark · 2013-01-23T17:11:26Z

The core commits will be reorganized into four stages:

1. move core class methods that should remain class methods implemented in main.cpp out of main.cpp and into core.cpp. (move)
1. move core class method implementations in main.h that should not be part of core classes out of the class declarations in main.h. (move)
1. turn methods that should not be part of the core classes into regular functions in main.h and main.cpp, get rid of the method prototypes in the classes, and add function declarations in main.h where necessary. (pattern replacement, add function declarations to main.h)
1. move the core class declarations to core.h (move)

BitcoinPullTester · 2013-01-24T04:24:12Z

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/366fb8aa64a581c22064dae33a3d08769375b9f3 for test log.

This pull does not merge cleanly onto current master

luke-jr · 2013-01-30T18:37:05Z

Rebase needed

Diapolo · 2013-01-30T18:39:10Z

I'm sure @CodeShark intents to rework this pull into smaller more logical pieces after 0.8 is final.

sipa · 2013-06-04T17:31:35Z

src/wallet.h


 class CAccountingEntry;
 class CWalletTx;
 class CReserveKey;
 class COutput;
+class CWalletDB;


Is this needed?

jgarzik · 2013-06-04T17:40:19Z

OK, very much like where this code is going. ACK 90% of it, modulo inline comments made earlier.

One specific criticism with this rebase: the rebase problems were all fixed in a final, appended commit. That does not work, because it breaks bisection properties. Each commit needs to produce a tree that is buildable and passes tests. Otherwise, "git bisect" will not work, and attempting to find a problematic commit in past history becomes more difficult.

I know that makes rebasing more difficult for changes like this, sorry :(

We cannot have: broken commit, broken commit, broken commit, commit that fixes everything.

sipa · 2013-06-04T17:44:29Z

I'm very much in favor of the code changes here (and they look move-only to, apart from the registration functions). It's only a first step, but it's a very needed one IMHO.

I agree with the comment about the last commit fixing everything, but apart from that, I'd like to see this merged soon. Since now seems the ideal time for this, I don't mind holding up other pullreqs for a short time, so this can get reviewed and finalized.

jgarzik · 2013-06-04T17:47:07Z

Agreed, RE holding other pullreqs, to avoid the endless rebase pain on @CodeShark 's part.

sipa · 2013-06-06T04:49:38Z

@CodeShark You'll still at least need to add core.h to bitcoin-qt.pro, and while you're on it, I don't see any harm in adding core.cpp to the other makefiles too - that'll make it easier to move stuff there in the future.

…ctures from main to core beginning with COutPoint.

…it a regular function in main.

…ngs to the mempool instance. Removed AreInputsStandard from CTransaction, made it a regular function in main. Moved CTransaction::GetOutputFor to CCoinsViewCache. Moved GetLegacySigOpCount and GetP2SHSigOpCount out of CTransaction into regular functions in main. Moved GetValueIn and HaveInputs from CTransaction into CCoinsViewCache. Moved AllowFree, ClientCheckInputs, CheckInputs, UpdateCoins, and CheckTransaction out of CTransaction and into main. Moved IsStandard and IsFinal out of CTransaction and put them in main as IsStandardTx and IsFinalTx. Moved GetValueOut out of CTransaction into main. Moved CTxIn, CTxOut, and CTransaction into core. Added minimum fee parameter to CTxOut::IsDust() temporarily until CTransaction is moved to core.h so that CTxOut needn't know about CTransaction.

CodeShark · 2013-06-06T06:17:41Z

@sipa done
@theuni core.h/core.cpp will have to be considered in what you're doing

CodeShark · 2013-06-06T07:25:20Z

Sorry, hit the close button by accident.

CodeShark · 2013-06-06T07:42:14Z

Alright - code freeze until merge.

sipa · 2013-06-07T09:11:56Z

src/db.cpp

@@ -486,6 +488,7 @@ void CDBEnv::Flush(bool fShutdown)
 // CAddrDB
 //

+unsigned char CAddrDB::pchMessageStart[4] = { 0x00, 0x00, 0x00, 0x00 };


This duplication is quite ugly. Mike's refactor that moves these chain-dependent constants to a separate class is nicer, but you may want to check that it doesn't introduce new dependencies.

sipa · 2013-06-07T09:38:08Z

ACK. Code changes look good to me, and I've tested syncing/mining/receiving/sending on testnet. I have a few inline comments left, but those can be dealt with in follow-up commits, so we don't need to stall the merging process too long.

laanwj · 2013-06-09T07:53:13Z

src/core.cpp

+// Distributed under the MIT/X11 software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "core.h"


Why have this implementation file that only includes a header?

More things are intended to be moved to core (in particular, CBlock), in a second step. As those will include implementation code too, already create a file for them, so e.g. #2748 picks it up already.

laanwj · 2013-06-09T19:09:05Z

I've tested this commit on testnet, no problems found.

Clean up code dependencies

* Make the togglePrivateSend button not react to the spacebar or enter. bitcoin#1766 * redo comment * change `privateSendAuto` focus policy to remain consistent

* MAZA-POS: (5575 commits) Mazafication of code More mazafication More mazafications and compile correction fixes fix for build issues fix for build issues fix string in net.cpp correct pow.cpp correct validation.cpp fixing for build errors fix typo Merge remote-tracking branch 'origin/MAZA-POS' into MAZA-POS merge to dash rebase Release notes 0.12.3.3 Remove redundant parameter fCheckDuplicateInputs from CheckTransaction Fix crash bug with duplicate inputs within a transaction Bump to 0.12.3.3 Release notes 0.12.3.2 (bitcoin#2174) Add tests for special rules for slow blocks on devnet/testnet (bitcoin#2176) Allow mining min diff for very slow (2h+) blocks (bitcoin#2175) Fix issues with selections on Masternode tab (bitcoin#2170) Sync mn list and mnw list from 3 peers max (bitcoin#2169) A few devnet related fixes (bitcoin#2168) Adjust diff for slow testnet/devnet blocks a bit smoother (bitcoin#2161) Make PS Buttons not react to spacebar (bitcoin#2154) Bump to 0.12.3.2 (bitcoin#2173) Bump to 0.12.3.1 (bitcoin#2158) Update release notes (bitcoin#2155) Use correct protocol when serializing messages in reply to `getdata` (bitcoin#2157) Fix p2pkh tests asserts (bitcoin#2153) Fix block value/payee validation in lite mode (bitcoin#2148) ...

sipa reviewed Jan 8, 2013
View reviewed changes

jgarzik mentioned this pull request Mar 31, 2013

Move GenerateBitcoins() call out of net.cpp's StartNode() #2431

Merged

sipa reviewed Jun 4, 2013
View reviewed changes

CodeShark added 7 commits June 5, 2013 23:14

Created core.h/core.cpp, added to makefiles. Started moving core stru…

effc277

…ctures from main to core beginning with COutPoint.

Moved CInPoint to core. Removed GetMinFee from CTransaction and made …

788536f

…it a regular function in main.

Moved CCoins, CTxOutCompressor, CTxInUndo, and CTxUndo to core.

65e7bbe

Removed script.cpp's dependence on main.h

48343a0

Moved UpdateTime out of CBlockHeader and moved CBlockHeader into core.

aabdf9e

Using boost::signals2 to message main from net.cpp.

501da25

Removed the main.h include from net.cpp.

6e68524

CodeShark closed this Jun 6, 2013

CodeShark reopened this Jun 6, 2013

sipa reviewed Jun 7, 2013
View reviewed changes

This was referenced Jun 7, 2013

Refactor chain-specific tweaks into a CChainParams class and introduce a regtest mode #2632

Merged

Yet another Coin Control Release #2343

Closed

laanwj reviewed Jun 9, 2013
View reviewed changes

jgarzik pushed a commit that referenced this pull request Jun 10, 2013

Merge pull request #2154 from CodeShark/dependencycleanup

f59530c

Clean up code dependencies

jgarzik merged commit f59530c into bitcoin:master Jun 10, 2013

CodeShark mentioned this pull request Jun 11, 2013

Finish moving core data structures into core.h. #2758

Merged

petertodd mentioned this pull request Jun 11, 2013

Log reason for non-standard transaction rejection #2743

Merged

sipa mentioned this pull request Jun 14, 2013

Cleanup code by using forward declarations and other methods... #2767

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Clean up code dependencies #2154

Clean up code dependencies #2154

Uh oh!

Conversation

CodeShark commented Jan 7, 2013

Uh oh!

gavinandresen commented Jan 7, 2013

Uh oh!

CodeShark commented Jan 7, 2013

Uh oh!

sipa commented Jan 7, 2013

Uh oh!

CodeShark commented Jan 7, 2013

Uh oh!

BitcoinPullTester commented Jan 7, 2013

Uh oh!

gavinandresen commented Jan 7, 2013

Uh oh!

CodeShark commented Jan 8, 2013

Uh oh!

CodeShark commented Jan 8, 2013

Uh oh!

CodeShark commented Jan 8, 2013

Uh oh!

Diapolo commented Jan 8, 2013

Uh oh!

gavinandresen commented Jan 8, 2013

Uh oh!

gavinandresen commented Jan 8, 2013

Uh oh!

CodeShark commented Jan 8, 2013

Uh oh!

CodeShark commented Jan 8, 2013

Uh oh!

sipa Jan 8, 2013

Choose a reason for hiding this comment

Uh oh!

BitcoinPullTester commented Jan 8, 2013

Uh oh!

sipa commented Jan 8, 2013

Uh oh!

sipa Jan 8, 2013

Choose a reason for hiding this comment

Uh oh!

CodeShark Jan 8, 2013

Choose a reason for hiding this comment

Uh oh!

BitcoinPullTester commented Jan 9, 2013

Uh oh!

BitcoinPullTester commented Jan 9, 2013

Uh oh!

CodeShark commented Jan 23, 2013

Uh oh!

BitcoinPullTester commented Jan 24, 2013

Uh oh!

luke-jr commented Jan 30, 2013

Uh oh!

Diapolo commented Jan 30, 2013

Uh oh!

sipa Jun 4, 2013

Choose a reason for hiding this comment

Uh oh!

jgarzik commented Jun 4, 2013

Uh oh!

sipa commented Jun 4, 2013

Uh oh!

jgarzik commented Jun 4, 2013

Uh oh!

sipa commented Jun 6, 2013

Uh oh!

CodeShark commented Jun 6, 2013

Uh oh!

CodeShark commented Jun 6, 2013

Uh oh!

CodeShark commented Jun 6, 2013

Uh oh!

sipa Jun 7, 2013

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 7, 2013