Bitcoin codebase cleaned, should adopt?

wumpus

hero member

Activity: 812

Merit: 1022

No Maps for These Territories

Quote from: hsoft on December 13, 2010, 04:23:46 PM

EDIT: Thinking about it, I'm having doubt about my statement (that inlined function don't have to be in headers)... I'd have to verify that. After all, I'm only learning C++... But it still doesn't change the fact that optimization should be targeted at measured bottlenecks.

Inlined functions must be in header files, at least for almost all current compilers, as they compile per compilation unit (.c/.o).

Compilers with whole-program-optimization (such as clang/llvm) could inline functions over compilation unit boundaries. Inlining and optimization would happen in the linker.

You cannot assume that though...

hsoft

newbie

Activity: 14

Merit: 0

Since I had only compiled my cleaned bitcoin code with Mac OS X thus far, I thought it would be a good idea to fix the cleaned codebase for Windows compilation. Darn is it complicated to create a bitcoin VC++ build setup! Is anyone building bitcoin using VC++ or is everyone using MinGW? I can compile the "official" version of bitcoin, but whenever I run it, I get tons of runtime assertion errors. Anyone ever got that?

Moreover, it's much more complicated than I thought to make my cleaned code compile under Windows. I thought it was only a matter of correctly "ifdef-ing" a couple of includes I had moved around, but it's much more complicated than that. Where gcc compiles fine, MSVC just spit errors about ambiguous and re-defined symbols and stuff like that. I'm trying to remove the "using namespace X" statements to go around this, but then I get even weirder errors. I don't feel like fighting with MSVC anymore, so I'm giving up.

The positive side is that I got to learn a little bit of C++.

hsoft

newbie

Activity: 14

Merit: 0

Quote from: Hal on December 13, 2010, 03:21:14 PM

One concern I'd have with losing the inlinable .h functions is whether it could slow down either initial block load/verify/indexing, or mining (a bit moot with gpu miners these days).

inlining doesn't have to be in .h files. I removed them because I thought it was silly to put them everywhere without first identifying bottlenecks. At first, I wondered if maybe it was the result of thoughtful optimization, but this inlining was really put at silly places, so I doubt it was the case. They can easily be brought back, but I think that anyone bringing them back should read about it first (for example: http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.3 ). Misused inlining makes the code slower.

EDIT: Thinking about it, I'm having doubt about my statement (that inlined function don't have to be in headers)... I'd have to verify that. After all, I'm only learning C++... But it still doesn't change the fact that optimization should be targeted at measured bottlenecks.

Hal

vip

Activity: 314

Merit: 4276

As a relative newbie to the code, I do find your version easier to read. It's easier to find the implementations of functions since most are class members and are now in named .cpp files. It's also nice to have the full class implementation in one place rather than having to flip back and forth between .h and .cpp files.

However that is for reading, and the needs of developers may be somewhat different. Obviously Satoshi knows the current layout intimately, and as the main developer it is up to him to see whether he could work with the new layout.

One concern I'd have with losing the inlinable .h functions is whether it could slow down either initial block load/verify/indexing, or mining (a bit moot with gpu miners these days).

davout

legendary

Activity: 1372

Merit: 1008

1davout

Quote from: jgarzik on December 13, 2010, 12:49:48 PM

Of course it's possible. There's nothing inherent about C++ that precludes unit tests or functional tests.

Haha yea I know, I was just using that formulation to avoid sounding like "hey that's cool but what are you guys waiting for to make a test suite? Huh

"

It could be deterministic if using a set block chain, well anyway, useful and elegant testing is pretty hard but sooo useful

jgarzik

legendary

Activity: 1596

Merit: 1100

Quote from: davout on December 13, 2010, 12:42:23 PM

As an addition, do you know if it's possible to write a test suite for C++ code?

Of course it's possible. There's nothing inherent about C++ that precludes unit tests or functional tests.

A bitcoin test suite would be useful, but somewhat non-deterministic due to the way a proof-of-work would have to be used.

jgarzik

legendary

Activity: 1596

Merit: 1100

Most agree that the current codebase suffers from "all in one file" syndrome.

However, I think it is overkill to break up every class into its own header and implementation files.

davout

legendary

Activity: 1372

Merit: 1008

1davout

Didn't read the actual code either but it's really cool you took on your time to do these refactorings.

As an addition, do you know if it's possible to write a test suite for C++ code?

davux

sr. member

Activity: 288

Merit: 263

Firstbits.com/1davux

I didn't look at the actual code (It would be worthless since I'm a crap at C++ anyway), but I think it's important to do the kind of cleaning you did. I hope it will make it into the code.

It's important for security and trust, as it allows easier peer reviewing and bug finding, which is all the more necessary that Bitcoin touches a domain where trust matters a lot.

It's important for extracting a library, so that other programs can appear, and still use the same backend code as the GUI. They would only have to code their specific parts (e.g. the readline part, if it's an interactive shell-like client), which would make their development easier and more secure. Just like JSON-RPC, but lower-level and more flexible: class loading and direct use of Bitcoin functions, instead of HTTP communication.

It's important for the improvement of the lib itself:

If it's better organized, more people can understand it and contribute to it.
Modular code is easier to manipulate and modify, add new features to it, etc.

And finally, the coupling to wxWidgets is the main reason why bitcoin hasn't made it into Debian yet (it's a nightmare). So sorting things out would greatly ease the package maintainer's work, and bitcoin's wider adoption by a valuable geek population.

I can also think of half a dozen topics on this very forum where infinite discussion could have been replaced by actual code.

hsoft

newbie

Activity: 14

Merit: 0

To be able to create a library out of the BitCoin code (as I explain in https://bitcointalksearch.org/topic/libbitcoin-a-good-idea-2075 ) and eventually create bindings in other languages for it (Python!), I cleaned Bitcoin's source code. I'm pretty much finished and the result is at:

http://bitbucket.org/hsoft/bitcoin-clean

So the question I'm asking you guys is: Should this be included in the main repository? Personally, I find the code much more readable now, but then I guess opinions can diverge, so that why I'm asking. The list of things I've done is described there:

http://bitbucket.org/hsoft/bitcoin-clean/src/f7995af49149/cleaning_process.txt

It's a huge change, but I doubt I introduced new bugs because I haven't changed a single line of actual code. I have just restructured it. If I had introduced bugs, my guess is that it would have been bugs causing the whole app not to work, but it builds and run fine (I get connections and I download blocks).

The next step towards my ultimate goal (libBitCoin, python bindings, alternate UIs) involves decoupling classes, which is more bug prone (because I have to actually change the code), but I added a tag in the repo (before-decouple) to indicate at which point I started modifying the code itself.

So, there I go: Do you find the cleaned code more readable and maintainable, or do you prefer the old one? Should it replace the official one on SVN?

Topic: Bitcoin codebase cleaned, should adopt? (Read 2144 times)