[WIP] New ChannelId enum by optout21 · Pull Request #2451 · lightningdevkit/rust-lightning

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 · 2023-07-26T06:04:23Z

Fixes#2408 . Introduces a new ChannelId enum, with two variants, for normal funding-tx-based and temporary ID, both wrapper around 32-byte data.

TODO:

Finalize naming (common case is now called FundingTxBased)
Determine how it affects APIs, uses, backward-compatibility
How to preserve the variant at serialization? (both are stored as the 32 bytes)

codecov-commenter · 2023-07-26T07:11:53Z

Codecov Report

Patch coverage: 81.84% and no project coverage change.

Comparison is base (685f266) 90.33% compared to head (70fb868) 90.33%.

❗ 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 #2451 +/- ##
=======================================
Coverage 90.33% 90.33% =======================================
Files 106 106 Lines 55732 55792 +60 Branches 55732 55792 +60 =======================================
+ Hits 50347 50402 +55 - Misses 5385 5390 +5

Files Changed	Coverage Δ
lightning/src/routing/gossip.rs	`89.67% <0.00%> (-0.07%)`	⬇️
lightning/src/ln/peer_handler.rs	`61.40% <13.79%> (-0.21%)`	⬇️
lightning/src/routing/router.rs	`93.57% <50.00%> (-0.06%)`	⬇️
lightning/src/util/macro_logger.rs	`89.58% <50.00%> (ø)`
lightning/src/events/mod.rs	`43.40% <76.47%> (+1.30%)`	⬆️
lightning/src/ln/msgs.rs	`85.51% <81.66%> (+0.54%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.73% <88.37%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`89.52% <97.14%> (+0.07%)`	⬆️
lightning-invoice/src/utils.rs	`97.69% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`94.82% <100.00%> (ø)`
... and 5 more

... and 5 files with indirect coverage changes

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

tnull · 2023-07-26T08:26:11Z

 	for channel in channels.into_iter().filter(|chan| chan.is_channel_ready) {
 if channel.get_inbound_payment_scid().is_none() || channel.counterparty.forwarding_info.is_none() {
- log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id));
+ log_trace!(logger, "Ignoring channel {} for invoice route hints", log_bytes!(channel.channel_id.bytes()[..]));


Since we're already touching it it would be nice to implement Display/Debug for ChannelId and log in a more readable form, e.g., as a hex string.

I've just done this (in #2485), and I've seen this comment only now :)

optout21 · 2023-07-26T21:17:05Z

One fuzz test fails, to check

optout21 added 2 commits July 26, 2023 07:40

New ChannelId type, wrapper for [u8; 32]
2601287

ChannelId is an enum
70fb868

tnull reviewed Jul 26, 2023
View reviewed changes