Do not generate a channel-closed mon update for never-signed chans

[TheBlueMatt]

I'm getting some fuzzing iterations in for 0.0.12, and sadly found a few small things.

The full_stack_target managed to find a bug where, if we receive
a funding_created message which has a channel_id identical to an
existing channel, we'll end up
(a) having the monitor update for the new channel fail (due to
duplicate outpoint),
(b) creating a monitor update for the new channel as we
force-close it,
(c) panicing due to the force-close monitor update is applied to
the original channel and is considered out-of-order.

Obviously we shouldn't be creating a force-close monitor update for
a channel which can never appear on chain, so we do that here and
add a test which previously failed and checks a few
duplicate-channel-id cases.

[codecov]

Codecov Report

Merging #750 (58eeb6b) into main (8f10a1d) will decrease coverage by 0.11%.
The diff coverage is 89.43%.

@@            Coverage Diff             @@
##             main     #750      +/-   ##
==========================================
- Coverage   91.44%   91.32%   -0.12%     
==========================================
  Files          37       37              
  Lines       22249    22383     +134     
==========================================
+ Hits        20346    20442      +96     
- Misses       1903     1941      +38     

Impacted Files	Coverage Δ
lightning/src/chain/chainmonitor.rs	94.66% <0.00%> (+2.66%)	⬆️
lightning/src/ln/chan_utils.rs	95.66% <0.00%> (-0.31%)	⬇️
lightning/src/ln/msgs.rs	89.60% <0.00%> (-0.48%)	⬇️
lightning/src/routing/router.rs	95.71% <0.00%> (-0.96%)	⬇️
lightning/src/util/events.rs	24.54% <0.00%> (ø)
lightning/src/ln/channelmanager.rs	85.02% <75.00%> (-0.34%)	⬇️
lightning/src/ln/functional_tests.rs	96.92% <94.89%> (-0.23%)	⬇️
lightning/src/chain/channelmonitor.rs	95.44% <100.00%> (ø)
lightning/src/ln/channel.rs	87.45% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.13% <100.00%> (+0.02%)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f10a1d...36063ee. Read the comment docs.

[TheBlueMatt]

full_stack_target found a second, very related bug, so went ahead and fixed that too.

[valentinewallace]

Nice to get these bugfixes in 🌴

[valentinewallace]

nit, add panic error string and error logs if a user manages to hit this (because even though they can hit this as a proxy, iiuc they shouldn't)?

[TheBlueMatt]

I do hope no user is building with feature = fuzztarget! But, added a message in case we hit it while writing tests.

[valentinewallace]

Rather than adding this function, I prefer this approach:

diff --git a/lightning/src/ln/general_unit_tests.rs b/lightning/src/ln/general_unit_tests.rs
index 15fdf1f0..b13eee6f 100644
--- a/lightning/src/ln/general_unit_tests.rs
+++ b/lightning/src/ln/general_unit_tests.rs
@@ -569,7 +569,8 @@ fn test_duplicate_chan_id() {
        }
        let funding_signed_msg = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id());

-       let channel_id = ::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
+       let outpoint = ::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index };
+       let channel_id = outpoint.to_channel_id();

        // Now we have the first channel past funding_created (ie it has a txid-based channel_id, not a
        // temporary one).
@@ -600,9 +601,14 @@ fn test_duplicate_chan_id() {
        nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
        create_funding_transaction(&nodes[0], 100000, 42); // Get and check the FundingGenerationReady event

-       nodes[0].node.funding_transaction_generated_dup_funding_id(&open_chan_2_msg.temporary_channel_id, funding_output);
+       let funding_created = {
+               let mut a_channel_lock = nodes[0].node.channel_state.lock().unwrap();
+               let mut as_chan = a_channel_lock.by_id.get_mut(&open_chan_2_msg.temporary_channel_id).unwrap();
+               let logger = test_utils::TestLogger::new();
+               as_chan.get_outbound_funding_created(outpoint, &&logger).unwrap()
+       };
        check_added_monitors!(nodes[0], 0);
-       nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id()));
+       nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created);
        // At this point we'll try to add a duplicate channel monitor, which will be rejected, but
        // still needs to be cleared here.
        check_added_monitors!(nodes[1], 1);

[TheBlueMatt]

Oh nice! I admit I spent too long trying to manually build the message signature, didn't realize our internal method here already made it easy.

[valentinewallace]

last 2 comments

[TheBlueMatt]

Squashed with no diff.

[jkczyz]

Consider making a local variable for the AcceptChannel message for consistency with how it was done earlier.

[TheBlueMatt]

Ah! I had previously moved to only storing as a variable if we need to modify it and resend. I just missed that we no longer modify+resend the previous one.

[TheBlueMatt]

Only change was to make the two handle_accept_channel calls identical.