126
127

A proposal to improve Bitcoin Unlimited's code quality (self.btc)

homerjthompson_ が 投稿

The bitcoin code that has been inherited from Core is a mess. When I saw that the latest assert was failing at line 5706, the first thing I thought was "Oh my God a file should never be 6000 lines long. That's a sign of undisciplined programmers generating an unreadable mess."

The ProcessMessage function alone is more than 1,300 lines long and there are plenty of other multi-hundred line functions throughout the code. Code in this state is very unpleasant to read and almost impossible to spot bugs in.

What's needed is a proper code review process but the code in its current state is not even suitable for code review because you can't look at a part of the code and understand everything that you need to understand in order to say whether there's a problem.

The only people who have a decent chance at spotting a problem are those who are intimately familiar with the code already - namely the authors. The code deters newcomers because of its complexity.

Also, the code in its current state is untestable. You simply cannot test a 1,000 line function. There is too much going on to be able to set up a situation, call the function, check the result and say that everything is ok.

The current methods of testing bitcoin (using python scripts to run and control the executable) are not professional and are completely inadequate. It's like testing a car by driving it around after it has been fully built. Yes, you must do that, but you should also test each component of the car to ensure that it meets specifications before you install that component.

For bitcoin, that means writing tests in C++ for functions written in C++. Test the functions, not just the executable.

It also means breaking the multi-hundred line functions which do very many things into lots of little functions that each do only one or two things. Those little functions can be tested, and more importantly, they can be read. If a function doesn't fit on the screen, then you can't see it. You can only see part of it, and try to remember the rest.

If you can behold the entire function at once, and understand every detail of it, then it is much harder for a bug to escape your notice.

Some people are very intelligent and can hold a lot of code in their head even if it doesn't fit on the screen. They can deal with a hundred-line function and perceive any flaw in it. They can write multiple hundred line functions and nothing ever goes wrong.

Those are the "wizards". They write incredibly complex messy code which is impossible for a normal person to review. They cannot be fired because nobody else understands their code. As time goes on, the code becomes more and more complex, eventually exceeding the wizard's capabilities. Then the entire project is an unmaintainable buggy mess.

We do not want that. We want code which normal programmers find easy to read. We do not want to intimidate people with complex code. We want code which is continually refactored into smaller, simpler, easy to test functions.

We don't want to become a small exclusive club of wizards.

So my proposal is: Stop trying to manage Bitcoin Unlimited as Core's code plus changes.

Core is writing wizard code. Enormous files containing enormous functions which are too complex for a newcomer to fully comprehend. Very intimidating, signs of intelligent minds at work, but poor software engineering. They're on a path of ever-increasing complexity.

I propose that BU break from that. Break up the functions until every one fits on a screen and is ideally only a few lines long. Break up the files until every file is easy to fully comprehend. Make the code reader-friendly. Make the variable names intelligible. The book Clean Code by Robert Martin does a good job of providing the motivation and skills for a task like this. Core's code is not clean. It smells and needs to be cleaned.

Only that way will it be possible for non-wizards to review the code.

The cost of doing this is that new changes to Core's code can't be easily copied. If Core does something new and BU wants to implement it, it will have to be re-implemented. Although that's more work, I think it's better in the long run.

Some people are expressing doubts about the BU code quality now. A way to answer those doubts is with a clearly expressed and thorough code-quality improvement project.

I'll volunteer to review code that is clean and simple and easy to read, with intelligible variable names and short functions that preferably do one thing each, and I hope others will join me.

That will require moving away from the Core codebase over time and making space for unexceptional but diligent coders who aren't and shouldn't need to be wizards.

全 82 件のコメント
並べ替え:
ベスト
トップ新着論争中古い順Q&A

[–]coin-master 13 ポイント14 ポイント  (2子コメント)

Didn't Classic devs already started that process. Maybe there could be some synergy between those two clients?

[–]Annapurna317 3 ポイント4 ポイント  (1子コメント)

I think they're trying to make the codebase less procedural.

[–]don_savage 0 ポイント1 ポイント  (0子コメント)

I think that would be somewhat synonymous with breaking it up, right?

[–]Annapurna317 36 ポイント37 ポイント  (14子コメント)

This is the exact reason that the community should reject the 33,000 new lines of code that Segwit introduces.

[–]OneOrangeTank 5 ポイント6 ポイント  (5子コメント)

Most of that code is tests...

[–]akuukka 1 ポイント2 ポイント  (1子コメント)

This should be the most upvoted reply.

[–]Adrian-X 0 ポイント1 ポイント  (0子コメント)

the economic impact is untested, that's why it's controversial, not because it works, but because it will work.

[–]paleh0rse 3 ポイント4 ポイント  (1子コメント)

I think we both know that BU isn't really down with that whole "testing" thing. It's obviously completely overrated.

[–]Adrian-X 0 ポイント1 ポイント  (0子コメント)

rushed, sure, it couldn't wait any longer.

[–]Adrian-X 0 ポイント1 ポイント  (0子コメント)

if it does what they say it does its not a good rule chains if you like the bitcoin we've had for the last 8 years.

[–]jeanduluoz 10 ポイント11 ポイント  (2子コメント)

This is a reason. But it's wayyyy down the line in terms of importance - there are far more pressing reasons why the current segwit implementation is a hot mess.

[–]mcr55 1 ポイント2 ポイント  (1子コメント)

Like?

[–]flamingboard 7 ポイント8 ポイント  (0子コメント)

Fuzzy math, bribe discount, the fact that it's a malleability cludge and not a block increase, changing how transactions work in a soft fork is fucking negligent, Core can't be trusted with the truth, etc...

[–]boomtnt46 8 ポイント9 ポイント  (0子コメント)

omfg 33000 lines.

[–]nullc -1 ポイント0 ポイント  (3子コメント)

yea, screw 30,000 new lines of tests! down with tests! down with tests!

(consensus part of segwit is about 2k lines of diff).

[–]flamingboard 5 ポイント6 ポイント  (0子コメント)

Seek professional mental help. Maybe Luke-jr and you can go halvesies on a therapist.

[–]Adrian-X 0 ポイント1 ポイント  (0子コメント)

screw the economic impact assessment, screw the bitcoin incentive design, the code works.

That's the problem I also think the code works, it's the lack of independent economic impact assessment and what functions the code enables that's controversial.

[–]toduModerator 8 ポイント9 ポイント  (4子コメント)

I propose that BU break from that. Break up the functions until every one fits on a screen and is ideally only a few lines long. Break up the files until every file is easy to fully comprehend. Make the code reader-friendly.

I like the idea of having functions that fit into one page of text and one page per file. But isn't having most functions and files be "only a few lines" taking this idea unreasonably far?

[–]homerjthompson_[S] 17 ポイント18 ポイント  (1子コメント)

Having the files be only a few lines long is unrealistic.

It can be done with functions, though. It's possible but not comfortable to write code where the functions are all five lines or fewer. It's realistic to limit functions to 10 lines or less.

What you typically do is write a function that's 15 or twenty lines and then, after it's been tested and you know it works, try to break it up into two smaller functions.

This happens kind of naturally in test-driven development where you write the test first (which fails), then write the code which makes the test pass, and then refactor the code to make all the functions short. It's called the red-green-refactor cycle. Sometimes people say the refactoring is blue and it's a red-green-blue cycle.

[–]toduModerator 3 ポイント4 ポイント  (0子コメント)

Thanks for explaining. What you wrote sounds reasonable.

[–]observerc 7 ポイント8 ポイント  (0子コメント)

No. It's just basic software engineering. The 'core' code base is the typical example of an horrible corporate beast that all it does effectively is sinking resources.

Any properly written piece of software that has survived the test of time is written in a much more intelligible and organised way. Not with 6000 thousand lines files or hundreds of lines functions. Any good software engineer ( the large majority are mediocre ) knows this.

[–]FormerlyEarlyAdopter 2 ポイント3 ポイント  (0子コメント)

You like the idea!? If one of my programmers would keep writing such shitty code after I verbally fucked him for that once, he would be fired pretty quickly and with prejudice, well before he has created too much of "job security for himself" and too much of technical debt for my company.

You must understand that some "genius programmers" actually create negative contribution to the codebase just by coding, I do not even refer to personal toxicity. You can see a perfect example of a bunch of those in rotten core.

If Bitcoin was a company, those fucks would be fired long ago. If Bitcoin was a country, those fucks would be imprisoned for life. If Bitcoin was an army, those fucks would be executed.

[–]dexX7 9 ポイント10 ポイント  (0子コメント)

As far as I can see, there has been an enormous amount by Core devs to improve code quality, to break things down, and to do proper refactoring:

https://github.com/bitcoin/bitcoin/pulls?q=label%3ARefactoring

[–]GranAutismo 7 ポイント8 ポイント  (0子コメント)

TIL the future of currency is spaghetti 😂 I really hope the code gets refactored soon. I'd help but I'm not a C++ guy.

[–]adamxk 4 ポイント5 ポイント  (0子コメント)

Completely agree, Core has needed refactored and cleaned up for years, but they just keep adding to the kludge. It never really left a beta state, and is not good enough for enterprise level software or as a reference client. It looks like fast prototype code, because a lot of it is.

Now that SegWit made Core's situation far worse with "wizard" code as you say, this is where BU could really set itself apart.

Fixing this attracts the devs we need to review and build this code, while Core devs are only more and more bogged down and exclusive as to who even knows how to deal with their own mess. That is definitely not good for the future of things.

[–]solex1Bitcoin Unlimited 4 ポイント5 ポイント  (0子コメント)

Great write-up. The bigger picture means that the time required to make code quality improvements of this scale is unavailable. I paid $2.03 for a simple txn on Monday, average conf time is climbing ridiculously. We pretty much have to go with what we have. Get bigger blocks, then look rigorously at compartmentalization etc when the network effect is no longer being strangled.

[–]toduModerator 8 ポイント9 ポイント  (6子コメント)

Ping /u/thezerg1. What do you think about breaking very large source code files into much smaller files? And maybe abandoning the monolithic source code files inherited from Bitcoin Core because it would be more secure (easier to read) and perhaps even faster to make an implementation from scratch?

[–]highintensitycanada 12 ポイント13 ポイント  (0子コメント)

Compartmentalizing? That's a best practices term, something that real enterprise software would have.

I don't think anyone can stop it if slpeople start doing it

[–]adamxk 11 ポイント12 ポイント  (2子コメント)

Core has needed refactored for years as an ugly heap of beta code, even Satoshi didn't think much of his own programming. "it works" is not good enough for enterprise level software backing a $20 Billion project

We really do need a solid reference client now that Core ruined theirs with SegWit.

[–]toduModerator 1 ポイント2 ポイント  (1子コメント)

I think we should get away from the idea of having "a reference client" though. It's better to have more than just one client that everyone is using.

[–]Richy_T 0 ポイント1 ポイント  (0子コメント)

While generally true, I think that a properly minimal reference client which could be built on by other implementations would be a good thing. I've said many times, "Core" is anything but.

[–]kaardilugeja 0 ポイント1 ポイント  (0子コメント)

I imagine it requires enormous amount of resources to write the code ground up.

I mean you could probably use parts of the old code and just brake it up to smaller readable functions etc. That still takes time. I think ideally it could/should be done simultaneously - maintain two batches of code on repo. The one with messy code that we are using right now and keep adding fixes and whatever is necessary to keep user experience as smooth as possible. At the same time start working on refactoring the code to bring it up to bar with current functionality and eventually ditch the old messy code. Continue with clean refactored code.

Edit: Anyway. I hope this gets done some day. This messy code can't be maintained forever.

[–]NukeMeNow -2 ポイント-1 ポイント  (0子コメント)

Oh yeah, it'd be easy

[–]sydwell 6 ポイント7 ポイント  (3子コメント)

Thank you for highlighting these facts. Anyone with any decent software experience can agree that both Core and therefor BU code is a mess.

Obviously, the core team and their minions have used and will continue to use their inside knowledge to attack BU nodes and software. BU better be ready.

Another solution may be to implement BU with the JavaScript library of BCOIN. See http://bcoin.io

[–]purestvfx 8 ポイント9 ポイント  (1子コメント)

I don't think the javascript route is a good idea.

[–]bitcartel 0 ポイント1 ポイント  (0子コメント)

TypeScript would be a good choice for targeting the JavaScript ecosystem.

[–]shibe5 0 ポイント1 ポイント  (0子コメント)

Another solution may be to implement BU with the JavaScript library of BCOIN.

I support alternative implementations. But I'm against giving the same name to unrelated projects.

[–]coinsinspace 4 ポイント5 ポイント  (15子コメント)

A ground-up rewrite may be a good idea, or starting from some sane reimplementation as a starting point.

Aside from code quality, core code is 'c with classes' which is a sad waste of c++ capabilities and inherits most primitive c problems. Modern c++ allows very expressive, readable, fast & safe code at the same time.

Some people are very intelligent and can hold a lot of code in their head even if it doesn't fit on the screen. They can deal with a hundred-line function and perceive any flaw in it. They can write multiple hundred line functions and nothing ever goes wrong. Those are the "wizards".

I don't agree with that definition... long messy function and messy architecture is the direct opposite of a 'wizard'. Everyone can understand a self-written thousand line function. At least as long as it's remembered.

[–]observerc 1 ポイント2 ポイント  (0子コメント)

Yeah. Less experience developers will worship guys that yield a 20 lines of code per minute.

The quoted example is typical. It's not about the person writing it being able to memorize it. That is pretty much a useless skill and ultimately, resourcing to that equates to pure stupidity.

They are not wizzards, they just lacked the skill and intelligence to make it the simple and obvious way. Any dumbfuck can take something simple and turn it into a complicated one. It's the opposite that requires skill. Taking something complicated and break it down to a simple and straightforwad structure.

Although I do not agree with you on the virtues of modern C++. Personaly I would just write it in C. And I strongly believe many of the unnecessary clutter comes from C++, such as putting the code in classes. But that is another discussion

[–]nullc 2 ポイント3 ポイント  (1子コメント)

, core code is 'c with classes' which is a sad waste

lol. Thanks for showing you either have no freeking idea what you're talking about or have never actually looked at the code.

[–]coinsinspace 6 ポイント7 ポイント  (0子コメント)

Yeah so I just looked at a random file: 

 void CChain::SetTip(CBlockIndex *pindex) {  
   if (pindex == NULL)

Have you heard about smart pointers? Writing shit like this today would be a firing offense. There are other numerous design issues, perfectly summarized with: c with classes.

[–]shibe5 0 ポイント1 ポイント  (11子コメント)

What C++ features would you use to improve code quality?

[–]coinsinspace 3 ポイント4 ポイント  (6子コメント)

Sorry can't write a long post now (time & on smartphone), just two bullet points:
1. Smart pointers!!!
2. Type system. It's there for a reason. chain.cpp GetAncestor. The real type of that function is a Maybe monad. For c++ I would probably use boost::optional. Returning a null pointer is straight up C.
In a more general way a number that must be between 0 and 20 is not an (u)int - it's a different integer type. It's perfectly ok to use int for local variables etc, but for eg. block structures dependent types should be used. It makes rule enforcement local and in one place, makes code smaller, faster and much safer. Sprinkling ifs and asserts everywhere it's accessed is, again, C.

[–]observerc 2 ポイント3 ポイント  (0子コメント)

Actually the traditional C way is to return an integer outside your theoretical counter-domain.

It's not just since the advent of Maybe monad that NULLs have sucked. Already 30 years ago, if you you return NULL and int, you would be called stupid. Quite rightly so.

Looking at your examples... all those sily 'safety' checks at the top of each function: WTF, did this people missed their first programming lecture

[–]shibe5 0 ポイント1 ポイント  (4子コメント)

chain.cpp GetAncestor. The real type of that function is a Maybe monad. For c++ I would probably use boost::optional.

Do you mean return type? I don't see how boost::optional would make it better.

[–]coinsinspace 0 ポイント1 ポイント  (3子コメント)

Yes return type. Optional is semantic. You know getting an ancestor can fail just from its return type. On the opposite side, if you keep to that style, lack of optional means function can't fail, ie. it's a design invariant it returns something valid. That makes code self documenting and avoids pointless checks in the latter case, making code smaller, faster and easier to read.

[–]shibe5 1 ポイント2 ポイント  (2子コメント)

Except self-documentation, I don't think any of that is actually better than a plain pointer.

[–]coinsinspace 0 ポイント1 ポイント  (1子コメント)

If you ignore self-documentation, using a1, a2, a3, ... as names for all existing variables, functions and classes in your code is as good as any other naming scheme ;)

[–]shibe5 0 ポイント1 ポイント  (0子コメント)

Self-documentation is good, especially when there is not much any other documentation.

[–]bitcartel 2 ポイント3 ポイント  (2子コメント)

A lot of the boost library functionality can now be found in the C++11 standard library. Migrating as much as possible to the standard library would be a good improvement. Or C++14 since since gcc 5 fully supports it.

[–]coinsinspace 0 ポイント1 ポイント  (1子コメント)

I know, but core is c++03 afaik? Or used to be. Bu definitely is.

[–]shibe5 1 ポイント2 ポイント  (0子コメント)

I saw an anonymous function in Core.

[–]MR-reddit-user 0 ポイント1 ポイント  (0子コメント)

Not talking about bitcoin specifically, but C++11 does bring some nice things to the table like lambdas, smart pointers, etc.

[–]ForkiusMaximus 6 ポイント7 ポイント  (3子コメント)

Rebasing off btcd, bcoin, libbitcoin, etc. seems more promising. And I think there are already people working on adding adjustable blocksize caps to all those implementations.

[–]emc9469 2 ポイント3 ポイント  (1子コメント)

This. You don't fuck around with production code just for the purpose of refactoring or because you don't like long files. If you can't understand it, that's one thing. I'm sure core has been refactoring Satoshi's shit code for years. Ain't broke or there are better things to do done' fuck with it. BU should produce a patch for emergent consensus to the core client or other client and that is it. Stop trying to do everything at once, hubris already got you. None of this matters if your buggy code never gets consensus. So many plans so much cart before the horse.

[–]marcoski711 0 ポイント1 ポイント  (0子コメント)

I agree. OP's principle is sound, but doesn't move the needle, where it's needed, right now.

Let's get dynamic blocksize in multiple implementations, with single focus on the goal: fire core from their ivory tower. With the blocksize raised and back to a true meritocracy, then we can return to OP's otherwise excellent suggestion.

[–]veroxii 0 ポイント1 ポイント  (0子コメント)

Yeah I would prefer people keep the current reference implementations monolithic as is. But start breaking things out into libraries. Imagine lib-emergentconsensus, lib-segwit, lib-xthin etc. Then anyone can write their own client and link in what they want.

Over time these would mature and something else will take over as the de facto client. No need to go mess around with the existing code bases.

[–]ydtm 1 ポイント2 ポイント  (0子コメント)

Our long-term goal should be formal program verification for Bitcoin implementations

Long-term, if we want Bitcoin to become a multi-trillion-dollar economy, then it's eventually going to become necessary to start using formal methods of program synthesis and verification, eg:

https://github.com/johnyf/tool_lists/blob/master/verification_synthesis.md

Of course, this kind of stuff is hard to do for programs written in an imperative or procedural language such as C++ - since such languages have much more complicated semantics than declarative or functional languages.

Longer-term, it might therefore also be interesting to think about doing Bitcoin implementations in languages which are designed from the ground-up to support formal program synthesis and verification.

For example, ideally, our long-long-term goal should be to express a specification of Bitcoin using a theorem-proving language such Coq, and then extract an implementation in the functional but efficient OCaml (which has performance fairly close to C/C++, but with semantics that are several orders of magnitude easier to verify) - and then maybe even deploy the whole thing using MirageOS (unikernel). (Of course, if desired, certain performance-critical pieces could still be implemented in C++, and separately verified - and linked using Ocaml's (excellent, monadic) foreign-function interface.)

Alternatively (if it is desired to avoid a language such as OCaml, which fewer programmers are familiar with), it might be possible to do another implementation also in C++, more modular than Core/Blockstream's implementation - and then use some of the program verification tools for C++ listed in the link above. (I suspect the ones based on Linear Temporal Logic (LTL) might be most promising.)

This kind of effort could probably be based on the implementations libbitcoin, btcd / btcsuite, bcoin... which are already much more modular than Core's kludgy monolithic spaghetti code.

The "anyone-can-spend" hack of SegWit-as-a-soft-fork is an example of Core/Blockstream's warped priorities needlessly introducing entirely new classes of threat vectors

In particular, we should be concerned about SegWit - which was supposed to be a major code cleanup (refactoring), but instead ended up being yet more spaghettification - due to the peculiar requirements arising from Core/Blockstream's perverse and dangerous insistence on doing SegWit as a soft fork - which led to all sorts of needless convolutedness in the code - in particular the notorious "anyone-can-spend" hack which was unfortunately used to implement SegWit (and which was also unfortunately used to implement Pay-to-Script-Hash (P2SH)).

The problem with the "anyone-can-spend" hack is that it is very dangerous - it introduces a totally new class of threat vector into Bitcoin's codebase - because when transactions are "anyone-can-spend", then aa 51% attack can now steal other people's coins - versus previously, when a 51% attack could only double-spend your own coins, or mine extra coins.

The dangerous new class of threat vector which Core/Blockstream want to recklessly and needlessly introduce into Bitcoin with their totally unnecessary "anyone-can-spend" hack (ie, it would be unnecessary if SegWit were done properly: as a hard fork), is simply the latest "red flag" showing how Core/Blockstream's warped priorities (they report to central bankers, not to us users) are very, very damaging to Bitcoin.

Core/Blockstream actually work for central bankers - not for us - which explains why their code doesn't address many of our top priorities (low fees, reliable transactions)

It's all about vision, priorities, and skills - and Core/Blockstream have shown that they have tunnel-vision (ignoring major issues such as usability), warped priorities (they want to cripple on-chain capacity as part of their nefarious roadmap to force people off-chain), and mediocre coding skills (after 8 years of them "maintaining" the code, it's deteriorated into spaghetti-code).

Meanwhile, the low-information losers on r\bitcoin tend to automatically look up to Core/Blockstream devs as "wizards".

However, in the overall universe of programmers, Core/Blockstream devs are actually rather mediocre, on several levels:

  • Their ideas and priorities about what to write (specifications) have been far from optimal. Specifications are obviously supposed to be based on what actual users need. As we know, they have been failing miserably in this area - because their actual "users" happen to be the central bankers who fund Blockstream - not the Bitcoin community in general

  • Their methods of how to write it (their implementations) have likewise been focused not on things like making the code modular, easy to understand, easy to test. Plus, they have insisted on only upgrading via soft forks, instead of hard forks. Both of these maneuvers decrease the code quality and code security for Bitcoin - and increase the job security for Blockstream devs.

[–]nullc 8 ポイント9 ポイント  (7子コメント)

The bitcoin code that has been inherited from Core is a mess. When I saw that the latest assert was failing at line 5706, the first thing I thought was "Oh my God a file should never be 6000 lines long. That's a sign of undisciplined programmers generating an unreadable mess."

Stop assuming that Bitcoin Unlimited shittyness is found in Bitcoin Core.

The largest .cpp in Bitcoin Core is validation.cpp which is 4305 lines long. BU is undoing refactors and even merging thing and dumping them into large files.

You simply cannot test a 1,000 line function. There is too much going on to be able to set up a situation, call the function, check the result and say that everything is ok.

Did you even look at the function? Here is the whole thing stripped to one level deep:

 {
     LogPrint("net", "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->id);
     if (IsArgSet("-dropmessagestest") && GetRand(GetArg("-dropmessagestest", 0)) == 0){}
     if (!(pfrom->GetLocalServices() & NODE_BLOOM) &&
               (strCommand == NetMsgType::FILTERLOAD ||
                strCommand == NetMsgType::FILTERADD)){}
     if (strCommand == NetMsgType::REJECT){}
     else if (strCommand == NetMsgType::VERSION){}
     else if (pfrom->nVersion == 0){}

     const CNetMsgMaker msgMaker(pfrom->GetSendVersion());

     if (strCommand == NetMsgType::VERACK){}
     else if (!pfrom->fSuccessfullyConnected){}
     else if (strCommand == NetMsgType::ADDR){}
     else if (strCommand == NetMsgType::SENDHEADERS){}
     else if (strCommand == NetMsgType::SENDCMPCT){}
     else if (strCommand == NetMsgType::INV){}
     else if (strCommand == NetMsgType::GETDATA){}
     else if (strCommand == NetMsgType::GETBLOCKS){}
     else if (strCommand == NetMsgType::GETBLOCKTXN){}
     else if (strCommand == NetMsgType::GETHEADERS){}
     else if (strCommand == NetMsgType::TX){}
     else if (strCommand == NetMsgType::CMPCTBLOCK && !fImporting && !fReindex){} // Ignore blocks received while importing
     else if (strCommand == NetMsgType::BLOCKTXN && !fImporting && !fReindex){} // Ignore blocks received while importing
     else if (strCommand == NetMsgType::HEADERS && !fImporting && !fReindex){} // Ignore headers received while importing
     else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex){}
     else if (strCommand == NetMsgType::GETADDR){}
     else if (strCommand == NetMsgType::MEMPOOL){}
     else if (strCommand == NetMsgType::PING){}
     else if (strCommand == NetMsgType::PONG){}
     else if (strCommand == NetMsgType::FILTERLOAD){}
     else if (strCommand == NetMsgType::FILTERADD){}
     else if (strCommand == NetMsgType::FILTERCLEAR){}
     else if (strCommand == NetMsgType::FEEFILTER){}
     else if (strCommand == NetMsgType::NOTFOUND){}
     else {}
     return true;
 }

It's just a big dispatch with no interaction between the parts, so its size has effectively no impact on testability-- you just call it with the distinct non-interacting strCommands. There is also an open PR to split it, but you can't just go willy nilly moving things around in a project that has many ongoing changes.

The current methods of testing bitcoin (using python scripts to run and control the executable) are not professional and are completely inadequate. It's like testing a car by driving it around after it has been fully built. Yes, you must do that, but you should also test each component of the car to ensure that it meets specifications before you install that component. For bitcoin, that means writing tests in C++ for functions written in C++. Test the functions, not just the executable.

You know that there are a set of unit tests too, right? (Oh wait, BU's latest binary release fails them and they just turned them off...)

[–]boomtnt46 3 ポイント4 ポイント  (0子コメント)

Ok. Keep your idea in Core. We'll see what happens in a few years.

[–]homerjthompson_[S] 7 ポイント8 ポイント  (4子コメント)

Thanks, Greg.

Your explanation that "you can't just go willy nilly moving things around in a project that has many ongoing changes" as an excuse for keeping a 1,000+ line function is quite enlightening.

Your suggestion that the code can be kept in its current stinky state by also stinking up the tests is also enlightening.

The 4305 line file is truly a testament to Core's refactoring achievements. You must be very proud.

[–]nullc 1 ポイント2 ポイント  (3子コメント)

Lets see your shining code examples then?

[–]homerjthompson_[S] 4 ポイント5 ポイント  (2子コメント)

while (true) {
    troll(greg);
}

[–]nullc 4 ポイント5 ポイント  (1子コメント)

while (true) {
    troll(greg);
}

Pedantically, loops can generally be assumed to terminate by a standards conforming compiler and especially since trolling me has no side-effects, it would be permissible to optimize you out.

[–]homerjthompson_[S] 0 ポイント1 ポイント  (0子コメント)

Not if I throw an exception while trolling you.

[–]coinsinspace 0 ポイント1 ポイント  (0子コメント)

Omg, a 25 branches long if. After the second else if it should be changed into a switch, to not repeat that "strCommand ==". Changing the name of that variable now requires using either an ide or at least search&replace.

Now switch would still be C, but at least good C. C++ way would be to have a dispatcher where each msg registers itself: which means dispatching code doesn't have to be changed every time a new message is added. Separation of concerns.

Thanks to all this, I significantly changed my probability estimates about main core devs' real intentions! As Segwit is so anti-scaling that it had to be designed either by idiots or people with malevolent intent. :)

[–]FormerlyEarlyAdopter 1 ポイント2 ポイント  (0子コメント)

This is a pile of shit but please do not kick it, it will stink too much.

Unfortunately, partly thanks to rotten core "genius expert programmers", this is the current status of core codebase and forked software. Given 20 billion economy that depends on it and determined adversaries waiting to exploit it, kicking it by doing a major refactoring and technical debt reduction is very risky.

[–]STFTrophycase 0 ポイント1 ポイント  (0子コメント)

ITT: People who know nothing about software or the software development process trying to give advice on how to write software.

[–]TotesMessenger 0 ポイント1 ポイント  (0子コメント)

I'm a bot, bleep, bloop. Someone has linked to this thread from another place on reddit:

If you follow any of the above links, please respect the rules of reddit and don't vote in the other threads. (Info / Contact)

[–]Next_5000 0 ポイント1 ポイント  (1子コメント)

Curiously, what was the state of the code as given to us from Satoshi?

[–]bitmegalomaniac 5 ポイント6 ポイント  (0子コメント)

Fairly horrendous. Satoshi was not a programmer but he did fairly well for someone with a good idea and a hobbyist level of programming.

Unfortunately, some of the things he did have had long reaching negative implications. The current assert(0) BU fiasco is a fairly good example that is fresh in everyone's mind was introduced by his code and him only ever compiling in debug (leaving the asserts in). Removing them now is problematic because the behaviour of the code relies on those asserts. Core has been slowly working on removing them, unfortunately BU has been working on adding more.

[–]observerc 0 ポイント1 ポイント  (0子コメント)

You are right. Although I don't see the reason for being polite and using so many euphemisms. The code is shit by any proper standard.

A proper implementation would probably have a tenth of the extent of the current one. But what's the incentive to do it? This does have me intrigued, other coins ave set up a model that finances own development.

I would welcome a fork that would mint a certain amount of coins to themselves. If they would be upfront about it, I would have no problem with that.

[–]LightShadow 0 ポイント1 ポイント  (2子コメント)

Could the reference implementation be written in a non-C++ language that would help the readability? Maybe Python or Javascript??

[–]MR-reddit-user 0 ポイント1 ポイント  (1子コメント)

Javascript is more readable than C++?

[–]LightShadow 0 ポイント1 ポイント  (0子コメント)

The syntax is easier and the functionality is more obvious. In C++ there's lots of ways to do the same thing, and that can be confusing when many developers are working on a large code base.

[–]LightShadow 0 ポイント1 ポイント  (0子コメント)

If someone did a 3-5 hour walkthrough of the bitcoin code base and made a video of it, it would help a lot of potential developers get on board.

[–]cdn_int_citizen[🍰] 0 ポイント1 ポイント  (0子コメント)

I had no idea it was more than 6k lines in one file.... that is insane... but could also be a clever tactic to retain status as "experts". I dont know which is worse.

[–]Adrian-X 0 ポイント1 ポイント  (0子コメント)

more help needed - go to www.bitco.in/forum write up a BUIPXXX give it a name, ask for donations this is a mammoth project to be roiled out well in the future but i like your approach.

the current development style just makes incumbents more valuable and centralizes development.

[–]MR-reddit-user 0 ポイント1 ポイント  (0子コメント)

1300+ line functions!?

I think we should also have a ground up rewrite of bitcoin. Not to necessarily replace BU/Core/whatever other fork, but we need more different full implementations of bitcoin to make sure the network can't be taken down easily, to give the user choice, and to decentralize.

[–]veroxii 0 ポイント1 ポイント  (0子コメント)

Martin Fowler is pretty much the authority on this. One of his many books on the subject: https://martinfowler.com/books/refactoring.html

Compulsory reading before anyone starts on this.

[–]BornoSondors 0 ポイント1 ポイント  (0子コメント)

Stop writing 10 paragraph long rants, start actually doing.

[–]bitmegalomaniac -4 ポイント-3 ポイント  (0子コメント)

The bitcoin code that has been inherited from Core is a mess

You sound like Donald Trump.