Bitcoin Forum
December 06, 2016, 08:12:31 AM *
News: Latest stable version of Bitcoin Core: 0.13.1  [Torrent].
 
   Home   Help Search Donate Login Register  
Pages: [1]
  Print  
Author Topic: Bitcoin codebase cleaned, should adopt?  (Read 1794 times)
hsoft
Newbie
*
Offline Offline

Activity: 14


View Profile
December 13, 2010, 02:51:17 PM
 #1

To be able to create a library out of the BitCoin code (as I explain in http://bitcointalk.org/index.php?topic=2075.0 ) 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?
1481011951
Hero Member
*
Offline Offline

Posts: 1481011951

View Profile Personal Message (Offline)

Ignore
1481011951
Reply with quote  #2

1481011951
Report to moderator
Advertised sites are not endorsed by the Bitcoin Forum. They may be unsafe, untrustworthy, or illegal in your jurisdiction. Advertise here.
1481011951
Hero Member
*
Offline Offline

Posts: 1481011951

View Profile Personal Message (Offline)

Ignore
1481011951
Reply with quote  #2

1481011951
Report to moderator
davux
Sr. Member
****
Offline Offline

Activity: 289


Firstbits.com/1davux


View Profile WWW
December 13, 2010, 05:35:26 PM
 #2

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.

1DavuxH9tLqU4c7zvG387aTG4mA7BcRpp2
México (Oaxaca) – France - Leeds
davout
Legendary
*
Offline Offline

Activity: 1358


1davout


View Profile WWW
December 13, 2010, 05:42:23 PM
 #3

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?

jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
December 13, 2010, 05:48:07 PM
 #4

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.

Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
jgarzik
Legendary
*
qt
Offline Offline

Activity: 1470


View Profile
December 13, 2010, 05:49:48 PM
 #5

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.

Jeff Garzik, bitcoin core dev team and BitPay engineer; opinions are my own, not my employer.
Donations / tip jar: 1BrufViLKnSWtuWGkryPsKsxonV2NQ7Tcj
davout
Legendary
*
Offline Offline

Activity: 1358


1davout


View Profile WWW
December 13, 2010, 05:59:08 PM
 #6

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 Smiley

Hal
VIP
Sr. Member
*
expert
Offline Offline

Activity: 314



View Profile
December 13, 2010, 08:21:14 PM
 #7

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).

Hal Finney
hsoft
Newbie
*
Offline Offline

Activity: 14


View Profile
December 13, 2010, 09:23:46 PM
 #8

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.
hsoft
Newbie
*
Offline Offline

Activity: 14


View Profile
December 15, 2010, 04:28:53 PM
 #9

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++.
wumpus
Hero Member
*****
qt
Offline Offline

Activity: 798

No Maps for These Territories


View Profile
December 15, 2010, 08:22:11 PM
 #10

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...

Bitcoin Core developer [PGP] Warning: For most, coin loss is a larger risk than coin theft. A disk can die any time. Regularly back up your wallet through FileBackup Wallet to an external storage or the (encrypted!) cloud. Use a separate offline wallet for storing larger amounts.
Pages: [1]
  Print  
 
Jump to:  

Sponsored by , a Bitcoin-accepting VPN.
Powered by MySQL Powered by PHP Powered by SMF 1.1.19 | SMF © 2006-2009, Simple Machines Valid XHTML 1.0! Valid CSS!