create-gossipstore.c can read scid -> satoshis csv file. The csv is i… #5

nettijoe96 · 2019-04-01T17:54:00Z

…n the format scid ,satoshis where there is a black space after scid. Made a header file that contains a struct. Modified makefile. Added cmdline arg --scidfile /path/to/csv and made the constant capacity command optional. create-gossipstore prints stats at the end.

Case 5 in the Tor documentation currently states that if you use `--bind-addr=autotor:127.0.0.1:9051`, you can get your onion address by running `lightning-cli getinfo`. I have not found that to be the case; with that flag no onion address will be generated. On the other hand, if `--addr=autotor:127.0.0.1:9051` is used instead, an onion address is generated and `lightning-cli getinfo` behaves as the docs say.

rustyrussell

OK, since this is your first significant change to the clightning codebase, I've gone through with a fine tooth comb and pointed out everything I could think of that I would have done differently. Seems like a lot there, but really, the code works fine as is, and I've cherry picked it for my tree already, but I think it's useful feedback.

rustyrussell · 2019-04-05T00:00:37Z

devtools/create-gossipstore.c

 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <wire/gen_peer_wire.h>

+
+


Trivial: these two extra lines are unnecessary.

rustyrussell · 2019-04-05T00:01:30Z

devtools/create-gossipstore.c

 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <wire/gen_peer_wire.h>

+
+
+struct scidsat * load_scid_file(FILE * scidfd) 


trivial: spacing after * is unusual.
Also, functions should be static by default; the only exception is when functions need to be accessed by other C files.

rustyrussell · 2019-04-05T00:02:01Z

devtools/create-gossipstore.c

+struct scidsat * load_scid_file(FILE * scidfd) 
+{
+        int n;
+        fscanf(scidfd, "%d\n", &n);	


Generally, you should always validate input. In this case, it means checking that fscanf returns 1.

rustyrussell · 2019-04-05T00:02:23Z

devtools/create-gossipstore.c

+        int n;
+        fscanf(scidfd, "%d\n", &n);	
+	char title[16];
+        fscanf(scidfd, "%s\n", title);	


This can overflow, so you want "%16s" to avoid buffer overrun.

rustyrussell · 2019-04-05T00:02:41Z

devtools/create-gossipstore.c

+{
+        int n;
+        fscanf(scidfd, "%d\n", &n);	
+	char title[16];


Trivial: we usually declare all vars at the top of the function or block.

rustyrussell · 2019-04-05T00:09:09Z

devtools/create-gossipstore.c

+                scids = load_scid_file(scidfd);
+	        fclose(scidfd);
+	}
+	else if (csat) {


Trivial: else belongs on same line as {

rustyrussell · 2019-04-05T00:09:55Z

devtools/create-gossipstore.c

+		}
+	}
+	else {
+		err(1, "must contain either --sat xor --scidfile");


But your check doesn't validate this is true. You probably want to check in the other branches that only one is set.

rustyrussell · 2019-04-05T00:11:34Z

devtools/create-gossipstore.c


 	if (infile) {
 		infd = open(infile, O_RDONLY);
 		if (infd < 0)
 			err(1, "opening %s", infile);
-	} else
-		infd = STDIN_FILENO;
+	}


This removal seems weird; if we don't specify infile infd will be undefined. Standard UNIX tools use stdin/stdout by default.

rustyrussell · 2019-04-05T00:12:09Z

devtools/create-gossipstore.c

@@ -71,13 +110,20 @@ int main(int argc, char *argv[])

 		switch (fromwire_peektype(inmsg)) {
 		case WIRE_CHANNEL_ANNOUNCEMENT:
+			if (scidfile) {
+				sat = scids[scidi].sat;
+				scidi += 1;


Trivial: scidi++ is more idiomatic here.

rustyrussell · 2019-04-05T00:13:49Z

devtools/create-gossipstore.h

+        struct amount_sat sat;
+} scidsat;
+
+struct scidsat * load_scid_file(FILE * scidfd);


We don't do a source check on header files in the dev directory, which is a mistake. Generally, they should
be protected against multiple inclusions with #ifndef guards. But importantly, I don't think this needs to be in a header at all does it?

A backtrace which makes it much easier to figure out what's happening. Signed-off-by: Rusty Russell <[email protected]>

…stream of gossip. The gossip is expected to be in format: 16-bit-big-endian-length [gossip message] Signed-off-by: Rusty Russell <[email protected]>

For giant nodes, it seems we spend a lot of time memmoving this array. Normally we'd go for a linked list, but that's actually hard: each channel has two nodes, so needs two embedded list pointers, and when iterating there's no good way to figure out which embedded pointer we'd be using. So we (ab)use htable; we don't really need an index, but it's good for cache-friendly iteration (our main operation). We can actually change to a hybrid later to avoid the extra allocation for small nodes. Signed-off-by: Rusty Russell <[email protected]>

This is useful for canned data, such as the million channels project. Signed-off-by: Rusty Russell <[email protected]>

We push a huge msg for listchannels with the million-channels project. We need to fix that, but this works around it so we can benchmark. Signed-off-by: Rusty Russell <[email protected]>

This lets us benchmark without a valid blockchain. Signed-off-by: Rusty Russell <[email protected]>

@cdecker

Suggested-by: @cdecker Signed-off-by: Rusty Russell <[email protected]>

…write. Signed-off-by: Rusty Russell <[email protected]>

… restarts Signed-off-by: Rusty Russell <[email protected]>

This causes natural batching, rather than on every little addition of JSON formatting. Before, to listchannels 100,000 channels took 82.48 seconds, after 6.82 seconds. Signed-off-by: Rusty Russell <[email protected]>

Adding a giant IO message simply causes it to be pruned immediately, so truncate it if it's more than 1/64 the max size. Signed-off-by: Rusty Russell <[email protected]>

…ced. Either private or simply not enough confirms. They would have been added on reconnect, but that's not ideal. Signed-off-by: Rusty Russell <[email protected]>

Otherwise we'd forget them on restart, again. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

…hannels project Outputs CSV. We add some stats for load times in developer mode, so we can easily read them out. peer_read_all_sec doesn't work, since we seem to reject about half the updates for having bad signatures. It's also very slow... routing fails, for unknown reasons, so that failure is ignored in routing_sec. Results from 5 runs, min-max(mean +/- stddev): store_load_msec,vsz_kb,store_rewrite_sec,listnodes_sec,listchannels_sec,routing_sec,peer_write_all_sec 39275-44779(40466.8+/-2.2e+03),2899248,41.010000-44.970000(41.972+/-1.5),2.280000-2.350000(2.304+/-0.025),49.770000-63.390000(59.178+/-5),33.310000-34.260000(33.62+/-0.35),42.100000-44.080000(43.082+/-0.67) Signed-off-by: Rusty Russell <[email protected]>

@niftynei

…llion channels project Suggested-by: @niftynei

…llion channels project MCP filename change.

Signed-off-by: Rusty Russell <[email protected]>

…in the format scid ,satoshis where there is a black space after scid. Made a header file that contains a struct. Modified makefile. Added cmdline arg --scidfile /path/to/csv and made the constant capacity command optional. create-gossipstore prints stats at the end.

Signed-off-by: Rusty Russell <[email protected]>

…ets. eg. for running under valgrind. Signed-off-by: Rusty Russell <[email protected]>

Direct leak of 64 byte(s) in 1 object(s) allocated from: #0 0x7f4dc279163e in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c63e) #1 0x564ee8a24bb1 in htable_default_alloc ccan/ccan/htable/htable.c:19 #2 0x564ee8a2551b in double_table ccan/ccan/htable/htable.c:226 #3 0x564ee8a259e5 in htable_add_ ccan/ccan/htable/htable.c:331 #4 0x564ee89a5300 in block_map_add lightningd/chaintopology.h:83 #5 0x564ee89a6ece in add_tip lightningd/chaintopology.c:626 #6 0x564ee89a72c3 in have_new_block lightningd/chaintopology.c:694 #7 0x564ee89a3ab0 in process_rawblock lightningd/bitcoind.c:466 #8 0x564ee89a2fb4 in bcli_finished lightningd/bitcoind.c:214 #9 0x564ee8a284d6 in destroy_conn ccan/ccan/io/poll.c:244 #10 0x564ee8a284f6 in destroy_conn_close_fd ccan/ccan/io/poll.c:250 #11 0x564ee8a34a0d in notify ccan/ccan/tal/tal.c:235 #12 0x564ee8a34efc in del_tree ccan/ccan/tal/tal.c:397 #13 0x564ee8a35288 in tal_free ccan/ccan/tal/tal.c:481 #14 0x564ee8a26cf5 in io_close ccan/ccan/io/io.c:450 ElementsProject#15 0x564ee8a28c11 in io_loop ccan/ccan/io/poll.c:449 ElementsProject#16 0x564ee89b3c3b in io_loop_with_timers lightningd/io_loop_with_timers.c:24 ElementsProject#17 0x564ee89ba540 in main lightningd/lightningd.c:822 ElementsProject#18 0x7f4dc2143b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Signed-off-by: Rusty Russell <[email protected]>

Direct leak of 32 byte(s) in 1 object(s) allocated from: #0 0x7f7678ee863e in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c63e) #1 0x55f8c7b0fce5 in htable_default_alloc ccan/ccan/htable/htable.c:19 #2 0x55f8c7b1064f in double_table ccan/ccan/htable/htable.c:226 #3 0x55f8c7b10b19 in htable_add_ ccan/ccan/htable/htable.c:331 #4 0x55f8c7afac63 in scriptpubkeyset_add wallet/txfilter.c:30 #5 0x55f8c7afafce in txfilter_add_scriptpubkey wallet/txfilter.c:77 #6 0x55f8c7afb05f in txfilter_add_derkey wallet/txfilter.c:91 #7 0x55f8c7aa4d67 in init_txfilter lightningd/lightningd.c:482 #8 0x55f8c7aa52d8 in main lightningd/lightningd.c:721 #9 0x7f767889ab6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x7f05f389563e in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c63e) #1 0x55cac1e6bc99 in htable_default_alloc ccan/ccan/htable/htable.c:19 #2 0x55cac1e6c603 in double_table ccan/ccan/htable/htable.c:226 #3 0x55cac1e6cacd in htable_add_ ccan/ccan/htable/htable.c:331 #4 0x55cac1e56e48 in outpointset_add wallet/txfilter.c:61 #5 0x55cac1e57162 in outpointfilter_add wallet/txfilter.c:116 #6 0x55cac1e5ea3a in wallet_utxoset_add wallet/wallet.c:2365 #7 0x55cac1deddc2 in topo_add_utxos lightningd/chaintopology.c:603 #8 0x55cac1dedeac in add_tip lightningd/chaintopology.c:620 #9 0x55cac1dee2de in have_new_block lightningd/chaintopology.c:694 #10 0x55cac1deaab0 in process_rawblock lightningd/bitcoind.c:466 #11 0x55cac1de9fb4 in bcli_finished lightningd/bitcoind.c:214 #12 0x55cac1e6f5be in destroy_conn ccan/ccan/io/poll.c:244 #13 0x55cac1e6f5de in destroy_conn_close_fd ccan/ccan/io/poll.c:250 #14 0x55cac1e7baf5 in notify ccan/ccan/tal/tal.c:235 ElementsProject#15 0x55cac1e7bfe4 in del_tree ccan/ccan/tal/tal.c:397 ElementsProject#16 0x55cac1e7c370 in tal_free ccan/ccan/tal/tal.c:481 ElementsProject#17 0x55cac1e6dddd in io_close ccan/ccan/io/io.c:450 ElementsProject#18 0x55cac1e6fcf9 in io_loop ccan/ccan/io/poll.c:449 ElementsProject#19 0x55cac1dfac66 in io_loop_with_timers lightningd/io_loop_with_timers.c:24 ElementsProject#20 0x55cac1e0156b in main lightningd/lightningd.c:822 ElementsProject#21 0x7f05f3247b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Signed-off-by: Rusty Russell <[email protected]>

Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x7ff02889063e in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c63e) #1 0x555ce2ad8d2e in htable_default_alloc ccan/ccan/htable/htable.c:19 #2 0x555ce2ad9698 in double_table ccan/ccan/htable/htable.c:226 #3 0x555ce2ad9b62 in htable_add_ ccan/ccan/htable/htable.c:331 #4 0x555ce2a638e4 in htlc_in_map_add lightningd/htlc_end.h:113 #5 0x555ce2a63beb in connect_htlc_in lightningd/htlc_end.c:39 #6 0x555ce2a85cbc in channel_added_their_htlc lightningd/peer_htlcs.c:1382 #7 0x555ce2a860e1 in peer_got_commitsig lightningd/peer_htlcs.c:1466 #8 0x555ce2a5db04 in channel_msg lightningd/channel_control.c:228 #9 0x555ce2a8d393 in sd_msg_read lightningd/subd.c:474 #10 0x555ce2ada157 in next_plan ccan/ccan/io/io.c:59 #11 0x555ce2adacd4 in do_plan ccan/ccan/io/io.c:407 #12 0x555ce2adad12 in io_ready ccan/ccan/io/io.c:417 #13 0x555ce2adcd67 in io_loop ccan/ccan/io/poll.c:445 #14 0x555ce2a67c66 in io_loop_with_timers lightningd/io_loop_with_timers.c:24 ElementsProject#15 0x555ce2a6e56b in main lightningd/lightningd.c:822 ElementsProject#16 0x7ff028242b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Signed-off-by: Rusty Russell <[email protected]>

Direct leak of 1024 byte(s) in 2 object(s) allocated from: #0 0x7f4c84ce4448 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c448) #1 0x55d11b782c96 in timer_default_alloc ccan/ccan/timer/timer.c:16 #2 0x55d11b7832b7 in add_level ccan/ccan/timer/timer.c:166 #3 0x55d11b783864 in timer_fast_forward ccan/ccan/timer/timer.c:334 #4 0x55d11b78396a in timers_expire ccan/ccan/timer/timer.c:359 #5 0x55d11b774993 in io_loop ccan/ccan/io/poll.c:395 #6 0x55d11b72322f in plugins_init lightningd/plugin.c:1013 #7 0x55d11b7060ea in main lightningd/lightningd.c:664 #8 0x7f4c84696b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Signed-off-by: Rusty Russell <[email protected]>

Indirect leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f4c84ce4448 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10c448) #1 0x55d11b77d270 in strmap_add_ ccan/ccan/strmap/strmap.c:90 #2 0x55d11b704603 in command_set_usage lightningd/jsonrpc.c:891 #3 0x55d11b733cb5 in param common/param.c:295 #4 0x55d11b6f7b37 in json_connect lightningd/connect_control.c:96 #5 0x55d11b7042ef in setup_command_usage lightningd/jsonrpc.c:841 #6 0x55d11b70443b in jsonrpc_command_add_perm lightningd/jsonrpc.c:863 #7 0x55d11b704533 in jsonrpc_setup lightningd/jsonrpc.c:876 #8 0x55d11b705695 in new_lightningd lightningd/lightningd.c:210 #9 0x55d11b706062 in main lightningd/lightningd.c:644 #10 0x7f4c84696b6a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26b6a) Signed-off-by: Rusty Russell <[email protected]>