Bitcoin source code is a giant mess

[misterbigg]

Presented without comment:



link (github.com)

[1715286370]

1715286370

[1715286370]

1715286370

[1715286370]

1715286370

[CIYAM]

I think this would very much depend upon the specific context - although I haven't coded a "goto" myself since the 1980's I did work on some C code with huge *case* statements in the 90's where it can actually make some sense (in general it is not a good idea with C++ due to the problems of exception handling).

Typically the only reason to use such a construct would be to "skip" to the *end* of a large block of code (and of course you could always use *break* inside a loop to do the same thing).

In no way is a *goto* more "efficient for the compiler" though (nor is it very good for human eyes as no-one would *expect* to even *see* a goto these days) so in regards to your quote I'd have to agree it doesn't inspire a lot of confidence.

[mmeijeri]

Gavin has said that the genius doesn't lie in the code base, but in the algorithms and that the code is crap. I agree with that assessment. There's a somewhat refactored version available that can also be used for alt coins, but I don't have a link handy right now. I've been thinking about refactoring the code myself. Everybody is afraid of breaking it since it appears to work, and that is reasonable to an extent, but on the other hand this code base is utterly unsuitable for serious work.

[grue]

Can we have some context on how it was used?

Also:

While overall usage of gotos has been declining, there are still situations in some languages where a goto provides the shortest and most straightforward way to express program's logic (while it's possible to express the same logic without gotos, the equivalent code will be longer and often more difficult to understand).

These situations include: multi-level breaks, resource allocation/deallocation, error handling in C language, computed goto in Perl language.[15][16]

[misterbigg]

Can we have some context on how it was used?

Yep I updated the original post to include a link.

...These situations include: multi-level breaks, resource allocation/deallocation, error handling in C language, computed goto in Perl language.

In this case its error handling in the C language (although Bitcoin is written in C++).

Everybody is afraid of breaking it since it appears to work, and that is reasonable to an extent, but on the other hand this code base is utterly unsuitable for serious work.

Yep. A year ago I came in here and I wanted to work on Bitcoin. Clean it up a bit, reduce external dependencies, etc... but I was pooh-poohed. Now I'm doing the same work in Ripple, and it is welcomed. For Bitcoin code to improve first the people working on it need to acknowledge that there's a growing problem and then commit to addressing it. Even if it is just a little bit at a time, or for example making sure that new code is clean (adding RAII utilities where needed, for example), it can help to reverse the spaghettification.

[CIYAM]

Yup - just checked the source link - error handling.

Well in my code you would have seen something like this:

Code:

   if (!pkey || !group || !pub_key || !priv_key) 
      throw runtime_error( "no keys or group found" );

BTW - I hate the term RAII (although it stuck) - I argued years ago on comp.lang.c++.moderated to call the idiom "scoped objects".

[misterbigg]

Well in my code you would have seen something like this:

Code:

   if (!pkey || !group || !pub_key || !priv_key) 
      throw runtime_error( "no keys or group found" );

Yeah I agree, I don't see how it is useful to return an error in this case. You want the program to halt. I seriously doubt that Bitcoin is written robustly throughout to handle out of memory conditions.

[CIYAM]

I seriously doubt that Bitcoin is written robustly throughout to handle out of memory conditions.

Actually I doubt there is much C++ code out there at all (apart from perhaps OS level code) that can really handle out of memory conditions well (for a start you can't throw a std::runtime_error as that needs a std::string to be allocated).

[misterbigg]

...I doubt there is much code out there at all that can really handle out of memory conditions well (for a start you can't throw a std::runtime_error as that needs a std::string to be allocated).

It's doable but it needs to be a design criteria from the beginning.

[empoweoqwj]

You're a OpenCoin employee / currently hired by OpenCoin?

[CIYAM]

It's doable but it needs to be a design criteria from the beginning.

Indeed - I did it with an Object Database I created (the error string buffers are pre-allocated before the ODB instance is even created) - it is a little tricky though as you end up using "non-standard" exceptions (although you can make it enough like a std::exception to be useful).

[misterbigg]

You're a OpenCoin employee / currently hired by OpenCoin?

No

[oleganza]

"goto err" is not a big deal in my view.

Bitcoin-QT is a mess for entirely different reasons.

1. Endianness is not well documented. libbitcoin flips some bytes where BitcoinQT does not, but also does not document why they are doing that.

2. Excessive use of C++ subclasses and operators makes it hard to read code. You see a << b, but it actually goes to a very specific place which is not entirely obvious where.

3. Some utility class names look like variable names.

4. Some funny design patterns are not documented even with a single line of text. E.g. CBigNum is smartly done (BN_CTX and BIGNUM are wrapped in classes differently), but you have to be really proficient in C++ or spend half a day deciphering the code to understand why it is done that way. For someone coming from C/Objective-C it's not easy.

I'm working on Mac wallet app and reimplement many things that BitcoinQT does in C and ObjC and learn how that whole thing works. It is smart, but it's messy.

[CIYAM]

2. Excessive use of C++ subclasses and operators makes it hard to read code. You see a << b, but it actually goes to a very specific place which is not entirely obvious where.

Streaming operators << and >> have always been the standard way of doing I/O in C++ so are you saying these operators are being used for something *other than streaming*?

[piotr_n]

Coding in C and never using goto, is like driving a car and never using its last gear. Safe for poor drivers...

[CIYAM]

Coding in C and never using goto, is like driving a car and never using its last gear. Safe for poor drivers...

Bitcoin is written in C++ (which is not even close to C as any C++ programmer knows).

[piotr_n]

Coding in C and never using goto, is like driving a car and never using its last gear. Safe for poor drivers...

Bitcoin is written in C++ (which is not even close to C as any C++ programmer knows).

Really?  Well, then I was wrong all my life, believing that C++ was backward compatible with C...

[Lohoris]

The fact that goto has been used badly doesn't make it a bad instrument: it's a great instrument if you know exactly when to use or not use it.

Opposing it just because someone isn't trained or smart enough to use it properly is a dumbing down which does much much much more harm than good.

[CIYAM]

Really?  Well, then I was wrong all my life, believing that C++ was backward compatible with C...

You are wrong - C++ is *not* backwards compatible with C only as *close as possible*.

[piotr_n]

Really?  Well, then I was wrong all my life, believing that C++ was backward compatible with C...

You are wrong - C++ is *not* backwards compatible with C only as *close as possible*.

Close enough to have a working goto...