Source code documentation

[AndrewBuck]

Hello to the other developers, I finally got a working build environment set up on my Ubuntu machine here and have been perusing the sourcecode a bit.  So far I like what I am seeing, the program makes use of the standard template library to avoid messy data structure code and the class structure seems to make pretty good use of public/private access to promote modularity.

The thing that really seems to be lacking though is a good organizational structure between the .h and .cpp files, as well as a lack of function documentation.  I would be happy to start writing documentation for the functions using the doxygen documentation system.  I am using this on OpenDungeons which is a game I am the lead coder on and it has served me well.

For those that are unfamiliar with it, Doxygen works by reading your sourcefiles to extract information which it automatically collects (like function parameters, class organization and inheritance, etc), as well as information you add yourself (comments describing what functions do, what variables are used for, etc).  After it parses all of the code it produces  a directory containing a bunch of HTML files which are very well crosslinked and easy to navigate.  It can also be configured to autogenerate neat little call graphs showing what subfunctions are called from each function (so you can trace dependancies/bugs).

All in all Doxygen is an excellent system and I would be happy to get it set up and begin writing documentation for the functions (at least the ones I can figure out anyway) and provide patches so they can be uploaded to SVN.  I don't want to do this though if other developers would be opposed to it so I wanted to post here before I got started.  Let me know if you would like me to do this.

EDIT:  In case people want to see what the documentation on an existing system looks like, here is a link to the documentation for the OGRE 3d rendering system.  The best place to get a feel for the documentation is to click the "Classes" link at the top and then look at the pages for a couple of classes.

-Buck

[1715392500]

1715392500

[lachesis]

I think that would be cool, but as always, the decision rests with Satoshi, the project lead.

[AndrewBuck]

I think that would be cool, but as always, the decision rests with Satoshi, the project lead.

I didn't think this would be something people would object to.  Hopefully I hear definitively from Satoshi that it is OK.  I will probably start working on this tonight then (tonight in USA time).  Also, I saw another thread questioning how to send in patches; the stated answer was that we should e-mail them to Satoshi, is this still the preferred method?

-Buck

[satoshi]

I like that in libraries for the external API's, but you can probably tell from the code that I'm not a fan of it for interior functions.  Big obligatory comment headers for each function space out the code and make you hesitate about creating a small little function where the comment header would be bigger than the function.  They're some trouble for maintenance, as changes to the function then require duplicate changes in the comment header.  I like to keep code compact so you can see more code on the screen at once.

To add them now at this point, what would be written would just be what's obvious from looking at the function.

The external API we have, in rpc.cpp, the usage documentation is in the help string.

Sorry to be a wet blanket.

[AndrewBuck]

No problem, that is why I asked about documentation before I started writing it.  I still would like to document it though and maybe we can come to some system which you would find acceptable.  One possible way is that I just run Doxygen on my own code and just use the auto-generated docs without adding any descriptions, etc.  This would have no impact on the project and myself or anyone else could do this whenever but it limits the usefulness of the documentation.

A second, and perhaps more appealing method would be to utilize the fact that Doxygen does not require the added documentation to be in the same file as the source code it is documenting.  We could add a single file that contains the documentation blocks with links pointing to the function names.  Doxygen then combines this with the auto-generated info it collects from the source to produce the docs.

Finally, whether we use Doxygen or not, I would like to write a "man page" for the program documenting the command-line options it takes.  Where is the command line processed in the code?  I looked at main.cpp and didn't see it (in fact I couldn't even find the "main" function).

-Buck

[satoshi]

It's in init.cpp.

It's a wxWidgets app, so it doesn't have a main() function.  It may in a little while, since I'm pretty close to making bitcoind build w/o wxBase.  (it'll be in init.cpp)

Sorry about my choice of the filename "main.cpp", another possible name would have been "core.cpp".  It's much too late to change.  I still prefer main.cpp.

We're still in great need of sample code showing the recommended way to use the JSON-RPC functions, like for a basic account system on a typical storefront website.  Using getreceivedbylabel using the username as the label, changing to a new bitcoin address once the stored one for that account gets used.  I posted a sample code fragment on the forum somewhere.  (search on getreceivedbylabel or getnewaddress)  The sample code could be a plain vanilla bank site where you can deposit and send payments.

[AndrewBuck]

I don't know anything about JSON and my knowledge of website design is quite limited so I will be of no use here.  I do have pretty extensive C++ knowledge though so I should be able to do a bit of work on the codebase.

I am currently tweaking the commandline handling a bit to make it behave more like the standard unix/linux command line (specifically I am adding -h to display help).  I also plan to get the man-page text written up based on the current code.

I will put the man page on the wiki when I get it written, where should I submit the patch for the source code?

EDIT:  Actually since I am writing the man page in the man page markup language I will include this in the patch as well since it would look like nonsense on the wiki.

-Buck

[AndrewBuck]

I am still crunching along on the man page, it is starting to look pretty good.  I did come across a couple of issues though.  First, there is a "-printtodebugger" command line option.  I added it to the "usage" information when you give -h and I also added it to the man page but I can't tell what it does by looking at the code, other than to see that it is only used on windows machines.

Secondly, in the process of investigating the OutputDebugStringF() function I discovered that the code has the ability to output to a console by setting the global variable fPrintToConsole to true.  The variable, however, was initialized to false and there was no where in the code that sets it to true.  Because of this I added a -printtoconsole which sets both this variable and fDebug to true.  I think both should be set but I am wondering if I should leave fDebug alone.  If I don't set it in the code then the user would have to do "-printtoconsole -debug" on the command line instead, we may or may not want this depending on whether there is more/less verbosity implied by debug but from a quick grep of the code it looks like it should be the way I currently have it where it automatically sets both variables.

-Buck

[AndrewBuck]

Sorry to keep replying to myself but I am posting these little snippets as I stumble across them.  I am currently documenting the -dropmessagestest command line switch and I see a minor improvement you may want to add to the code.  Currently if a message is dropped as a result of this switch, a debug message is printed to the log indicating a message was dropped.  It would probably be pretty useful to print the contents of the dropped message to the log as well.  This way if one of the dropped messages really does break bitcoin, you can see what message caused the problem.  It would also allow you to later re-create the problem by adding a filter on the incoming network stream blocking messages like the one that broke it the first time so that you can verify that you have fixed the issue later on.

If you think it is a good idea I can probably add this myself as well.

-Buck

[Insti]

Thanks Andrew.
All your hard work is not going unnoticed.

[AndrewBuck]

Thanks Insti.  I got the man page written.  I documented all of the command line switches the program supports, not just the ones in the usage output given by "bitcoin -h".  Any of the ones that do not appear in the usage output should have a comment in the man page file noting this (comment lines start with   ."   which is a period then a double quote).

I provided the update as a patch since I also made some changes to the sourcecode itself.  If you want to install the man page first extract it from the diff file with the patch program and then copy the file "bitcoin.1" to section 1 of you man pages.  On my Ubuntu systems the directory /usr/local/man/man1/ serves as a good place to keep it.  Actually on my systems I put a symlink from there to the bitcoin/trunk/bitcoin.1 file so it stays up to date if it gets updated in SVN.

I have also exported the man page using man2html and will post this to the wiki so people can reference it without having to install it on their systems, as well as for the underprivileged Windows users who have to forgo the true awesomeness of the man page system. 

Attached to this message you should find a zip file with the patch in it.

EDIT:  The wiki doesn't seem to be sending the registration e-mail so I can log in to edit, is there some problem with the server or something?

-Buck

[satoshi]

I didn't realize you were going to document all the intentionally undocumented commands.  They're unsupported and not intended to be used by users.

All the user-facing commands are listed in the -? help.

[AndrewBuck]

I wondered if they were not really meant to be public facing.  I think they are useful to have documented though, at least for now.  If the program accepts it as valid input it should be documented.  You can either add a notice to the commands in the man-page that are experimental, or just remove them.  In my opinion it makes more sense to label them experimental so people know they might change, but can use them if they need to.  I would just add the following to the beginning of each of the commands in the makefile:

\fBUnsupported - Behaviour may change in future versions\fR

The \fB switches on bold text and the \fR switches back to regular.  This way people can use the program to its fullest potential during the development stages.  Too much documentation is never a bad thing, especially for an open source project.  Since people can see the code they will find these calls anyway and use them whether you want them to or not.  If they are documented and marked as volatile then people can at least make an informed choice on whether or not they want to use them.

For example, just at the moment someone in IRC is making use of the -printblock command to generate statistics about the block chain that might help us understand the program better (as in how it performs in the real world).  Although the output of this command may change in the future, and therefore we shouldn't be building complex frameworks around it, it is nice to know it exists if you need something done as a quick hack.  Also because the program is open source, if someone comes to depend on a certain command line switch they can maintain it.  Eventually what you thought was just a temporary debugging tool make end up being one of the most widely used switches available.

-Buck

[satoshi]

They're only intended for intrepid programmers who read the sourcecode.

[AndrewBuck]

So basically what you are saying is that the program you are writing, one which you expect people to do banking and financial transactions with, should have undocumented interfaces available only to the people who dig through the sourcecode -- and then even then after finding these interfaces they should have no indication of whether they are meant to be there (and were left out of the docs by accident) or whether they could be removed/changed at any time.

The -printblock command is an excellent interface for other people to build frameworks that want to study the usage and scalability of the client.  The -noirc command would be good for privacy concerned people who want to limit how much they advertise their presence on the network.  The -dropmessagetest would be very useful to the guy who built a modified client with a new blockchain to form a test network for security/scalability testing.  All of these are "for developers only" according to your definition, even though they are quite useful.

I have spent the last week setting up a build environment, learning your codebase, helping people on IRC, and writing tools to make the bitcoinmarket function more effectively.  In addition to that I spent ~2 days preparing the man page so others wouldn't have to spend as much time doing the same thing.  I did this with the intention of improving the client as well as the protocol since I think the idea behind the program is a brilliant one.  Frankly however, given your approach to the open-source development process, I am beginning to think my time would be better spent elsewhere.

I have posted the current version of the man page to the wiki here if anyone is interested in using it.  I may or may not continue to develop the client, but will likely continue to work with Dwdollar on the market stuff.

-Buck