Move channel -> peer tracking to OutPoints from Channel IDs #2795
Conversation
For backwards compatibility reasons, we need to track a mapping from funding outpoints to channel ids. To reduce diff, this was previously done with channel IDs, converting the `OutPoint`s to channel IDs before using the map. This worked fine, but is somewhat brittle - because we allow redundant channel IDs across different peers, we had to avoid insertion until we had a real channel ID, and thus also had to be careful to avoid removal unless we were using a real channel ID, rather than a temporary one. This brittleness actually crept in to handling of errors in funding acceptance, allowing a remote party to get us to remove an entry by sending a overlapping temporary channel ID with a separate real channel ID. Luckily, this map is relatively infrequently used, only used in the case we see a monitor update completion from a rather ancient monitor which is unaware of the counterparty node. Even after this change, the channel -> peer tracking storage is still somewhat brittle, as we rely on entries not being added until we are confident no conflicting `OutPoint`s have been used across channels, and similarly not removing unless that check has completed.
Historically, `ChannelMonitor`s had idea who their counterparty was. This was fine, until `ChannelManager` started indexing by peer, at which point it needed to know the counterparty when it saw a `ChannelMonitorUpdate` complete. To address this, a "temporary" map from channel ID to peer was added, but no upgrade path was created for existing `ChannelMonitor`s to not rely on this map. This commit adds such an upgrade path, setting the `counterparty_node_id` on all `ChannelMonitor`s as they're updated, allowing us to eventually break backwards compatibility and remove `ChannelManager::outpoint_to_peer`.
Now that we provide the counterparty node id, we can set logging metadata with a counterparty node id and and channel id, which we do here.
TheBlueMatt
force-pushed
the
2023-11-robuster-chan-to-peer
branch
from
4930c19
to
819b677
Compare
|
Oops, fixed one more case pointed out by @wpaulino . |
|
TheBlueMatt
force-pushed
the
2023-11-robuster-chan-to-peer
branch
from
819b677
to
4dceeac
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
==========================================
+ Coverage 88.62% 90.08% +1.45%
==========================================
Files 115 115
Lines 91221 98770 +7549
Branches 91221 98770 +7549
==========================================
+ Hits 80844 88975 +8131
+ Misses 7922 7416 -506
+ Partials 2455 2379 -76 ☔ View full report in Codecov by Sentry. |
TheBlueMatt
force-pushed
the
2023-11-robuster-chan-to-peer
branch
from
4dceeac
to
08ee5b2
Compare
|
Addressed dangling comment reference introduced in an intermediate PR: $ git diff-tree -U1 4dceeace 08ee5b2ddiff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cbc0d8647..6a101dff3 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6243,4 +6243,4 @@ where
// `update_maps_on_chan_removal`), we'll remove the existing channel
- // from `id_to_peer`. Thus, we must first unset the funding outpoint on
- // the channel.
+ // from `outpoint_to_peer`. Thus, we must first unset the funding outpoint
+ // on the channel.
let err = ChannelError::Close($err.to_owned());
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index 05510eac6..fca6abb35 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -8973,6 +8973,6 @@ fn test_duplicate_temporary_channel_id_from_different_peers() {
#[test]
-fn test_duplicate_funding_funding_err() {
+fn test_duplicate_funding_err_in_funding() {
// Test that if we have a live channel with one peer, then another peer comes along and tries
// to create a second channel with the same txid we'll fail and not overwrite the
- // outpoint_to_channel map in `ChannelManager`.
+ // outpoint_to_peer map in `ChannelManager`.
// |
lightning/src/ln/channelmanager.rs
Outdated
| // on the channel. | ||
| let err = ChannelError::Close($err.to_owned()); | ||
| chan.unset_funding_info(msg.temporary_channel_id); | ||
| return Err(convert_chan_phase_err!(self, err, &mut ChannelPhase::Funded(chan), &funded_channel_id).1); |
There was a problem hiding this comment.
It doesn't matter what ChannelPhase is used here, right? I think ChannelPhase variants could use some docs, hard to tell that this is the correct phase here.
There was a problem hiding this comment.
It doesnt. The only thing that changes is whether we broadcast a closing channel_update, which we won't anyway cause we don't have an SCID.
There was a problem hiding this comment.
Sadly the struct is type Channel which means we have to use Funded. That said, its clearly "wrong" - the channel is not (any longer) funded, I skipped the phase wrapping and used the macro form that explicitly states the funding status.
| return Err(MsgHandleErrInternal::send_err_msg_no_close( | ||
| "The funding_created message had the same funding_txid as an existing channel - funding is not possible".to_owned(), | ||
| channel_id)); | ||
| fail_chan!("Duplicate funding outpoint"); |
There was a problem hiding this comment.
Missing test coverage for this change. It looks like a behavior change? Doesn't seem harmful though and the spec doesn't seem to weigh in on what to do here.
There was a problem hiding this comment.
We absolutely need to fail here. If we have two channels open with the same funding we'll happily let that funding amount get double-counted. But, its not a behavior change, we always failed here (the channel has been taken out of the map, we're just gonna drop it either way).
When we fail to accept a counterparty's funding for various reasons, we should ensure we call the correct cleanup methods in `internal_funding_created` to remove the temporary data for the channel in our various internal structs (primarily the SCID alias map). This adds the missing cleanup, using `convert_chan_phase_err` consistently in all the error paths. This also ensures we get a `ChannelClosed` event when relevant.
TheBlueMatt
force-pushed
the
2023-11-robuster-chan-to-peer
branch
from
08ee5b2
to
e9452c7
Compare
|
I went ahead and changed the failure macro to call the $ git diff-tree -U3 08ee5b2d e9452c70diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 6a101dff3..08296c434 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6245,7 +6245,7 @@ where
// on the channel.
let err = ChannelError::Close($err.to_owned());
chan.unset_funding_info(msg.temporary_channel_id);
- return Err(convert_chan_phase_err!(self, err, &mut ChannelPhase::Funded(chan), &funded_channel_id).1);
+ return Err(convert_chan_phase_err!(self, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
} } }
match peer_state.channel_by_id.entry(funded_channel_id) { |
For backwards compatibility reasons, we need to track a mapping
from funding outpoints to channel ids. To reduce diff, this was
previously done with channel IDs, converting the
OutPoints tochannel IDs before using the map.
This worked fine, but is somewhat brittle - because we allow
redundant channel IDs across different peers, we had to avoid
insertion until we had a real channel ID, and thus also had to be
careful to avoid removal unless we were using a real channel ID,
rather than a temporary one.
This brittleness actually crept in to handling of errors in funding
acceptance, allowing a remote party to get us to remove an entry by
sending a overlapping temporary channel ID with a separate real
channel ID.
Luckily, this map is relatively infrequently used, only used in the
case we see a monitor update completion from a rather ancient
monitor which is unaware of the counterparty node.
Even after this change, the channel -> peer tracking storage is
still somewhat brittle, as we rely on entries not being added until
we are confident no conflicting
OutPoints have been used acrosschannels, and similarly not removing unless that check has
completed.