From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sog-mx-1.v43.ch3.sourceforge.com ([172.29.43.191] helo=mx.sourceforge.net) by sfs-ml-4.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1Qg2cs-0003wz-35 for bitcoin-development@lists.sourceforge.net; Sun, 10 Jul 2011 22:37:22 +0000 Received: from fmmailgate05.web.de ([217.72.192.243]) by sog-mx-1.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1Qg2cr-0006zl-1e for bitcoin-development@lists.sourceforge.net; Sun, 10 Jul 2011 22:37:22 +0000 Received: from mwmweb052 ( [172.20.18.61]) by fmmailgate05.web.de (Postfix) with ESMTP id 44D446536A6B for ; Mon, 11 Jul 2011 00:37:15 +0200 (CEST) Received: from [87.194.33.247] by mwmweb052 with HTTP; Mon Jul 11 00:37:15 CEST 2011 Date: Mon, 11 Jul 2011 00:37:15 +0200 (CEST) From: "Michael Offel" To: bitcoin-development@lists.sourceforge.net Message-ID: <97305540.4426247.1310337435268.JavaMail.fmail@mwmweb052> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Priority: 3 Importance: normal Sensitivity: Normal X-Provags-ID: V01U2FsdGVkX18Qv1qUCYkGzbCzkTWu/6PBTI3+cel9daLWfQQlVWvLL7Np1wFpYzwg eZ7eJJvp6GRVTzi/yqttLyGy5PeGNkRx X-Spam-Score: 0.0 (/) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (michael.offel[at]web.de) -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/, no trust [217.72.192.243 listed in list.dnswl.org] -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay domain 0.0 T_TO_NO_BRKTS_FREEMAIL To: misformatted and free email service X-Headers-End: 1Qg2cr-0006zl-1e Subject: [Bitcoin-development] overall bitcoin client code quality X-BeenThere: bitcoin-development@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Jul 2011 22:37:22 -0000 Hello, =C2=A0 I would like to start a discussion about code quality to catch some opinion= s and create an codebase cleanup plan. =C2=A0 Let me just start with some points I've seen while reading and stepping thr= ow bitcoin: =C2=A0 =C2=A0 1. nearly no code documentation =C2=A0 All I found was the original paper and some partial wiki pages and the over= all coding style does not help much here. I would love to see some class hi= erarchy, method descriptions and thoughts in the code. Instead one can find= comments like this... =C2=A0 >=C2=A0=C2=A0=C2=A0 // Map ports with UPnP >=C2=A0=C2=A0=C2=A0 if (fHaveUPnP) >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MapPort(fUseUPnP); =C2=A0 This comment is just waste of valuable disk space and time for anyone who r= eads it. While I can guess what the CScript class does I would more like to= understand the thoughts behind the decision to implement some crypto macro= s in a compile time interpreter and why Berkeley db 4 is used at all. =C2=A0 =C2=A0 2. isolation of modules =C2=A0 It would be a lot easier to understand parts of the code if they would use = well defined interfaces to communicate. Bitcoin makes use of global variabl= es, public member variables in classes and global external functions what m= akes understanding the code a lot harder. To give an example here, the irc = module has no interface at use it or to use. It gets some kind of magic thr= ead started and pushes received addresses directly to some global function = in net.cpp. The code is full of concepts like this. A well defined interfac= e would be an bitcoin unrelated IRC module interface and some kind of trans= lation class between the IRC and peer2peer network interface. =C2=A0 =C2=A0 3. poor use of threads =C2=A0 Bitcoin starts a new thread for every little module it has and as soon as t= here is some serious work to do, it locks some kind of global mutex. While = this eliminates nearly every performance advantage, it introduces a high di= fficulty in writing bug free code. Not only that one has to know which mute= x to lock when, there is no documentation about that, one may also accident= ly add dead locks. This kind of bug is very hard to find, it may run well f= or a million of tests and crash or just hang on the next one. And my experi= ence tells me that it will not be an developer nor an little user when it o= ccurs. The fist user who hit's the bug will be mt gox doing an emergency tr= ansfer. ;) My suggestion is to remove all threads and critical sections and= build a sequential easy to follow model. Some modules like the cpu miner m= ay still require to spawn threads, but he should do this internally and hid= e any synchronization. =C2=A0 =C2=A0 4. long build times =C2=A0 It takes longer to build Bitcoin than building some of the million lines of= code projects I'm working on. The reasons I did see so far is the use of b= oost, lack of module isolation and implementations in header files. While t= he rest is just bad coding style the use of boost is arguable. As far as I = can see it is mainly used for threading and FOREACH. I already put threadin= g on the table, but using pthread or making an platform dependent startthre= ad and mutex would be much more lightweight and nobody needs FOREACH. Boost= is also an heavy non standard dependency that is an unnecessary barrier fo= r new developers. =C2=A0 =C2=A0 5. style guide =C2=A0 There is a style guide but it does not include anything about readability. I'm talking about one file per class, no methods and single code line longe= r than a screen page. It should be natural to write code like this and I di= slike having a lot of rules but the code shows that there is a need for suc= h thing. =C2=A0 =C2=A0 6. hardcoded values =C2=A0 There are tons of hardcoded values for the official and the testnet block c= hain. It would be a great step to move all chain related settings to a chai= n description file. This would allow custom chains and clean up the code. =C2=A0 =C2=A0 All this is just the tip of the iceberg. Bitcoin is a widely used applicati= on and users are transferring millions of dollars. The current code quality= is very prone to bug's. To be clear I'm not saying there are a lot of bugs= nor do I blame someone for the code quality. It is still a beta version an= d it was necessary to bring out a working version to attract more developer= s. And it is hard to analyze the code or just see a bug during development.= One can be happy to understand what a method does, but this gives not the = confidence to see and report obvious bugs. =C2=A0 Let me also say that I'm not pointing to someone to do all this. I'm willin= g to spend a lot of time on this promising project but this kind of cleanup= is simply too large for one person who is new to the code. My overall suggestion is to begin a complete rewrite, inspired by the old c= ode rather than moving a lot of "known to be somehow functional" around. The official Bitcoin client should be some kind of an reference project for= other clients and must therefore be extra clean and well documented. =C2=A0 Hopefully I did not hurt someone's feelings. =C2=A0 Michael =C2=A0 =C2=A0 ___________________________________________________________ Schon geh=C3=B6rt? WEB.DE hat einen genialen Phishing-Filter in die Toolbar eingebaut! http://produkte.web.de/go/toolbar