DB: Store the signatures of channel announcement sent from remote peer into DB #2619

trueptolemy · 2019-05-05T12:55:39Z

Fixes #2409
Here I add a new wire type:
channel_got_announcement (1017)

When Channeld receives the remote channel announcement signatures, it will generate a msg1 like:
WIRE_CHANNEL_GOT_ANNOUNCEMENT + remote_ann_node_sig + remote_ann_bitcoin_sig
and send this msg to Master. (After sending signatures, Channeld will be holding until receive the reply from Master.)
Master resolves msg and stores the signatures into DB.

Like issue #2409 , the adventage is when we reenable our channel, you can init it with the remote announcement signatures from DB directly other than wasting your time to wait for remote peer's announce reply.

SimonVrouwe · 2019-05-06T14:36:48Z

channeld/channeld.c

+			    	    type_to_string(peer, struct short_channel_id,
+						   remote_scid));
+
+		/* make sure new node signature and bitcoin signature equal


Shouldn't we just check that the signature is valid? Signatures change when re-signing (using different k value).

Thank you! You're right, and there shouldn't compare with the old sigs if the remote generate new sigs. Gossipd will verify the announcement, so I think we'd better update the new sigs here.

SimonVrouwe · 2019-05-06T14:53:51Z

I think it would be nice to have some sort of (py)test to test the functionality. Also to get more clarity on what kind of behavior we actually want/expect.

Related questions:

Does anybody know which LND version(s) cause this issue?
Could we use lightning-integration to test?

trueptolemy · 2019-05-06T17:01:42Z

Also to get more clarity on what kind of behavior we actually want/expect.

@SimonVrouwe I think the point is we want to get into the phase of announcement from the phase of waiting for remote signatures more quickly.
We care how to get the valid signature faster, and we have 2 choices: waiting for remote reply or using the old signatures(rejection for the new sigs is a bad idea:( ). In the origin code, we just check the validity of signatures and don't consider if signatures are different with the ones received before.
In this PR, local peer still send local sigs to the remote and receives the remote reply. It commucates with others like original.
But the difference is : it use the old signatures directly to announce although it will also receive the remote reply with remote signatures(the old or the new). In my understanding, the announcement is ok iff the signatures are all valid.

trueptolemy · 2019-05-06T18:25:34Z

@SimonVrouwe Sorry. I find another change for the current PR: when we reenable the channel, we directly announce with the old sigs meanwhile sending remote peer our sigs. So we will announce once again when we receive the announcement reply.
Sending our own sigs to the remote peer is necessary. This leads directly to the issue that how to handle the reply from remote peer. Now I handle this special reply as usual, and also ask Channeld to announce.
Another alternative way is to ignore this reply.
I think the formor way(which was achived in this PR) seems more reasonable, but now we meet two announcement almost at the same time.
lightning-integration is needed though it doesn't matter for clightning.
Thank you very mush :)

trueptolemy · 2019-05-06T18:36:26Z

Sending our own sigs to the remote peer is necessary. This leads directly to the issue that how to handle the reply from remote peer. Now I handle this special reply as usual, and also ask Channeld to announce.
Another alternative way is to ignore this reply.

@cdecker hi, what do you think of these two ways?

cdecker

Quite a nice pull request, congratulation 👍 I went through and I found a
few pieces that are a bit strange though (some are a bit pedantic I'll admit
😉). In particular there are a few places where you are over-checking
(signatures may change, scids may also change), and the last commit is a bit
too much sync that was likely caused by trying to only send the signatures
once. Please remove the reply in the last commit, and relax some of the checks.

Minor comments:

WIRE_CHANNEL_GOT_ANNOUNCEMENT is introduced in the first commit, but never
handled. This breaks rebaseability (each commit must compile on its own).
The same applies to commit 71dcf4b which
changes the signature of new_channel but doesn't fix the callers.

channeld/channeld.c

cdecker · 2019-05-10T10:01:58Z

lightningd/channel.c

-			    const u8 *remote_upfront_shutdown_script)
+			    const u8 *remote_upfront_shutdown_script,
+				secp256k1_ecdsa_signature *remote_ann_node_sig,
+				secp256k1_ecdsa_signature *remote_ann_bitcoin_sig)


@rustyrussell should these be marked as TAKES if we tal_steal them below? I can never remember the semantics of TAKES 😉

I find we marked as TAKES if we tal_dup, and there's some similar cases in this function:
we didn't marked scid, last_tx, last_sent_commit as TAKES and we tal_steal them below,
but we marked transient_billboard as TAKES because we tal_strdup it below.

Rusty replied me that tal_steal doesn't need to be marked as TAKES, and tal_dup can be marked :)

cdecker · 2019-05-10T10:05:48Z

wallet/db.c

-	{ "ALTER TABLE forwarded_payments ADD resolved_time INTEGER", NULL },
+	{ "ALTER TABLE channel_htlcs ADD received_time INTEGER;", NULL },
+	{ "ALTER TABLE forwarded_payments ADD received_time INTEGER;", NULL },
+	{ "ALTER TABLE forwarded_payments ADD resolved_time INTEGER;", NULL },


Please don't modify existing migrations. While this change may be harmless cosmetics, changing the migrations may end up corrupting deployed databases.

Sorry, I'll take care, too

channeld/channeld.c

cdecker · 2019-05-10T10:14:12Z

lightningd/channel_control.c

+	secp256k1_ecdsa_signature *remote_ann_bitcoin_sig;
+
+	remote_ann_node_sig = tal(tmpctx, secp256k1_ecdsa_signature);
+	remote_ann_bitcoin_sig = tal(tmpctx, secp256k1_ecdsa_signature);


You can allocate them directly off of channel if you tal_free them on error as well (which should be rare anyway).

Yes, now I don't allocate them directly.

cdecker · 2019-05-10T10:16:35Z

lightningd/channel.h

+
+	/* channel announcement sigs info from remote peer*/
+	secp256k1_ecdsa_signature *remote_ann_node_sig;
+	secp256k1_ecdsa_signature *remote_ann_bitcoin_sig;


Why are we keeping these in memory at all? We could just as well load them on demand when a new channeld gets started.

Now I replace it by a new load function, which is only called when we generate a channel_init message.

channeld/channel_wire.csv

cdecker · 2019-05-10T10:19:16Z

lightningd/channel_control.c

+	 * In this case, master may receive announcement more than once!
+	 */
+	tal_free(channel->remote_ann_node_sig);
+	tal_free(channel->remote_ann_bitcoin_sig);


If the master was shut down we're dead as well. No need to handle that case here.

Yes, I'll remove it.

trueptolemy · 2019-05-10T16:06:29Z

@cdecker Sincerely thank you! I'll correct these days. Many points, detials to learn and I hope I can do better next time :)

trueptolemy · 2019-05-19T13:39:39Z

Hi, @cdecker I've correct the code. What do you think about it now? :-)

cdecker

Excellent pull request @trueptolemy, I went through with a fine-toothed comb and commented all the minor things that came to mind.

cdecker · 2019-05-27T11:12:24Z

wallet/wallet.c

@@ -595,6 +595,42 @@ wallet_htlc_sigs_load(const tal_t *ctx, struct wallet *w, u64 channelid)
 	return htlc_sigs;
 }

+bool wallet_remote_ann_sigs_load(struct wallet *w, u64 id,
+				 secp256k1_ecdsa_signature **remote_ann_node_sig,
+				 secp256k1_ecdsa_signature **remote_ann_bitcoin_sig)


If we are passing a pointer-to-pointer-of-sugnature we should also be in charge of allocating the signature, otherwise we need to allocate outside of the call, keep an alias, call the function and then free the alias in case of a failure. If however we allocate internally then the pointer will get populated correctly.

Thank you! Now I manage the allocation in this func!

cdecker · 2019-05-27T11:13:28Z

wallet/wallet.c

+
+	if (sqlite3_column_type(stmt, 0) != SQLITE_NULL) {
+		if (!sqlite3_column_signature(stmt, 0, *remote_ann_node_sig)) {
+			db_stmt_done(stmt);


In other parts of the code we use a fail: label to clear and return if we have multiple similar failure paths in a single function.

Yes, now I use fail:

wallet/wallet.c

cdecker · 2019-05-27T11:14:14Z

wallet/wallet.c

@@ -1086,6 +1141,16 @@ void wallet_channel_save(struct wallet *w, struct channel *chan)
 		sqlite3_bind_null(stmt, 1);
 	sqlite3_bind_int64(stmt, 2, chan->dbid);
 	db_exec_prepared(w->db, stmt);
+
+	/* save announcement sigs only when we ge announcement*/


ge -> get

Also these fields ought to be initialized to null anyway in the DB, so no need to do this additional initialization.

cdecker · 2019-05-27T11:16:14Z

wallet/wallet.h

+ * @w: wallet containing the channel
+ * @id: channel database id
+ * @remote_ann_node_sig: location to load remote_ann_node_sig to
+ * @remote_ann_bitcoin_sig: location to load remote_ann_bitcoin_sig to


Like mentioned above, with this signature I would have expected the function to allocate the signatures on the heap. But in that case we're missing a ctx to allocate from.

Thank you! Now I add it.

cdecker · 2019-05-27T11:18:40Z

lightningd/channel_control.c

+	if(!(channel->channel_flags & CHANNEL_FLAGS_ANNOUNCE_CHANNEL)) {
+		channel_internal_error(channel,
+				       "No ANNOUNCE flag, no announcemnt signatures! %s",
+				       tal_hex(tmpctx, msg));


This check, while correct, may be a bit aggressive in the current network and might result in a lot of failures (nodes usually just spray you with the signatures without checking the announcement flags). We can still enforce that we don't announce by simply not sending sigs ourselves.

Please don't fail the channel over such a minor offence, but log it and we can tighten this later.

Thank you! Now I remove this check

cdecker · 2019-05-27T11:21:14Z

channeld/channeld.c

+		peer->have_sigs[REMOTE] = true;
+
+		tal_steal(tmpctx, remote_ann_node_sig);
+		tal_steal(tmpctx, remote_ann_bitcoin_sig);


Why are you stealing these onto tmpctx? You could also pass the pointers to peer->announcement_node_sigs[REMOTE] and peer->announcement_bitcoin_sigs[REMOTE] into the fromwire_channel_init call directly, which avoids having these temp variables and having to deal with intermediate allocations and ownership.

Yes, I find these tal_steal are extra. But I think fromwire_channel_init needs these 2 pointer, because they can handle NULL sigs:

channel_init,,remote_ann_node_sig,?secp256k1_ecdsa_signature channel_init,,remote_ann_bitcoin_sig,?secp256k1_ecdsa_signature

emm, after deleting tal_steal ,Travis CI tell me memory leak. So I tal_free these directly.(I find other variables used in fromwire_channel_init are freed directly, so I also do it.)

cdecker · 2019-05-27T11:22:43Z

lightningd/channel_control.c

+	secp256k1_ecdsa_signature *remote_ann_node_sig, *remote_ann_bitcoin_sig;
+
+	remote_ann_node_sig = tal(tmpctx, secp256k1_ecdsa_signature);
+	remote_ann_bitcoin_sig = tal(tmpctx, secp256k1_ecdsa_signature);


This is the weird ownership I was talking about before. Please have wallet_remote_ann_sigs_load manage allocations. The only reason we aren't producing a memory leak here is that tmpctx and all child allocations is cleared in the io_loop.

Thank you so much! Now I pass ctx to wallet_remote_ann_sigs_load and let it manage allocations.

cdecker · 2019-05-28T14:46:33Z

wallet/wallet.c

+
+fail:
+	*remote_ann_node_sig = tal_free(*remote_ann_node_sig);
+	*remote_ann_bitcoin_sig = tal_free(*remote_ann_bitcoin_sig);


This can easily lead to a double-free or freeing a random memory location since we do not allocate in all cases. It'd be better to check both fields for SQLITE_NULL at the top, and set the out-args to NULL and return immediately if either is SQLITE_NULL. If both are not SQLITE_NULL initialized both using tal and then goto fail; if either of the sqlite3_column_signature calls fails. That way we either initialize all or none, and free only when both are actually initialized.

Thank you! What you said is better, kinds of cases make sense easily.

1. Add the fields of remote channel announcement signatures into TABLE channels 2. Add the related function

Channeld sends announcement signatures to Master by this message. When Channeld receive a new channel announcement msg, (After channel locking)it will sends announcement signatures to Master by this message.

1. Add remote_ann_node_sigs and remote_bitcoin_sigs fields in channel_init message; 2. Master add announcement signatures into channel_init message, and send this message to Channeld. Channeld will initial the channel with this signatures when it reenables the channel.

cdecker · 2019-05-29T08:14:08Z

ACK e884bc6

trueptolemy requested a review from cdecker as a code owner May 5, 2019 12:55

SimonVrouwe reviewed May 6, 2019

View reviewed changes

trueptolemy force-pushed the issue-2409-new branch from eec96b9 to 55cca59 Compare May 6, 2019 18:06

cdecker requested changes May 10, 2019

View reviewed changes

trueptolemy force-pushed the issue-2409-new branch 17 times, most recently from 983414d to 43e47fb Compare May 19, 2019 11:08

trueptolemy force-pushed the issue-2409-new branch from 43e47fb to 08a5721 Compare May 21, 2019 15:15

cdecker requested changes May 27, 2019

View reviewed changes

jb55 mentioned this pull request May 27, 2019

Store the announcement_signature in the database to work around some impls not sending it on reconnect #2409

Closed

trueptolemy force-pushed the issue-2409-new branch 6 times, most recently from ab2371d to 4bf51a3 Compare May 28, 2019 14:42

cdecker reviewed May 28, 2019

View reviewed changes

trueptolemy force-pushed the issue-2409-new branch from 4bf51a3 to a8b927a Compare May 28, 2019 16:35

trueptolemy added 4 commits May 29, 2019 01:04

DB: Store the remote channel announcement signatures into DB

c5c74b6

1. Add the fields of remote channel announcement signatures into TABLE channels 2. Add the related function

run-wallet: Add the check of case that channel received ann_sigs

a8e672e

Channeld: Add new wire type:channel_got_announcement

44195a2

Channeld sends announcement signatures to Master by this message. When Channeld receive a new channel announcement msg, (After channel locking)it will sends announcement signatures to Master by this message.

trueptolemy force-pushed the issue-2409-new branch from a8b927a to e884bc6 Compare May 28, 2019 17:04

cdecker approved these changes May 29, 2019

View reviewed changes

cdecker merged commit 1141243 into ElementsProject:master May 29, 2019

SimonVrouwe mentioned this pull request Jun 4, 2019

pytest: add test store announcement_signatures, cleanups and a small bugfix #2706

Merged

DB: Store the signatures of channel announcement sent from remote peer into DB #2619

DB: Store the signatures of channel announcement sent from remote peer into DB #2619

Conversation

trueptolemy commented May 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonVrouwe commented May 6, 2019

trueptolemy commented May 6, 2019 • edited Loading

trueptolemy commented May 6, 2019 • edited Loading

trueptolemy commented May 6, 2019

cdecker 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

trueptolemy commented May 10, 2019

trueptolemy commented May 19, 2019

cdecker 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

trueptolemy May 28, 2019 • edited Loading

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

cdecker commented May 29, 2019

trueptolemy commented May 5, 2019 •

edited

Loading

trueptolemy commented May 6, 2019 •

edited

Loading

trueptolemy commented May 6, 2019 •

edited

Loading

trueptolemy May 28, 2019 •

edited

Loading