fix: Add information about how many blocks to go until funding is confirmed

[trueptolemy]

This is another try for issue #2150 and issue #1780 as @rustyrussell suggested:

1. Add minimum_depth in struct channel in common/initial_channel.h and change corresponding init function: new_initial_channel().

2. Add confirmation_needed in in struct peer in channeld/channeld.c.

3. Rename channel_funding_locked to channel_funding_depth in
   channeld/channel_wire.csv.

4. Rename channel_tell_funding_locked to channel_tell_depth.

5. Rename funding_lockin_cb to funding_depth_cb in lightningd/peer_cntrol.c.

6. When funding depth changes, Master calls channel_tell_depth even if depth < minimum_depth and tell channeld the funding depth. But in channel_tell_depth, lockin_complete will be called iff depth > minimum_depth.

7. Channeld ignores the funding_depth unless depth >= minimum_depth(except to update billboard, and set peer->confirmation_needed = minimum_depth - depth). Note confirmation_needed in struct peer must be 0 when funding hit minimum_depth in locally topology but it doesn't correspond to remote locking state because of network delay.

Thank you all for your comments and suggestions!

[SimonVrouwe]

Ok, I believe normally cdecker does the first review for new contributors, but I have already given some comments (hopefully better advice then the first one). Looks good 👍

[trueptolemy]

Ok, I believe normally cdecker does the first review for new contributors, but I have already given some comments (hopefully better advice then the first one). Looks good +1

Thank you again! Your advice are great :)

[rustyrussell]

OK, this looks great! I'd usually say you should rebase and add a CHANGELOG.md message, but I'll do it in this case to avoid further delays :)

Thanks for your patience!

[rustyrussell]

Ack 7a02553

[trueptolemy]

@rustyrussell Thanks for your suggestion:)
Sorry, I will add the CHANGELOG.md next time!

[cdecker]

Adding the failures when togo and funding_locked[LOCAL] do not agree makes me wonder if that can't happen during a rescan.

[cdecker]

This error made me stumble a bit. Are we sure this can't happen during restarts with rescans?

[trueptolemy]

Hi, @cdecker I'm not sure if what I think is correct. Hope you don't mind my pool English.

In the original code, the channel will be saved in the DB in this code block.
But now, to prevent saving channel with unlocked funding, I add the condition local_locked && !channel->scid. So, these channel with unlocked funding won't be saved until depth >= minimum_depth.

And the struct peer in Channeld always inits from there. The peer in Channeld won't be saved in DB (this is because I don't find function to save this struct. I'm not sure.), and it is only dependent on the channel information in the Lightningd.

I suppose when we restart, we need rebuild this peer struct(in Channeld) according to the channel(in Lightningd) again (it bases the suppose that we don't save peer struct in Channeld).
There may be 4 situations:

1. the depth >= minimum_depth(we can get the topo from DB) , and the channel has been saved in DB before: we can init the peer with peer->funding_locked[LOCAL] == true(there we use if channel->scid exist to init funding_locked[LOCAL]), and peer->depth_togo == minimum_depth(I directly set this value to peer->depth_togo). But after some minutes, Lightningd will tell us the depth >= minimum_depth, we won't meet this error.
2. the depth >= minimum_depth, but we didn't save the channel in time: we can init the peer with peer->funding_locked[LOCAL] = false, and we alse init the peer->depth_togo = minimum_depth directly.
3. the depth < minimum_depth, and we saved the channel in the DB: it is impossible! we only save the channel under the condition that funding is locked.
4. the depth < minimum_depth, and we didn't save the channel: we can init peer with funding_locked[LOCAL] = false and peer->depth_togo = minimum_depth.

So I think we won't meet this error when restart. What do you think about this?

[trueptolemy]

Hi!@cdecker I think this error won't happen unless the topo reorganize with setting too samll minimum_depth.
The peer we used in Channeld is really initialed according the channel_init message from Lightning. And we never save the peer in the DB directly. In other words, during every start, we need initial the peer->funding_locked[LOCAL] from Lightningd(peer->funding_locked[LOCAL] corresponds if channel->scid exist). So the 4 situations I listed above are reasonable.

I think the only situation that this error happens is:
Lightningd find the funding depth changes and don't hit minimum_depth, then Lightningd tell us the depth now. But before that, funding has hit minimum_depth once and we has drived the channel->scid. The reason of this situation maybe the minimum_depth is too small and topo reorganized.
We don't limit the smallist minimum_depth(BOLT2 just ask mimimum_depth can't be too big, and clightning ask it must be smaller than 10). But I think we don't need to set the smallest minimum_depth. This condition here can be a kind of alert that reorg has happend.
what do you think about?

[SimonVrouwe]

I know this goes off-topic, but currently we allow small minimum_depth = 1 (the default is 3), which is easily reorged and can change scid. I actually think that scid should not be calculated before ANNOUNCE_MIN_DEPTH (=6), do we really need the scid before that?

[SimonVrouwe]

So IMHO the above check can be removed.

[trueptolemy]

@SimonVrouwe You'are right, and I should delete these checks here.
And I also notice a extreme case:
Suppose we set a private channel (don't set announcement flag), and set a too small minimum locking depth (as you say, 1). When we notice the funding tx locked and we drive short_channel_id and store into the DB, but unfortunately, after these operation, our node crash. When we restart, the reorg happens.
Now we initial our channel from DB with old short_channel_id, and we may can't be told that reorg happened because we delete the corresponding watchtx.

[SimonVrouwe]

... and we may can't be told that reorg happened because we delete the corresponding watchtx.

Good catch!
Wallet transactions are never deleted from the db, but at reorg their height field is set to NULL. When a block is reorged-out, all tx's with that (old) height are selected, and their cb is fired with depth 0.

But if the watch was deleted, after only 1 confirm when minimum_depth=1 and ANNOUNCE_FLAG unset, the watch is gone and nothing happens!

@cdecker So this looks like a bug? Although it is a very rare case (how often do tx get reorged to another height?), I will make a new issue about this.

[trueptolemy]

It's a really rare case. The main problem I think is too samll minimum_depth.
Maybe we can set limit of the minimum_depth, or any other operations to handle it(fail the channel, or generate warning...).

[SimonVrouwe]

I think this error won't happen unless the topo reorganize with setting too samll minimum_depth.

I have been running some tests with reorgs. Because we allow small minimum_depth = 1, so we can operate the channel already after 1 confirm (for direct payments only, without announcing), the funding tx may be reorged out and then mined in another block after lockin.

When that happens, the short_channel_id should be recalculated and stored in DB. Or perhaps we should wait with calculating scid until ANNOUNCE_MIN_DEPTH? Not sure if that belongs in this PR or a new one.

When funding tx is reorged out, the funding_depth_cb is called with depth=0.

Maybe we should add a python test for these kind of reorgs? I can give it a shot.

I made a branch based on your PR which includes suggested modifications.

[trueptolemy]

@SimonVrouwe Thank you and you help me sooooo much:) ! Let me have a look!

[trueptolemy]

@cdecker Thank you, and I've rebased it :)
I delete the check as you marked and do some change as @SimonVrouwe suggested.
But there is a reorg case (maybe a bug we don't handle now) as Simon and I descripted above (here and here).
The original check is for this case, but failing the program directly is not approprite. The way to handle this case maybe need a another issue and PR.

[rustyrussell]

Ack 7f55203

[rustyrussell]

I am so sorry that this merge took so long! Great work though!