Skip to content

Introduce new ChannelId struct - #2485

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
optout21:channel-id-4struct1
Aug 27, 2023
Merged

Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
optout21:channel-id-4struct1

Conversation

@optout21

@optout21optout21 commented Aug 9, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.

ChannelId is exposed as lightning::ln::ChannelId.

This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:

  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()

The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).

Review hints:

TODO:

  • Determine how it affects APIs, uses, backward-compatibility
  • Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  • Review hints

@optout21optout21 mentioned this pull request Aug 9, 2023
Closed
4 tasks
@codecov-commenter

codecov-commenter commented Aug 10, 2023•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 84.16% and project coverage change: +0.02% 🎉

Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.
Report is 2 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more

... and 5 files with indirect coverage changes

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

@optout21
optout21 marked this pull request as ready for review August 10, 2023 07:54
This was referenced Aug 10, 2023
Closed
Closed
@optout21optout21 changed the title [WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen

dunxen commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like

  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.

For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.

@optout21optout21 mentioned this pull request Aug 12, 2023
Merged
TheBlueMatt
TheBlueMatt reviewed Aug 14, 2023
View reviewed changes

@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.

Mostly some minor process nits on commit construction, but LGTM.

Comment threadlightning/src/chain/transaction.rs Outdated
Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning-invoice/src/utils.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@optout21
optout21force-pushed the channel-id-4struct1 branch 2 times, most recently from 50565dc to c84dad1CompareAugust 22, 2023 15:23
@optout21

optout21 commented Aug 22, 2023

Copy link
Copy Markdown
ContributorAuthor

Review comments addressed. commits regrouped (into 2 commits) as suggested.
Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.

dunxen
dunxen reviewed Aug 23, 2023
View reviewed changes

@dunxendunxen left a comment

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.

LGTM with moving the ChannelId stuff to its own mod.

Comment threadlightning/src/ln/channel.rs Outdated
Comment threadlightning/src/ln/channel.rs Outdated
@optout21
optout21force-pushed the channel-id-4struct1 branch 2 times, most recently from 31cf609 to 7ec2b2eCompareAugust 24, 2023 09:38
@optout21

optout21 commented Aug 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits

dunxen
dunxen previously approved these changes Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs Outdated
@dunxen

dunxen commented Aug 24, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Oops sorry, yeah looks good for squash. Also missing module docs for the new module

@optout21
optout21force-pushed the channel-id-4struct1 branch 2 times, most recently from c89c64e to 5854f40CompareAugust 24, 2023 12:20
@optout21

optout21 commented Aug 24, 2023

Copy link
Copy Markdown
ContributorAuthor

Per-commit check failed, fixed (moved last fix in first commit)

dunxen
dunxen previously approved these changes Aug 24, 2023
View reviewed changes
@optout21

optout21 commented Aug 25, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)
Rebased again.

@TheBlueMatt

TheBlueMatt commented Aug 25, 2023

Copy link
Copy Markdown
Collaborator

Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.

TheBlueMatt
TheBlueMatt reviewed Aug 25, 2023
View reviewed changes

@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.

Two quick comments, didnt look at the second commit but i assume its fine.

Comment threadlightning/src/ln/channel_id.rs Outdated
Comment threadlightning/src/ln/channel_id.rs Outdated
@optout21

optout21 commented Aug 25, 2023

Copy link
Copy Markdown
ContributorAuthor

Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).
Also found some instances in tests where temp ID is generated, but from_bytes() was used.
Also some rustdoc formatting.

TheBlueMatt
TheBlueMatt approved these changes Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};

@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?

@optout21optout21Aug 26, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.

@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)

@optout21

optout21 commented Aug 26, 2023

Copy link
Copy Markdown
ContributorAuthor

2 CI builds fail, likely due to #2527 . 2 approvals.

@TheBlueMatt
TheBlueMatt merged commit 61d896d into lightningdevkit:mainAug 27, 2023
@optout21optout21 mentioned this pull request Aug 27, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes
+1 more reviewer
@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type

5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Introduce new ChannelId struct by optout21 · Pull Request #2485 · lightningdevkit/rust-lightning · GitHub
Skip to content
Introduce new ChannelId struct - #2485
Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Aug 27, 2023
Merged
Introduce new ChannelId struct#2485
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom 
optout21:channel-id-4struct1
Conversation
@optout21
@optout21optout21
commented
Aug 9, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa

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

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#2408 . Introduces a new ChannelId struct, enclosing the 32-byte data. It has specific constructors, for normal funding-tx-based and for temporary channel ID cases.
ChannelId is exposed as lightning::ln::ChannelId.
This is a breaking change. The type of channel_id parameter has been changed from [u8; 32] to ChannelId in several ChannelManager APIs, namely:
  • create_chan_between_nodes()
  • create_chan_between_nodes_with_value()
  • create_funding_transaction()
  • sign_funding_transaction()
  • open_zero_conf_channel()
  • create_chan_between_nodes_with_value_confirm_second()
  • create_chan_between_nodes_with_value_confirm()
  • create_chan_between_nodes_with_value_a()
  • create_announced_chan_between_nodes()
  • create_announced_chan_between_nodes_with_value()
  • close_channel()
  • test_txn_broadcast()
The type has been changed in several events as well (FundingGenerationReady, PaymentClaimable, PaymentForwarded, ChannelPending, etc.).
Review hints:
TODO:
  •  Determine how it affects APIs, uses, backward-compatibility
  •  Tests for ChannelId serialization/deserialization
  • New macro log_channel_id!()
  •  Review hints
@optout21optout21
mentioned this pull request
Aug 9, 2023
 Closed

4 tasks
@codecov-commenter
codecov-commenter
commented
Aug 10, 2023•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Patch coverage: 84.16% and project coverage change: +0.02% 🎉
Comparison is base (3dffe54) 90.56% compared to head (e99e6ab) 90.59%.

Report is 2 commits behind head on main.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@ Coverage Diff @@## main #2485 +/- ##
==========================================
+ Coverage 90.56% 90.59% +0.02% 
==========================================
Files 109 110 +1 Lines 57304 57410 +106 Branches 57304 57410 +106 ==========================================
+ Hits 51899 52008 +109 + Misses 5405 5402 -3 
Files ChangedCoverage Δ
lightning/src/ln/mod.rs96.15% <ø> (ø)
lightning/src/routing/gossip.rs89.94% <0.00%> (-0.13%)⬇️
lightning/src/util/test_utils.rs73.61% <ø> (ø)
lightning/src/ln/peer_handler.rs61.45% <13.79%> (+0.40%)⬆️
lightning/src/util/macro_logger.rs93.61% <50.00%> (+4.03%)⬆️
lightning/src/events/mod.rs48.33% <78.94%> (-0.10%)⬇️
lightning/src/ln/msgs.rs85.82% <81.66%> (+0.21%)⬆️
lightning/src/ln/channelmanager.rs87.19% <88.23%> (-0.02%)⬇️
lightning/src/ln/channel.rs89.83% <95.83%> (+<0.01%)⬆️
lightning-invoice/src/utils.rs97.67% <100.00%> (ø)
... and 7 more
... and 5 files with indirect coverage changes
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
@optout21
optout21
marked this pull request as ready for review
August 10, 2023 07:54

This was referenced Aug 10, 2023
 Closed

 Closed

@optout21optout21
changed the title
[WIP] Introduce new ChannelId structIntroduce new ChannelId structAug 10, 2023
@dunxen
dunxen
commented
Aug 10, 2023

Copy link


Copy Markdown

Contributor
This overall looks like an improvement, but I'd like to maybe see what others had to say about types for different IDs like
  • temporary channel IDs for V1 unfunded channels
  • channel IDs for V1 funded channels with known funding txids
  • temporary channel IDs (revocation basepoint XORed with zero) for V2 channels for open_channel2 and accept_channel2
  • channel IDs (both party revocation basepoints XORed) for V2 channels after accept_channel2.
For me currently I don't see a huge benefit in splitting all these cases into distinct types or even enum variants. So I think this PR is all we need so that we at least have type safety for channel ID bytes vs hash bytes for example.
@optout21optout21
mentioned this pull request
Aug 12, 2023
 Merged

TheBlueMatt
TheBlueMatt
reviewed
Aug 14, 2023
View reviewed changes
@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.

Mostly some minor process nits on commit construction, but LGTM.
Comment threadlightning/src/chain/transaction.rs
Outdated

Comment threadlightning/src/chain/transaction.rs
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning-invoice/src/utils.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
50565dc to
c84dad1CompareAugust 22, 2023 15:23
@optout21
optout21
commented
Aug 22, 2023

Copy link


Copy Markdown

ContributorAuthor
Review comments addressed. commits regrouped (into 2 commits) as suggested.

Only outstanding issue is where to have the ChannelId declaration -- leave in channel.rs, in mod.rs, or in new, own mod.
dunxen
dunxen
reviewed
Aug 23, 2023
View reviewed changes
@dunxendunxen
left a comment


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.

LGTM with moving the ChannelId stuff to its own mod.
Comment threadlightning/src/ln/channel.rs
Outdated

Comment threadlightning/src/ln/channel.rs
Outdated

@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
31cf609 to
7ec2b2eCompareAugust 24, 2023 09:38
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Moved ChannelId to its own new module (channel_id.rs); but need to clean up the commits
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Outdated

@dunxen
dunxen
commented
Aug 24, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Oops sorry, yeah looks good for squash. Also missing module docs for the new module
@optout21
optout21force-pushed
the
channel-id-4struct1
branch
2 times, most recently
from
c89c64e to
5854f40CompareAugust 24, 2023 12:20
@optout21
optout21
commented
Aug 24, 2023

Copy link


Copy Markdown

ContributorAuthor
Per-commit check failed, fixed (moved last fix in first commit)
dunxen
dunxen
previously approved these changes
Aug 24, 2023
View reviewed changes
@optout21
optout21
commented
Aug 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
This PR touches many logging lines, so it frequently gets in conflict with current main... I keep rebasing+resolving conflicts, but it would be nice to get more approvals ;)

Rebased again.
@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2023

Copy link


Copy Markdown

Collaborator
Heh, sorry, been a super busy merge week. Will review now and hopefully @dunxen is still around to ack.
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2023
View reviewed changes
@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.

Two quick comments, didnt look at the second commit but i assume its fine.
Comment threadlightning/src/ln/channel_id.rs
Outdated

Comment threadlightning/src/ln/channel_id.rs
Outdated

@optout21
optout21
commented
Aug 25, 2023

Copy link


Copy Markdown

ContributorAuthor
Simplified as suggested (@TheBlueMatt): changed struct into a pub tuple; removed .bytes() (use .0 instead).

Also found some instances in tests where temp ID is generated, but from_bytes() was used.

Also some rustdoc formatting.
TheBlueMatt
TheBlueMatt
approved these changes
Aug 26, 2023
View reviewed changes
Comment threadlightning/src/ln/channel_id.rs
Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/payment_tests.rs
use crate::chain::transaction::OutPoint;
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS};
@TheBlueMattTheBlueMattAug 26, 2023

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.

why did you add brackets?
@optout21optout21Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Hmm, typo, after subsequent add and delete.
@TheBlueMattTheBlueMattAug 27, 2023

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.

Good to keep the blame layer clean (doesn't matter much for use statements), but makes it easier to read stuff back later :)
@optout21
optout21
commented
Aug 26, 2023

Copy link


Copy Markdown

ContributorAuthor
2 CI builds fail, likely due to #2527 . 2 approvals.
@TheBlueMatt
TheBlueMatt
merged commit 61d896d
into
lightningdevkit:mainAug 27, 2023
@optout21optout21
mentioned this pull request
Aug 27, 2023
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

+1 more reviewer

@jbesraajbesraajbesraa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Abstraction of ChannelId type


5 participants

@optout21@codecov-commenter@dunxen@TheBlueMatt@jbesraa