The Great Code Documentation Effort, Part I #1899

rustyrussell · 2018-08-30T10:22:17Z

Obviously, I couldn't resist some cleanups along the way. Part II is finished already, but I wanted feedback on this one.

The idea is to walk through the code and along the way point out the techniques and style we use; as it goes on, it will concentrate more on the specifics of the particular daemon. It should be read in order, however.

renepickhardt

thanks for this documentation. Would have helped a lot one month ago but still helped me right now. I guess there is still a large gap for most people to get onborded

renepickhardt · 2018-08-30T13:33:29Z

doc/HACKING.md

@@ -14,24 +14,10 @@ Getting Started
 It's in C, to encourage alternate implementations.  Patches are welcome!
 You should read our [Style Guide](STYLE.md).

-To read the code, you'll probably need to understand `ccan/tal`: it's a


It lookes like the hint about ccan/tal is now completely removed. When I first sterted reading the code it was actually helpful for me to know about this particular data structure. Acutally I would rather have enjoyed some more explaination and paterns how to use it. (for example in the beginning I thought ctx is short for commitment tx and not for context was quite confusing)

OK, I'll move this to the first occurrence in the code then.

renepickhardt · 2018-08-30T13:38:06Z

lightningd/lightningd.c

+ * The role of this daemon is to start the subdaemons, shuffle peers
+ * between them, handle the JSON RPC requests, bitcoind, the database
+ * and centralize logging.  In theory, it doesn't trust the other
+ * daemons, though we expect hsmd to be responsive.


When being completely new I will wonder what hsmd is. first of all I do not even realize that it is hsm-daemon I don't know wht hsm is and I don't understant why it is cruicial.

renepickhardt · 2018-08-30T13:39:32Z

lightningd/lightningd.c

@@ -18,11 +48,14 @@
 #include <ccan/tal/grab_file/grab_file.h>
 #include <ccan/tal/path/path.h>
 #include <ccan/tal/str/str.h>
+
+/*~ This is common code: routines shared by one or more programs. */


programms or daemons?

lightning-cli in at least one case... I've expanded.

renepickhardt · 2018-08-30T13:41:18Z

lightningd/lightningd.c

+ * It's another one of Rusty's projects, and we copy and paste it
+ * automatically into the source tree here, so you should never edit
+ * it.  There's a Makefile target update-ccan to update it (and add modules
+ * if CCAN_NEW is specified). */


maybe include the removed comments from HACKING.md here?

renepickhardt · 2018-08-30T13:46:52Z

lightningd/lightningd.c

 	if (getenv("LIGHTNINGD_DEV_MEMLEAK"))
 		memleak_init();
 #endif

+	/*~ These are CCAN lists: an embedded double-linked list.  It's not


if I remember correnctly there are c-macros involved to create such CCAN data structure. As someone new to the project I am confused when to use such macros, if they are autogenerated and how the general process should look like. Maybe we could include a link examples or even provide some small CCAN best practices in a seperate code folder.
on a more meta level: I am not an experienced c programmer. I realize that the CCAN libs seem to ease up stuff but they made the code rather hard to read for me

I'd prefer to simple put an example inline if we need it. Having a single go-to place to read is always better than some out-of-the-way file nobody will read or maintain...

I ended up just listing some helpers and fleshing it out a bit more. I'd rather reference them as we see their use. I tried to create a minimal example, but it was contrived and still 20 lines :(

renepickhardt · 2018-08-30T14:40:42Z

lightningd/lightningd.c


-	/* Initialize wallet, now that we are in the correct directory */
+	/*~ Our "wallet" code really wraps the db, which is more than a simple


what more is it?

renepickhardt · 2018-08-30T14:43:30Z

lightningd/lightningd.c

 	io_poll_debug = io_poll_override(io_poll_lightningd);

-	/* Set up HSM. */
+	/*~ Set up HSM: it knows our node secret key, so tells us who we are. */


as the hsmd was mentioned to be the one daemon that needs to be responsive we should explain this a little bit more here?

renepickhardt · 2018-08-30T14:47:49Z

lightningd/lightningd.c


-	/* Create RPC socket (if any) */
+	/*~ Create RPC socket (if any): now we can talk to clients. */


I believe this one is ncessary to have lightning-cli. Maybe it is standard stuff for people familar with rpc but how is such an API exposed (sure over unix domain socket) but where in the code are api calls provided? I guess this doesnot happen in jsonrpc so a hint in this comment would be nice.

Currently the JSON-RPC autoregisters callbacks using the ccan/autodata module, however that poses some issues if we'd like to use clang based sanitizers since it does weird things with uninitialized data, so maybe we'd want to ween off that anyway at some point.

renepickhardt · 2018-08-30T14:50:53Z

lightningd/lightningd.c

 	crashlog = ld->log;

+	/*~ The root of every backtrace (almost). */


is this the event loop?

if yes where does io_loop come from? would elaborate here a little bit

renepickhardt · 2018-08-30T14:53:33Z

lightningd/lightningd.c

@@ -482,5 +699,7 @@ int main(int argc, char *argv[])
 	memleak_cleanup();
 #endif
 	daemon_shutdown();
+
+	/*~ Farewell.  Next stop: hsmd/hsm.c. */


why is it actually lightningd.c with a d but hsm.c without d ?

Good point. That's bugged me for a while too, so I renamed them all...

Signed-off-by: Mark Beckwith <wythe@intrig.com>

rustyrussell · 2018-08-30T23:13:22Z

@renepickhardt Thank youso much for the detailed review. And sorry I didn't write this s month ago :(

My initial reservations were that I was using too much space with these comments, but I see that my problem was too little, not too much! I'll go through and create a fix-up using your feedback as a guide...

practicalswift · 2018-08-30T23:20:06Z

lightningd/lightningd.c

+	/*~ There's always a battle between what a constructor like this
+	 * should do, and what should be added later by the caller.  In
+	 * general, because we use valgrind heavily for testing, we prefer not
+	 * to intialize unused fields which we expect the caller to set:


Typo: "intialize"

practicalswift · 2018-08-30T23:20:20Z

lightningd/lightningd.c


-	/* Get the blockheight we are currently at, UINT32_MAX is used to signal
+	/*~ Get the blockheight we are currently at, UINT32_MAX is used to signal
 	 * an unitialized wallet and that we should start off of bitcoind's


Typo: "unitialized"

practicalswift · 2018-08-30T23:21:55Z

lightningd/lightningd.c

-
+/*~ The core lightning object: it's passed everywhere, and is basically a
+ * global variable.  This new_xxx pattern is something we'll see often:
+ * it allocates and initializes a new structure, using *tal*, the heirarchitcal


Typo: "heirarchitcal"

Shall we call lightningd the context object, or maybe root of the entire program?

practicalswift · 2018-08-30T23:22:28Z

lightningd/lightningd.c

+	 * elements, which can be accessed with tal_count() (or tal_bytelen()
+	 * for raw bytecount).  It's common for simple arrays to use
+	 * tal_resize(), which is a typesafe realloc function, but as all
+	 * talocations need a parent, we start with an empty array rather than


Typo: "talocations"

practicalswift · 2018-08-31T06:48:33Z

lightningd/lightningd.c

+
+	/*~ These are hash tables of incoming and outgoing HTLCs (contracts),
+	 * defined as `struct htlc_in` and `struct htlc_out`in htlc_end.h.
+	 * The hash tables are declared ther using the very ugly


Typo: "ther"

cdecker · 2018-09-02T09:24:02Z

lightningd/lightningd.c

+ *
+ * Comments beginning with a ~ (like this one!) are part of our shared
+ * adventure through the source, so they're more meta than normal code
+ * comments, and mean to be read in a certain order.


mean -> meant

cdecker · 2018-09-02T09:28:39Z

lightningd/lightningd.c

+	 * 'tallocated' off it are also freed.  In this case, freeing 'ctx'
+	 * will free 'ld'.
+	 *
+	 * It's incredibly useful for grouping object lifetimes, as we'll see.


"For the technically inclined: tal allocations usually build a tree, and tal_freeing any node in the tree will result in the entire subtree rooted at that node to be freed.

Be careful though that a any cycle in the allocation graph may end up causing an infinite loop."

Also for my personal understanding: the allocations are freed in post-order right? (first children, then the parent).

It won't loop on cycles. Freeing is done in two phases: object is marked destroyed, destructor called, then recurse into children, then finally free(object). So destructors are pre-order, deallocation is post-order.

It's common to want to access parents in a destructor (eg. delete from its linked list), so deallocation has to be deferred.

cdecker · 2018-09-02T17:25:03Z

lightningd/lightningd.c

 	struct ext_key ext;
+	/*~ Note the use of ccan/short_types u64 rather than uint64_t.


Why do I always imagine the creepy voice from Portal when the comments speak in first person? :-)

cdecker · 2018-09-02T17:39:58Z

lightningd/lightningd.c

 	if (chdir(cwd) != 0)
 		fatal("Could not return to directory %s: %s",
 		      cwd, strerror(errno));

 	db_reopen_after_fork(ld->wallet->db);
+
+	/*~ Why not allocate cwd off tmpctx?  Probably because this code predates
+	 * tmpctx.  So we free manually here. */


I thought the it's more because the global tmpctx is an event loop concept and this is outside of the loop.

Yeah, but it's before the loop; we tend to allow that elsewhere...

Signed-off-by: Mark Beckwith <wythe@intrig.com>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We're a daemon. They're subdaemons. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We already have access via the ld object, and we initialized this one twice anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We have a transaction anyway, and it's simpler. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2018-09-03T01:07:42Z

Despite lack of (concrete) acks, I'm going to merge now; it's going to cause rebase hell to delay further, and I'm already itching to post parts II and III :)

Also, wallet has no business wiring up HTLCs; move that code to peer_htlcs.c. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@renepickhardt

@renepickhardt: why is it actually lightningd.c with a d but hsm.c without d ? And delete unused gossipd/gossip.h. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Code changes: 1. Expose daemon_poll() so lightningd can call it directly, which avoids us having store a global and document it. 2. Remove the (undocumented, unused, forgotten) --rpc-file="" option to disable JSON RPC. 3. Move the ickiness of finding the executable path into subd.c, so it doesn't distract from lightningd.c overview. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@renepickhardt

Documentation changes: 1. Lots of extra detail suggested by @renepickhardt. 2. typo fixes from @practicalswift. 3. A section on 'const' usage. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@practicalswift

Suggested-by: @practicalswift and @cdecker. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

jb55 · 2018-09-03T18:06:23Z

Knuth would be proud. Thanks rusty!

rustyrussell requested review from cdecker, ZmnSCPxj, practicalswift, jb55, renepickhardt and SimonVrouwe August 30, 2018 10:22

rustyrussell self-assigned this Aug 30, 2018

rustyrussell added enhancement docs good first issue good for onboarding labels Aug 30, 2018

renepickhardt reviewed Aug 30, 2018

View reviewed changes

wythe added a commit to wythe/lightning that referenced this pull request Aug 30, 2018

param: updated comments in the spirit of ElementsProject#1899

d58bb4a

Signed-off-by: Mark Beckwith <wythe@intrig.com>

practicalswift reviewed Aug 30, 2018

View reviewed changes

practicalswift reviewed Aug 31, 2018

View reviewed changes

cdecker reviewed Sep 2, 2018

View reviewed changes

rustyrussell pushed a commit that referenced this pull request Sep 3, 2018

param: updated comments in the spirit of #1899

4ad16b6

Signed-off-by: Mark Beckwith <wythe@intrig.com>

rustyrussell added 5 commits September 3, 2018 10:11

lightningd: remove unnecessary globals.

b49d8e2

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: rename 'daemons' to 'subdaemons'.

7bc7f54

We're a daemon. They're subdaemons. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: remove gratuitous SIG_IGN: daemon_setup() does it already.

daf80f8

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

chaintopology: remove redundant wallet pointer.

a59dead

We already have access via the ld object, and we initialized this one twice anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

wallet: do wallet_invoice init during preparation.

d62c6c4

We have a transaction anyway, and it's simpler. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: move channel/peer/htlc load into own function.

2ea90f2

Also, wallet has no business wiring up HTLCs; move that code to peer_htlcs.c. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: inline overview documentation (part 1 of 8).

8d6c327

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/docs branch 2 times, most recently from fa6fc14 to a66afbf Compare September 3, 2018 03:00

rustyrussell added 4 commits September 3, 2018 13:10

Put the 'd' back in the daemons.

c26b1cc

@renepickhardt: why is it actually lightningd.c with a d but hsm.c without d ? And delete unused gossipd/gossip.h. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

More documentation changes.

078c788

Documentation changes: 1. Lots of extra detail suggested by @renepickhardt. 2. typo fixes from @practicalswift. 3. A section on 'const' usage. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: more comment fixes.

20208c8

Suggested-by: @practicalswift and @cdecker. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/docs branch from a66afbf to 20208c8 Compare September 3, 2018 03:41

rustyrussell merged commit f2e0857 into ElementsProject:master Sep 3, 2018

rustyrussell deleted the guilt/docs branch September 3, 2018 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Great Code Documentation Effort, Part I #1899

The Great Code Documentation Effort, Part I #1899

rustyrussell commented Aug 30, 2018

renepickhardt left a comment

renepickhardt Aug 30, 2018

rustyrussell Aug 30, 2018

renepickhardt Aug 30, 2018

renepickhardt Aug 30, 2018

rustyrussell Aug 30, 2018

renepickhardt Aug 30, 2018

renepickhardt Aug 30, 2018

rustyrussell Aug 30, 2018

rustyrussell Aug 31, 2018

renepickhardt Aug 30, 2018

renepickhardt Aug 30, 2018

rustyrussell Aug 31, 2018

renepickhardt Aug 30, 2018

cdecker Sep 3, 2018

renepickhardt Aug 30, 2018

renepickhardt Aug 30, 2018

renepickhardt Aug 30, 2018

rustyrussell Aug 31, 2018

rustyrussell commented Aug 30, 2018

practicalswift Aug 30, 2018

practicalswift Aug 30, 2018

practicalswift Aug 30, 2018

cdecker Sep 2, 2018

practicalswift Aug 30, 2018

practicalswift Aug 31, 2018

cdecker Sep 2, 2018

cdecker Sep 2, 2018

rustyrussell Sep 3, 2018

cdecker Sep 2, 2018

cdecker Sep 2, 2018

rustyrussell Sep 3, 2018

rustyrussell commented Sep 3, 2018

jb55 commented Sep 3, 2018


		/* Initialize wallet, now that we are in the correct directory */
		/*~ Our "wallet" code really wraps the db, which is more than a simple


		/* Create RPC socket (if any) */
		/~ Create RPC socket (if any): now we can talk to clients. /

		crashlog = ld->log;

		/~ The root of every backtrace (almost). /

		struct ext_key ext;
		/*~ Note the use of ccan/short_types u64 rather than uint64_t.

The Great Code Documentation Effort, Part I #1899

The Great Code Documentation Effort, Part I #1899

Conversation

rustyrussell commented Aug 30, 2018

renepickhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Sep 3, 2018

jb55 commented Sep 3, 2018