Start tracking ChannelMonitors by channel ID in ChainMonitor and ChannelManager by wpaulino · Pull Request #3554 · lightningdevkit/rust-lightning

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino · 2025-01-21T19:11:21Z

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify ChannelMonitors with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime.

In the ChainMonitor, we replace the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

In the ChannelManager, in-flight monitor updates are persisted, so we still need to track the funding outpoint to support downgrades by writing the in them as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop the old TLV.

Looking to get concept ACKs on whether we want to go down this path. The first two commits can be considered today, but the last one will have to wait a few releases due to the backwards compatibility concern. Note that this PR does not represent all of the work to achieve our goal, there's still a good bit of refactoring/cleanup to follow.

codecov · 2025-01-21T21:13:15Z

Codecov Report

Attention: Patch coverage is 91.41414% with 34 lines in your changes missing coverage. Please review.

Project coverage is 88.49%. Comparing base (569f906) to head (717db82).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.97%	18 Missing and 2 partials ⚠️
lightning/src/chain/chainmonitor.rs	89.70%	6 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	64.28%	5 Missing ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs	98.41%	0 Missing and 1 partial ⚠️
lightning/src/ln/functional_tests.rs	97.05%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3554 +/- ##
==========================================
- Coverage 88.54% 88.49% -0.05% 
==========================================
Files 149 149 Lines 114459 114709 +250 Branches 114459 114709 +250 ==========================================
+ Hits 101345 101517 +172 - Misses 10618 10691 +73 - Partials 2496 2501 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Concept ACK

jkczyz

CI Still unhappy initializing ChannelManagerReadArgs in the fuzz test.

TheBlueMatt

Conceptually LGTM, I think the first two commits make sense, yea. The last commit will obviously have to wait a release or two.

wpaulino · 2025-01-28T18:23:24Z

Rebased due to conflicts and removed the last commit

jkczyz

LGTM

TheBlueMatt

On real comment, a few nits.

Once dual funding/splicing is supported, channels will no longer maintain their original funding outpoint. Ideally, we identify `ChannelMonitor`s with a stable identifier, such as the channel ID, which is fixed throughout the channel's lifetime. This commit replaces the use of funding outpoints as the key to the monitor index with the channel ID. Note that this is strictly done to the in-memory state; it does not change how we track monitors within their persisted state, they are still keyed by their funding outpoint there. Addressing that is left for follow-up work.

As motivated by the previous commit, we do some of the same work here at the `ChannelManager` level instead. Unfortunately, we still need to track the funding outpoint to support downgrades by writing the in flight monitor updates as two separate TLVs, one using the channel IDs, and the other using the funding outpoints. Once we are willing to stop supporting downgrades past this version, we can fully drop it.

It's not needed except for one place where we can just access the field directly, so we can avoid the allocation on each call. For API consumers, they may still access the funding script via `ChannelMonitor::get_funding_script`.

wpaulino added the weekly goal Someone wants to land this this week label Jan 21, 2025

wpaulino requested review from TheBlueMatt and jkczyz January 21, 2025 19:11

wpaulino force-pushed the channel-monitor-by-channel-id branch 2 times, most recently from 40aeb66 to 1c7a0a1Compare January 21, 2025 21:04

jkczyz reviewed Jan 21, 2025
View reviewed changes

wpaulino mentioned this pull request Jan 21, 2025
Encapsulate Channel enum variants inside a struct #3550
Merged

wpaulino force-pushed the channel-monitor-by-channel-id branch from 1c7a0a1 to 850ec1bCompare January 23, 2025 20:48

jkczyz reviewed Jan 23, 2025
View reviewed changes

wpaulino force-pushed the channel-monitor-by-channel-id branch from 850ec1b to a24d821Compare January 23, 2025 23:33

TheBlueMatt reviewed Jan 24, 2025
View reviewed changes

wpaulino force-pushed the channel-monitor-by-channel-id branch from a24d821 to 5bcca97Compare January 28, 2025 18:22

jkczyz mentioned this pull request Jan 28, 2025
Support persisting ChannelMonitors after splicing #3569
Merged

jkczyz reviewed Jan 29, 2025
View reviewed changes

Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino force-pushed the channel-monitor-by-channel-id branch from 5bcca97 to c680c0fCompare January 29, 2025 21:40

jkczyz previously approved these changes Jan 29, 2025
View reviewed changes

jkczyz mentioned this pull request Jan 30, 2025
Should ChannelMonitor keys be converted to ChannelIds (and what to do with spliced ChannelMonitors) #3325
Closed

TheBlueMatt reviewed Jan 31, 2025
View reviewed changes

Comment threadlightning/src/chain/chainmonitor.rs Outdated
Comment threadlightning/src/chain/chainmonitor.rs Outdated
Comment threadlightning/src/chain/chainmonitor.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/channelmanager.rs Outdated

wpaulino added 3 commits January 31, 2025 13:26

wpaulino dismissed jkczyz’s stale review via 717db82January 31, 2025 21:26

wpaulino force-pushed the channel-monitor-by-channel-id branch from c680c0f to 717db82Compare January 31, 2025 21:26

TheBlueMatt approved these changes Jan 31, 2025
View reviewed changes

jkczyz approved these changes Jan 31, 2025
View reviewed changes

wpaulino merged commit 9b66e2c into lightningdevkit:mainJan 31, 2025

wpaulino deleted the channel-monitor-by-channel-id branch January 31, 2025 23:41

jkczyz mentioned this pull request Jun 21, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks