path: root/93/5a9a11ff85165f1eea235c3451bd5dbed782a1

Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193]
	helo=mx.sourceforge.net)
	by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76)
	(envelope-from <bitcoin-list@bluematt.me>) id 1Qg3Yl-0003xW-Al
	for bitcoin-development@lists.sourceforge.net;
	Sun, 10 Jul 2011 23:37:11 +0000
Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of bluematt.me
	designates 208.79.240.5 as permitted sender)
	client-ip=208.79.240.5; envelope-from=bitcoin-list@bluematt.me;
	helo=smtpauth.rollernet.us; 
Received: from smtpauth.rollernet.us ([208.79.240.5])
	by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:AES256-SHA:256)
	(Exim 4.76) id 1Qg3Yk-0000TY-8L
	for bitcoin-development@lists.sourceforge.net;
	Sun, 10 Jul 2011 23:37:11 +0000
Received: from smtpauth.rollernet.us (localhost [127.0.0.1])
	by smtpauth.rollernet.us (Postfix) with ESMTP id 8D67959400C
	for <bitcoin-development@lists.sourceforge.net>;
	Sun, 10 Jul 2011 16:36:48 -0700 (PDT)
Received: from mail.bluematt.me (unknown
	[IPv6:2001:470:9ff2:2:20c:29ff:fe16:f239])
	(using TLSv1 with cipher ADH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	(Authenticated sender: @bluematt.me)
	by smtpauth.rollernet.us (Postfix) with ESMTPSA
	for <bitcoin-development@lists.sourceforge.net>;
	Sun, 10 Jul 2011 16:36:45 -0700 (PDT)
Received: from [IPv6:2001:470:9ff2:1:2c0:caff:fe33:858b] (unknown
	[IPv6:2001:470:9ff2:1:2c0:caff:fe33:858b])
	by mail.bluematt.me (Postfix) with ESMTPSA id 222ECB3CC
	for <bitcoin-development@lists.sourceforge.net>;
	Mon, 11 Jul 2011 01:36:53 +0200 (CEST)
From: Matt Corallo <bitcoin-list@bluematt.me>
To: bitcoin-development@lists.sourceforge.net
In-Reply-To: <97305540.4426247.1310337435268.JavaMail.fmail@mwmweb052>
References: <97305540.4426247.1310337435268.JavaMail.fmail@mwmweb052>
Content-Type: multipart/signed; micalg="pgp-sha1";
	protocol="application/pgp-signature";
	boundary="=-fBygYMKXI+bpycgt+ob8"
Date: Mon, 11 Jul 2011 01:36:53 +0200
Message-ID: <1310341013.2230.66.camel@Desktop666>
Mime-Version: 1.0
X-Mailer: Evolution 2.32.2 
X-Rollernet-Abuse: Processed by Roller Network Mail Services. Contact
	abuse@rollernet.us to report violations. Abuse policy:
	http://rollernet.us/abuse.php
X-Rollernet-Submit: Submit ID b58.4e1a378d.eda.0
X-Spam-Score: -1.5 (-)
X-Spam-Report: Spam Filtering performed by mx.sourceforge.net.
	See http://spamassassin.org/tag/ for more details.
	-1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for
	sender-domain
	-0.0 SPF_HELO_PASS          SPF: HELO matches SPF record
	-0.0 SPF_PASS               SPF: sender matches SPF record
X-Headers-End: 1Qg3Yk-0000TY-8L
Subject: Re: [Bitcoin-development] overall bitcoin client code quality
X-BeenThere: bitcoin-development@lists.sourceforge.net
X-Mailman-Version: 2.1.9
Precedence: list
List-Id: <bitcoin-development.lists.sourceforge.net>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
	<mailto:bitcoin-development-request@lists.sourceforge.net?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=bitcoin-development>
List-Post: <mailto:bitcoin-development@lists.sourceforge.net>
List-Help: <mailto:bitcoin-development-request@lists.sourceforge.net?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
	<mailto:bitcoin-development-request@lists.sourceforge.net?subject=subscribe>
X-List-Received-Date: Sun, 10 Jul 2011 23:37:11 -0000


--=-fBygYMKXI+bpycgt+ob8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, 2011-07-11 at 00:37 +0200, Michael Offel wrote:
> Hello,
> =20
> I would like to start a discussion about code quality to catch some opini=
ons and create an codebase cleanup plan.
> =20
> Let me just start with some points I've seen while reading and stepping t=
hrow bitcoin:
> =20
> =20
> 1. nearly no code documentation
Yep, anyone with the time can gladly comment up the code, it would be
much appreciated, but as it stands now there are more important things
to do...like many of the things here:

> All I found was the original paper and some partial wiki pages and the ov=
erall coding style does not help much here. I would love to see some class =
hierarchy, method descriptions and thoughts in the code.=20
Yes, thats one of the general development goal, sipa's CWallet was an
excellent start, but much more work needs done in terms of clear
splitting of the code.

> Instead one can find comments like this...
> =20
> >    // Map ports with UPnP
> >    if (fHaveUPnP)
> >        MapPort(fUseUPnP);
> =20
> This comment is just waste of valuable disk space and time for anyone who=
 reads it.
My bad, was just following the previous comments...
> While I can guess what the CScript class does I would more like to unders=
tand the thoughts behind the decision to implement some crypto macros in a =
compile time interpreter and=20
> why Berkeley db 4 is used at all.
At the time Bitcoin began being built, Ubuntu 9.04 (or was it 9.10?) was
used, as it offered the oldest libc on the newest OS.  Ubuntu 9.04 just
happened to only have db4.7.  For backward compatibility, db4.7 has been
used ever since (except, for some reason, the osx builds).  In 0.4,
db4.8 will be used.  If you are asking why bdb was used to begin with,
why not? its an excellent db and why reinvent the wheel?

> =20
> 2. isolation of modules
> =20
> It would be a lot easier to understand parts of the code if they would us=
e well defined interfaces to communicate. Bitcoin makes use of global varia=
bles, public member variables in classes and global external functions what=
 makes 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 thread started and pushes received addresse=
s directly to some global function in net.cpp. The code is full of concepts=
 like this.
> A well defined interface would be an bitcoin unrelated IRC module interfa=
ce and some kind of translation class between the IRC and peer2peer network=
 interface.
Though satoshi was clearly brilliant, he didn't care much for code
cleanliness.  This is one of the next development goals (IMO).

> =20
> 3. poor use of threads
> =20
> Bitcoin starts a new thread for every little module it has and as soon as=
 there is some serious work to do, it locks some kind of global mutex. Whil=
e this eliminates nearly every performance advantage, it introduces a high =
difficulty in writing bug free code.
> Not only that one has to know which mutex to lock when, there is no docum=
entation about that, one may also accidently add dead locks.
> This kind of bug is very hard to find, it may run well for a million of t=
ests and crash or just hang on the next one. And my experience tells me tha=
t it will not be an developer nor an little user when it occurs.
> The fist user who hit's the bug will be mt gox doing an emergency transfe=
r. ;)
This is something that will come with general code cleanup and
modularization.  The locks will become specific to the object (as they
should be) and the performance and clarity will be fixed.

> My suggestion is to remove all threads and critical sections and build a =
sequential easy to follow model.
> Some modules like the cpu miner may still require to spawn threads, but h=
e should do this internally and hide any synchronization.
Though it would be ideal to rewrite 90% of Bitcoin just to fix code
clarity, that is way more work than anyone has time for, in the mean
time there is more than just code cleanup that needs done.  It has to be
done in chunks.

> =20
> 4. long build times
> =20
> 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=
 boost, lack of module isolation and implementations in header files.
> While the 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=
 threading on the table,
> but using pthread or making an platform dependent startthread and mutex w=
ould be much more lightweight and nobody needs FOREACH. Boost is also an he=
avy non standard dependency that is an unnecessary barrier for new develope=
rs.
Header files could stand to be cleaned up a bit, though all the
implementation stuff is limited to one or two lines (though sometimes
thats too much).  If you want to rewrite Bitcoin sans-boost, please do,
however Boost really isnt a huge barrier as its a build-once thing.  If
you are on Linux, all you have to do is install a bunch of packages and
build wx.  If you are on Windows, why are you on Windows? ;)

> =20
> 5. style guide
> =20
> 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 lon=
ger than a screen page. It should be natural to write code like this and I =
dislike having a lot of rules but the code shows that there is a need for s=
uch thing.
Its not due to the current coders, its due to how it was originally
written.

> =20
> 6. hardcoded values
> =20
> There are tons of hardcoded values for the official and the testnet block=
 chain. It would be a great step to move all chain related settings to a ch=
ain description file. This would allow custom chains and clean up the code.
This one is an interesting debate.  There is no real reason to do this
aside from some questionable code cleanup.  Also, there is no reason to
encourage improperly-implemented alternate chains.  Alternate chains
should be designed in such a way as to share the main chain's difficulty
as described by Mike on the forum, not just make a new chain and hope it
sticks.

> =20
> All this is just the tip of the iceberg. Bitcoin is a widely used applica=
tion and users are transferring millions of dollars. The current code quali=
ty 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 and it was necessary to b=
ring out a working version to attract more developers.
> 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 c=
onfidence to see and report obvious bugs.
> =20
> Let me also say that I'm not pointing to someone to do all this. I'm will=
ing to spend a lot of time on this promising project but this kind of clean=
up 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=
 code rather than moving a lot of "known to be somehow functional" around.
Really no reason to do that.  Although the code is messy in terms of
global usage and poorly-implemented RPC/net/etc, most of the code is
absolutely fine.  Just throw it in clearly-defined methods and classes
and it would be much more readable and less prone to mistakes.
Additionally, the things that are poorly-implemented can be slowly
changed over time in a clean and independent fashion instead of having
to rewrite massive chunks at a time.  Even if we had a full-time
development team of many, many developers, this isn't the right way to
do it.  The code itself is cleaner that it first appears, even if its
global structure is not.

> The official Bitcoin client should be some kind of an reference project f=
or other clients and must therefore be extra clean and well documented.
True, but it is much higher priority to clean up the code than comment
it better, plus there are various other features/more user-facing issues
that need fixed as well, so...
> =20
> Hopefully I did not hurt someone's feelings.
Don't think so, the code sucks in terms of cleanliness, everyone knows
it, its just a question of who is going to and when its going to get
fixed.

Matt

--=-fBygYMKXI+bpycgt+ob8
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAABAgAGBQJOGjeQAAoJEBrh01BD4I5UZAwP/0wf/pF2NGAs3IYw8a7CfI7I
T+Ni6UKXFdm8ROc7eXKR7vXgkl+ocYeeupXv4muFsJYl6uIkOdJRIJ+L/CAacVAa
yoRCAPXi4h/GaP/MNM4mdmvTHY5hQAfyJ8qpVAR3VPQmkJgnUm7vnZLawJBd5qLO
5kPBowo2xqnkgBLvfHEH4kUQ3NPTv1AHx8s7gFD8ompT28UaqZy2IUx5gDXH9UfD
GqU8n2qb0cde3HRRwqAPgWg7EBPk8OMOvaKt4XYiKskdgu4jGTv+VS3WNgGuxWwo
2MRNJul7idKhKIFRsmifYYTXN9MUd1qF9mgX6NBvFSJFVKyHExzJWrwGa0hgLroU
IBGHV4S7sJsKESh0plNmfLmwS7aWIhtVobOVcZW/Cwa5KFL5u2sZGDosTOFGxNCr
J1O7VDIB9kO5436859+XxdjupO/Y8t2N9QH1IwmwRIE/PN/ZABA3A1iK9/l1yXZM
HOz0l/MbdOd2niCETGS+UthIcjQaXL8+BWuy6oAFIy5zXLBJxJyAZrRpJcIo+7SL
cfXs9SjupXWkorV2BRwzHEomyc2WCNrR5fzqGY1KyQnKlr34q7V9J6B3CiVQVBtl
oBURXkLvmiQo3mSKZkypAMAZlb3fjdhgXpUzT1ACYrekD8CBS0NECgpobwvrhLW3
1snWB6Ww7Z1EFrn/xgVZ
=v2E1
-----END PGP SIGNATURE-----

--=-fBygYMKXI+bpycgt+ob8--