Date: Tue, 12 Jul 2011 22:47:12 +0100
From: Michael Offel <michael.offel@web.de>
Message-ID: <602127524.20110712224712@web.de>
To: bitcoin-development@lists.sourceforge.net
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: michael.offel@web.de
Subject: Re: [Bitcoin-development] overall bitcoin client code quality
Precedence: list

Monday, July 11, 2011, 12:31:20 AM, Jeff Garzik wrote:
>> 2. isolation of modules
> It is a long term goal to move towards 'libbitcoin"
What  about creating a branch and start libbtc by implementing a small
module  like irc or p2p connection handling and use the new lib in the
client. I think this would be a proper start for a new clean code base
without  having  a  non  functional  client  for some time and it also
provides  some  kind  of red line between libbtc (cleaned up code) and
the old code base, making it easy to maintain order.
Would this approach be accepted for a merge?


Monday, July 11, 2011, 12:36:53 AM, Matt Corallo wrote:
>> While I can guess what the CScript class does I would more like to understand the thoughts behind the decision to implement some crypto macros in a compile time interpreter and 
>> 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?
It  was  more  meant  as an rhetorical question. A documented decision
would give anyone the chance of arguing against the usage of a library
instead  of asking stupid questions. A mailing list archive suits well
for  this  type  of information, so let me try to get some information
here.  Db4  is  an  excellent  choice  if  you  need  indexed database
functionality without SQL interface. But compared to an stl map lookup
and  fopen,  fwrite  and  fclose  it  is much harder to understand and
brings  a  lot  performance  overhead.  This  is  true as long as your
information are small enough to stay in main memory. A stl map storing
file  offsets  is  also  not that hard to write and understand. On the
other  side  using  an  SQL  interface  would  bring  the advantage of
swapping  database  providers.  An enterprise website could use oracle
while the average user could use sqlite. Also is db4 used for any type
of disk storage, this makes files like wallet.dat some kind of hard to
read. It is in no way more secure than storing private key's in an xml
file. But it is much harder to maintain and understand by the user and
the average programer.

> If you are on Windows, why are you on Windows? ;)
I'm  forced  to to use windows by the type of clients I'm working for.
And  during  leisure  I  like  to use a System that does not need much
effort to simply do what it is made for. ;)

>>
>> 6. hardcoded values
>>  
>> 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 chain 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.
It  is  not  that  interesting  as it looks first. There is no good in
running multiple chains for production use. To share the difficulty is
indeed  a  good  start  to  solve  the problem. That's also one of the
things  I  don't  like  off  the QBitcoin client. What I meant is just
to have the possibility to have all adjustable parameters in one place
and  to  be  able  to  quickly build an internal testnet without crazy
firewalling  to prevent it from dying. The first would allow to detect
problematic ddos protection settings early and giving the average user
the  possibility  to adjust all important settings if he knows what he
does.  That  includes  not  only alternate chains. One could choose to
include  transactions  only  at  a  higher  fee  or  at no fee at all.
Everyone could do such things by changing the code anyway. But not all
brilliant administrators or users are programmers.

>> The official Bitcoin client should be some kind of an reference project for 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...
Good code is the best documentation anyway.

Monday, July 11, 2011, 3:01:51 AM, Luke-Jr wrote:
>> 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.
> There are many rewrites in progress, often with much better designs.
There  is  no  other  client  that  uses  C  and is meant to be a full
implementation  and platform independent except QBitcoin. QBitcoin seems
to  have  no  public repository to work on or I have overlooked it ?!?
Starting  a  new client on my own is just like starting an other never
ending and never used open source project.

>> The official Bitcoin client should be some kind of an reference project
>> for other clients and must therefore be extra clean and well documented.
> Bitcoin is supposed to be an authorityless project. There is no official.
While there is no authority, there is just one fully working client to
look  at.  This  may  lead to an working but instable network if other
clients are trying to interpret net.cpp and fail on it in details.

>> *everything else*
> Fix it yourself and submit the changes. If they don't get merged, fork.
As  I  said, there is no need for an other never ending story. I would
like  to  know  if  my affords have a chance to get merged or accepted
before I start to work on it.

Tuesday, July 12, 2011, 4:31:07 AM, Gavin Andresen wrote:
> We'll just tell everybody to stop using bitcoin so much for six months
> or so while we implement a much better client.  It will be exactly
> like the bitcoin we have now, except with a much nicer internal
> architecture and much cleaner code-base, and we're pretty sure we can
> get it done in six months if everything goes exactly as planned.
It  is  some  kind of arrogant to believe that anyone would stop using
bitcoin  if some programers decide to stop working for some month. And
it  is  also fond to not fix bugs in the old code base if they appear.
Also  there are lots of people out there using old clients anyway. The
important  improvement is more about quick extendibility and therefore
more  feature  rich secure code. This would not only help the official
code  base,  it would also improve trust and result in better external
bitcoin related projects.

> Then again, it might be easier to modify the
> CRITICAL_SECTION code to detect and report deadlocks (anybody have
> experience doing that?).
That  would be true if possible, but I'm pretty sure that the only way
to  detect  deadlocks  is  by either analyzing the code or single step
simulating it, what is really tricky on network applications.

Tuesday, July 12, 2011, 6:19:28 AM, Jeff Garzik wrote:
>> While spying on the old code, I noticed one major problem that could be
>> fixed quite easily. That is, the 1 class-per .h/.cpp rule is completely
>> ignored in main.h/cpp and net.h/cpp If all of the classes in the project
>> were re-factored to their own files,
> This is about the last thing we should do, and it's one of the worst
> coding practices of many C++ projects (and unfortunately carried over
> to Java by force).  See Knuth and his work on literate programming.
> Putting logically similar code -together- is often more impactful and
> meaningful than splitting up files into dozens (hundreds or thousands,
> in large projects) of tiny, 20-line files.
We  seem  to have very different opinions on that, but let's try to be
objective.  I  belive  that every class should be able to stand on its
own.  That way it can be reused in other projects or situations in the
same  project.  And  it  is  much  more easy to catch and extend class
behavior  if  it  is isolated to one file instead of multiple files or
mixed between other class methods in one file. On the other hand, what
is  bad  on  having  50-80  code  files in bitcoin? In terms of source
control  it  even  gives some kind of easier to read history and fewer
merge  conflicts.  When  you  start  writing  a  class for exactly one
propose  in  one specific situation used by one other class you should
think   about  writing  a  nested  class,  which  can  and  should  be
implemented in the same cpp file. That way you can achieve you similar
code in one location while accepting the rule others like.
Another nice side effect is the ability to see a class dependency list
be looking at the include listing.

Tuesday, July 12, 2011, 8:21:12 AM, John Smith wrote:
> re:4) I also don't see why boost would be a 'nonstandard
> dependency'. From what I understand, a large part of the new C++0x
> standard is derived from boost. Also C++ compilers are getting
> faster and more smart all the time, so I absolutely don't see "build speed" as a goal.
Don't  get  me  wrong. If boost would be used for something meaningful
there  would  be  no point in removing it. Everything non questionable
about boost does already find its way into most stl implementations.
And  everything  that find it's way into C++ 0x does it for the reason
that  it  is  better handled by an language extension than by an boost
construct. Otherwise there would be no point in extending the language.

Michael