lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id #9390

NishantBansal2003 · 2024-12-28T09:51:27Z

Change Description

Fixes: #8650
This PR enhances channel ID representation by introducing both integer and string formats for ShortChannelId:

Renamed ChanId to ShortChanIdInt:
- Renamed to better reflect its role as the integer representation of ShortChannelId.
Added ShortChanId:
- Introduced a string representation of ShortChannelId in the format block x transaction x output.
Introduced chan_id:
- Added chan_id based on the definition in the BOLT-2 Peer Protocol specification, aligning with Lightning Network standards.

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

coderabbitai · 2024-12-28T09:51:34Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

NishantBansal2003 · 2024-12-29T05:56:41Z

PTAL @ziggie1984

ziggie1984 · 2024-12-29T07:42:28Z

Please read the contribution guidelines thoroughly in particular what was immediately catching my eye is the commit structure

NishantBansal2003 · 2024-12-29T07:52:04Z

Please read the contribution guidelines thoroughly in particular what was immediately catching my eye is the commit structure

Got it! Working on it Now...

alexbosworth · 2024-12-29T16:47:07Z

Maybe people should be warned that a renaming an API attribute would break things

ziggie1984 · 2024-12-29T17:54:01Z

lnrpc/lightning.proto

@@ -1471,7 +1471,7 @@ message Channel {
    height, the next 3 the index within the block, and the last 2 bytes are the
    output index for the channel.
    */
-    uint64 chan_id = 4 [jstype = JS_STRING];
+    uint64 short_chan_id_int = 4 [jstype = JS_STRING];


we are not renaming stuff, we are deprecating it instead, so no worries @alexbosworth, this approach is not the right one here.

Should I mark this chan_id as deprecated and introduce fields like short_chan_id_int, short_chan_id, and channel_id?

I think we can keep it but instead return the right chan_id not the short chan id here, but I need to think about backwards-compatibility issues.

The right chan_id being in string format may also introduce backward compatibility issues.

So, I can think of two solutions:

Mark chan_id as deprecated and introduce fields like short_chan_id_int, short_chan_id, and channel_id.

Introduce new names (eg.- chan_id for uint64, chan_id_str for string, and channel_id for BOLT-2 definition) for the additional fields while preserving the same name for the existing field.

wdyt?

Yeah you cannot reuse this, it is uint64 and for the real channel_id we need string or bytes, so best is to start from a clean state and deprecate the old value.

NishantBansal2003 · 2025-01-01T10:09:26Z

Hey @ziggie1984, I have addressed the Git commit structure and made the required changes based on your review. Please take a look.

ziggie1984 · 2025-01-01T22:44:08Z

Thank you for the PR, I think it is enough to have the short_chan_id and the normal channel_id in the rpc msg. Then when for example depicting the channel struct via lncli listchannels we should show both representation for the short channel id, but we should not put the different types into the protofile (int&string) that should then happen at the lncli or application level.

NishantBansal2003 · 2025-01-02T18:01:42Z

Thank you for the PR, I think it is enough to have the short_chan_id and the normal channel_id in the rpc msg. Then when for example depicting the channel struct via lncli listchannels we should show both representation for the short channel id, but we should not put the different types into the protofile (int&string) that should then happen at the lncli or application level.

Addresses the reviewed changes. Here is how the lncli listchannels output now appears (on the regtest network):

{
    "channels": [
        {
            "active": true,
            "remote_pubkey": "02769271ddfb03149bc9e2e6124bb8d83209284dda3affed0dead73f57b64f45dd",
            "channel_point": "6ab312e3b744e1b80a33a6541697df88766515c31c08e839bf11dc9fcc036a19:0",
            "chan_id": "829031767408640",
            "capacity": "20000",
            "local_balance": "16530",
            "remote_balance": "0",
            "commit_fee": "3140",
            "commit_weight": "772",
            "fee_per_kw": "2500",
            "unsettled_balance": "0",
            "total_satoshis_sent": "0",
            "total_satoshis_received": "0",
            "num_updates": "0",
            "pending_htlcs": [],
            "csv_delay": 144,
            "private": false,
            "initiator": true,
            "chan_status_flags": "ChanStatusDefault",
            "local_chan_reserve_sat": "354",
            "remote_chan_reserve_sat": "354",
            "static_remote_key": false,
            "commitment_type": "ANCHORS",
            "lifetime": "11",
            "uptime": "11",
            "close_address": "",
            "push_amount_sat": "0",
            "thaw_height": 0,
            "local_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "354",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "19800000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "remote_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "354",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "19800000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "alias_scids": [],
            "zero_conf": false,
            "zero_conf_confirmed_scid": "0",
            "peer_alias": "02769271ddfb03149bc9",
            "peer_scid_alias": "0",
            "memo": "",
            "custom_channel_data": "",
            "channel_id": "196a03cc9fdc11bf39e8081cc315657688df971654a6330ab8e144b7e312b36a",
            "short_chan_id": "829031767408640",
            "short_chan_id_str": "754x1x0"
        },
        {
            "active": true,
            "remote_pubkey": "02769271ddfb03149bc9e2e6124bb8d83209284dda3affed0dead73f57b64f45dd",
            "channel_point": "e428fb553769e4e53dbfb9749f8b057272969517ea85c63f46288487bfd62cf0:0",
            "chan_id": "807041534853120",
            "capacity": "100000",
            "local_balance": "0",
            "remote_balance": "96530",
            "commit_fee": "3140",
            "commit_weight": "772",
            "fee_per_kw": "2500",
            "unsettled_balance": "0",
            "total_satoshis_sent": "0",
            "total_satoshis_received": "0",
            "num_updates": "0",
            "pending_htlcs": [],
            "csv_delay": 144,
            "private": false,
            "initiator": false,
            "chan_status_flags": "ChanStatusDefault",
            "local_chan_reserve_sat": "1000",
            "remote_chan_reserve_sat": "1000",
            "static_remote_key": false,
            "commitment_type": "ANCHORS",
            "lifetime": "459",
            "uptime": "421",
            "close_address": "",
            "push_amount_sat": "0",
            "thaw_height": 0,
            "local_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "1000",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "99000000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "remote_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "1000",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "99000000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "alias_scids": [],
            "zero_conf": false,
            "zero_conf_confirmed_scid": "0",
            "peer_alias": "02769271ddfb03149bc9",
            "peer_scid_alias": "0",
            "memo": "",
            "custom_channel_data": "",
            "channel_id": "f02cd6bf878428463fc685ea1795967272058b9f74b9bf3de5e4693755fb28e4",
            "short_chan_id": "807041534853120",
            "short_chan_id_str": "734x1x0"
        }
    ]
}

ziggie1984 · 2025-01-02T21:25:03Z

While looking at the protobuf file, a lot of the structs use chan_id for the short channel id, so I am at this point not convinced about deprecating this value but rather just adding a new field for the longer channel id to keep the change small.

Maybe something like funding_outpoint_id

I think it is not really worth dealing with all these name changes in the protofiles if so many structs are affected. Wdyt @guggero ?

ziggie1984 · 2025-01-03T10:40:10Z

I have a better idea:

So initial request was to make the lncli listchannels output more detailed by adding the channel_id and also adding the human-readable representation of the short channel_id to the output of the command.

We can do all this by just changing the lncli program rather than touching the lnd core unit. The human-readable representation of the short_channel_id can be derived from the integer value currently already available. Moreover also the Xored ChannelID can be calculated using the ChannelPoint already available in the grpc msg.

So I favour the approach to not change anything in the RPC code, but just add all the information on the lncli side!

Modified the `String` method of `ShortChannelID` to generate a human-readable representation using 'x' as a separator. (block x transaction x output). Signed-off-by: Nishant Bansal <[email protected]>

…s output. Signed-off-by: Nishant Bansal <[email protected]>

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 · 2025-01-03T17:52:51Z

I have a better idea:

So initial request was to make the lncli listchannels output more detailed by adding the channel_id and also adding the human-readable representation of the short channel_id to the output of the command.

We can do all this by just changing the lncli program rather than touching the lnd core unit. The human-readable representation of the short_channel_id can be derived from the integer value currently already available. Moreover also the Xored ChannelID can be calculated using the ChannelPoint already available in the grpc msg.

So I favour the approach to not change anything in the RPC code, but just add all the information on the lncli side!

Done. Here is how the lncli listchannels output now appears:

{
    "channels": [
        {
            "active": false,
            "remote_pubkey": "02769271ddfb03149bc9e2e6124bb8d83209284dda3affed0dead73f57b64f45dd",
            "channel_point": "6ab312e3b744e1b80a33a6541697df88766515c31c08e839bf11dc9fcc036a19:0",
            "funding_outpoint_id": "196a03cc9fdc11bf39e8081cc315657688df971654a6330ab8e144b7e312b36a",
            "chan_id": "829031767408640",
            "chan_id_str": "754x1x0",
            "capacity": "20000",
            "local_balance": "16530",
            "remote_balance": "0",
            "commit_fee": "3140",
            "commit_weight": "772",
            "fee_per_kw": "2500",
            "unsettled_balance": "0",
            "total_satoshis_sent": "0",
            "total_satoshis_received": "0",
            "num_updates": "0",
            "pending_htlcs": [],
            "csv_delay": 144,
            "private": false,
            "initiator": true,
            "chan_status_flags": "ChanStatusDefault",
            "local_chan_reserve_sat": "354",
            "remote_chan_reserve_sat": "354",
            "static_remote_key": false,
            "commitment_type": "ANCHORS",
            "lifetime": "0",
            "uptime": "0",
            "close_address": "",
            "push_amount_sat": "0",
            "thaw_height": 0,
            "local_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "354",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "19800000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "remote_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "354",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "19800000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "alias_scids": [],
            "zero_conf": false,
            "zero_conf_confirmed_scid": "0",
            "peer_alias": "02769271ddfb03149bc9",
            "peer_scid_alias": "0",
            "memo": "",
            "custom_channel_data": ""
        },
        {
            "active": false,
            "remote_pubkey": "02769271ddfb03149bc9e2e6124bb8d83209284dda3affed0dead73f57b64f45dd",
            "channel_point": "e428fb553769e4e53dbfb9749f8b057272969517ea85c63f46288487bfd62cf0:0",
            "funding_outpoint_id": "f02cd6bf878428463fc685ea1795967272058b9f74b9bf3de5e4693755fb28e4",
            "chan_id": "807041534853120",
            "chan_id_str": "734x1x0",
            "capacity": "100000",
            "local_balance": "0",
            "remote_balance": "96530",
            "commit_fee": "3140",
            "commit_weight": "772",
            "fee_per_kw": "2500",
            "unsettled_balance": "0",
            "total_satoshis_sent": "0",
            "total_satoshis_received": "0",
            "num_updates": "0",
            "pending_htlcs": [],
            "csv_delay": 144,
            "private": false,
            "initiator": false,
            "chan_status_flags": "ChanStatusDefault",
            "local_chan_reserve_sat": "1000",
            "remote_chan_reserve_sat": "1000",
            "static_remote_key": false,
            "commitment_type": "ANCHORS",
            "lifetime": "0",
            "uptime": "0",
            "close_address": "",
            "push_amount_sat": "0",
            "thaw_height": 0,
            "local_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "1000",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "99000000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "remote_constraints": {
                "csv_delay": 144,
                "chan_reserve_sat": "1000",
                "dust_limit_sat": "354",
                "max_pending_amt_msat": "99000000",
                "min_htlc_msat": "1",
                "max_accepted_htlcs": 483
            },
            "alias_scids": [],
            "zero_conf": false,
            "zero_conf_confirmed_scid": "0",
            "peer_alias": "02769271ddfb03149bc9",
            "peer_scid_alias": "0",
            "memo": "",
            "custom_channel_data": ""
        }
    ]
}

NishantBansal2003 force-pushed the append-channel branch from 6cf95f0 to 755d729 Compare December 28, 2024 10:00

ziggie1984 reviewed Dec 29, 2024

View reviewed changes

NishantBansal2003 force-pushed the append-channel branch from 755d729 to 3d19e19 Compare January 1, 2025 10:04

NishantBansal2003 changed the title ~~lnrpc: append listchannels command with the chan_id and short_chan_id~~ lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id Jan 1, 2025

NishantBansal2003 requested a review from ziggie1984 January 1, 2025 10:09

NishantBansal2003 added 3 commits January 3, 2025 22:49

lnwire: update String method for ShortChannelID to use 'x' separator.

c8321d5

Modified the `String` method of `ShortChannelID` to generate a human-readable representation using 'x' as a separator. (block x transaction x output). Signed-off-by: Nishant Bansal <[email protected]>

cmd/lncli: add chan_id_str and funding_outpoint_id in listchannel…

72602fa

…s output. Signed-off-by: Nishant Bansal <[email protected]>

docs: add release notes.

de85265

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 force-pushed the append-channel branch from 90ed477 to de85265 Compare January 3, 2025 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id #9390

lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id #9390

NishantBansal2003 commented Dec 28, 2024

coderabbitai bot commented Dec 28, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

NishantBansal2003 commented Dec 29, 2024

ziggie1984 commented Dec 29, 2024 •

edited

Loading

NishantBansal2003 commented Dec 29, 2024

alexbosworth commented Dec 29, 2024

ziggie1984 Dec 29, 2024 •

edited

Loading

NishantBansal2003 Dec 29, 2024

ziggie1984 Dec 29, 2024

NishantBansal2003 Dec 31, 2024

NishantBansal2003 Dec 31, 2024

ziggie1984 Dec 31, 2024

NishantBansal2003 commented Jan 1, 2025

ziggie1984 commented Jan 1, 2025

NishantBansal2003 commented Jan 2, 2025

ziggie1984 commented Jan 2, 2025

ziggie1984 commented Jan 3, 2025

NishantBansal2003 commented Jan 3, 2025

lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id #9390

Are you sure you want to change the base?

lnrpc+lnwire: append listchannels command with the chan_id and short_chan_id #9390

Conversation

NishantBansal2003 commented Dec 28, 2024

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

coderabbitai bot commented Dec 28, 2024

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

NishantBansal2003 commented Dec 29, 2024

ziggie1984 commented Dec 29, 2024 • edited Loading

NishantBansal2003 commented Dec 29, 2024

alexbosworth commented Dec 29, 2024

ziggie1984 Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

NishantBansal2003 Dec 29, 2024

Choose a reason for hiding this comment

ziggie1984 Dec 29, 2024

Choose a reason for hiding this comment

NishantBansal2003 Dec 31, 2024

Choose a reason for hiding this comment

NishantBansal2003 Dec 31, 2024

Choose a reason for hiding this comment

ziggie1984 Dec 31, 2024

Choose a reason for hiding this comment

NishantBansal2003 commented Jan 1, 2025

ziggie1984 commented Jan 1, 2025

NishantBansal2003 commented Jan 2, 2025

ziggie1984 commented Jan 2, 2025

ziggie1984 commented Jan 3, 2025

NishantBansal2003 commented Jan 3, 2025

CodeRabbit Configuration File (`.coderabbit.yaml`)

ziggie1984 commented Dec 29, 2024 •

edited

Loading

ziggie1984 Dec 29, 2024 •

edited

Loading