Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Produce a single monitor update per commitment number - #3738

Closed
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number
Closed

Produce a single monitor update per commitment number#3738
tankyleo wants to merge 1 commit into
lightningdevkit:mainfrom
tankyleo:one-update-per-commitment-number

Conversation

@tankyleo

Copy link
Copy Markdown
Contributor
 Produce a single monitor update per commitment number
Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.

@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! I see this is a draft PR.
I'll wait to assign reviewers until you mark it as ready for review.
Just convert it out of draft status when you're ready for review!

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 2adce74 to 829402cCompareApril 15, 2025 17:11
@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt this is what we've been discussing with @wpaulino can we get your impression of this approach ? Thank you.

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

A high level overview of how I am thinking these new monitor updates will get produced on channel / tx-builder side:

Channel ───────────► HTLC Outputs─────────────────────► HTLC Outputs │ │ │ │ └►──────► TX Builder ─────────► Commitment Transcation

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design makes sense to me.

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment threadlightning/src/chain/channelmonitor.rs
@wpaulino

wpaulino commented Apr 15, 2025

Copy link
Copy Markdown
Contributor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),
},
(7, LatestCounterpartyCommitmentTXs) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be even, and the members below should be odd

Comment on lines +673 to +675
(0, htlc_data, required_vec),
(2, commitment_txs, required_vec),
(4, claimed_htlcs, required_vec),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

Comment threadlightning/src/ln/chan_utils.rs Outdated
Comment on lines +609 to +612
(0, offered, required),
(2, amount_msat, required),
(4, cltv_expiry, required),
(6, payment_hash, required),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be odd

@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch 2 times, most recently from 8fd9765 to 0b93e43CompareApril 16, 2025 02:49
@codecov

codecovBot commented Apr 16, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 40.42553% with 28 lines in your changes missing coverage. Please review.

Project coverage is 89.19%. Comparing base (83e9e80) to head (0b93e43).
Report is 5 commits behind head on main.

Files with missing linesPatch %Lines
lightning/src/chain/channelmonitor.rs6.89%27 Missing ⚠️
lightning/src/ln/channel.rs94.44%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #3738 +/- ##
==========================================
+ Coverage 89.12% 89.19% +0.06% 
==========================================
Files 156 156 Lines 123514 123984 +470 Branches 123514 123984 +470 ==========================================
+ Hits 110086 110582 +496 + Misses 10749 10734 -15 + Partials 2679 2668 -11 

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Such updates will contain a single HTLC-source table for that
commitment number, together with a vector of commitment transactions
created at that commitment number. This vector will grow with the number
of pending splices for a channel.
The HTLC-source table produced by channel will stop storing information
on which HTLCs were assigned which output index. Instead, this
information will be stored in each commitment transaction in the
monitor update.
This commit deduplicates the `HTLCSource` collection for each commitment
number, but `HTLCOutputData` would store information already stored in
each `CommitmentTransaction` in a monitor update.
This is a partial rework of b8a03cd. That commit has not been shipped in
a release yet.
@tankyleo
tankyleoforce-pushed the one-update-per-commitment-number branch from 0b93e43 to 8ac0358CompareApril 16, 2025 03:06
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

@tankyleo

Copy link
Copy Markdown
ContributorAuthor

@TheBlueMatt note that going with this design will mean undoing a good chunk of the work in #3664 to no longer keep the non-dust sources separate. Should be fine as long as we get it shipped in the same release.

Why do we not want to keep them separate now?

The HTLC-source table now no longer holds "dust-vs-nondust" data; that information is now solely stored in each CommitmentTransaction.

So in the updates, we no longer have this concept of a "dust-vs-nondust source"; they are all part of a single vector that contains all the sources for this set of commitment transactions.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

@tankyleo

tankyleo commented Apr 16, 2025

Copy link
Copy Markdown
ContributorAuthor

Right, but why? I guess now because there's many commitment txn we will no longer rely on the HTLCs to be sorted in commitment-tx-order and so now it doesn't matter (and HTLCs could theoretically be dust in some commitments but not in others)?

The HTLCs could be sorted in the same commitment tx order across funding scopes, but the transaction output indices of the HTLCs might vary, as which index the non-HTLC outputs get assigned to might vary across funding scopes - because of the different channel_value_satoshis, to_remote and to_local will get different amounts.

Right now though I don't see a way for a HTLC to be dust in one scope, and non-dust in another scope... but looks like this will change with dynamic commitments if I recall.

Looking at BOLT PR 1117 yes - that is a proposal to vary the dust_limit_satoshis across funding scopes.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Right, so basically there's no point in splitting by dust/non-dust because we're not longer going to be able to rely on the order of the HTLCSources in the update, and instead now have to scan them when we need them, so we just shove them all in the update in a big heap cause it doesn't matter. Makes sense to me.

@wpaulino

Copy link
Copy Markdown
Contributor

Done in #3855

@wpaulinowpaulino closed this Jul 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@tankyleo@ldk-reviews-bot@wpaulino@TheBlueMatt