Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

public scid utils #2694

Merged
merged 1 commit into from Jan 25, 2024
Merged

Conversation

Evanfeenstra
Copy link
Contributor

These SCID utils are super useful! Would love to have this file public so we can use it in other crates. The fake_scid stuff is handy as well

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d2242f6) 88.78% compared to head (9a665ca) 88.51%.
Report is 460 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2694      +/-   ##
==========================================
- Coverage   88.78%   88.51%   -0.28%     
==========================================
  Files         112      115       +3     
  Lines       88474    92277    +3803     
  Branches    88474    92277    +3803     
==========================================
+ Hits        78553    81678    +3125     
- Misses       7686     8094     +408     
- Partials     2235     2505     +270     

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

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

Ran into needing a function from here yesterday

@TheBlueMatt
Copy link
Collaborator

Can you expand on the motivation here a bit? A commit with just "public scid utils" isn't very descriptive of what you're trying to do, why you're trying to do it, and how you're going about accomplishing it.

@Evanfeenstra
Copy link
Contributor Author

Can you expand on the motivation here a bit? A commit with just "public scid utils" isn't very descriptive of what you're trying to do, why you're trying to do it, and how you're going about accomplishing it.

Please see my comment in this other PR #2677 (comment). These "lite" users on Sphinx Chat effectively share a channel by creating "virtual private channels" behind a real node. The utils in this file are useful for the server assigning SCIDs (our app actually creates a new identity keypair for each contact or group, and can simultaneously have self-custodial sats via VLS bindings and real channels). These "virtual" accounts can be thought of like "onion mailboxes" whose purpose is to obfuscate activity from the server operator and group chat admins

@TheBlueMatt
Copy link
Collaborator

Lets discuss there what's required, but specific to this I don't think exposing these utils as-is provides any value unless fake_scid_rand_bytes is also exposed from ChannelManager, which I'm not super comfortable doing.

@Evanfeenstra
Copy link
Contributor Author

Any other thought on this @TheBlueMatt? I'm happy to close the issue it if you don't feel comfortable exposing this stuff ... easy enough to copy+paste little utils like scid_from_parts

@TheBlueMatt
Copy link
Collaborator

Apologies for the delay here. Sorry, somehow I'd been confused and thought you specifically needed the fake_scid stuff, rather than only the utils themselves. Indeed, we absolutely should make the utils public, but maybe let's leave fake_scid pub(crate) unless you have an immediate need?

@Evanfeenstra
Copy link
Contributor Author

Apologies for the delay here. Sorry, somehow I'd been confused and thought you specifically needed the fake_scid stuff, rather than only the utils themselves. Indeed, we absolutely should make the utils public, but maybe let's leave fake_scid pub(crate) unless you have an immediate need?

no prob! My need for the fake_scid is related to our discussion on discord a couple weeks ago: https://discord.com/channels/915026692102316113/915026887066132481/1168624836759801928. I am hoping to use the Namespace::OutboundAlias::get_fake_scid() for the "fake channels" created on a service provider node. That way a route hint to a fake channel will be indistinguishable from a route hint to a real (aliased) channel.

@TheBlueMatt
Copy link
Collaborator

Right, if you have a "service provider node" can you not just call ChannelManager::get_intercept_scid and use that?

@Evanfeenstra
Copy link
Contributor Author

Right, if you have a "service provider node" can you not just call ChannelManager::get_intercept_scid and use that?

our node is actually a CLN node, we are using the sendonion RPC command to trigger payments. The Rust code shares a seed with CLN (so onions can be unwrapped) but we do not have an instance of ChannelManager

@@ -65,7 +70,7 @@ pub fn scid_from_parts(block: u64, tx_index: u64, vout_index: u64) -> Result<u64
/// 2) phantom node payments, to get an scid for the phantom node's phantom channel
/// 3) payments intended to be intercepted will route using a fake scid (this is typically used so
/// the forwarding node can open a JIT channel to the next hop)
pub(crate) mod fake_scid {
pub mod fake_scid {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we discussed this on discord prior to the holidays and concluded that all the fake_scid stuff wasn't actually that useful downstream since its really tied to internal LDK behavior and non-ChannelManager-based applications should do their own random SCID generation. However, the module itself is useful to expose. Mind making those changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sorry to take forever on this simple change. Should be good to go now

Copy link

coderabbitai bot commented Jan 21, 2024

Warning

Rate Limit Exceeded

@Evanfeenstra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 46 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 6b0ba8c and 9a665ca.

Walkthrough

The lightning library has been updated to enhance its channel ID handling capabilities. The scid_utils submodule is now publicly accessible, allowing developers to use its functions for creating and parsing short channel IDs. Conversely, the transaction_utils submodule has been made private. A new error enumeration, ShortChannelIdError, has been introduced to handle potential overflow issues in blocks, transaction indices, and output indices. Additionally, the Namespace enum within the fake_scid module has been expanded with more variants.

Changes

Files Change Summary
.../src/util/mod.rs Made scid_utils public and transaction_utils private.
.../src/util/scid_utils.rs Publicized submodule, added short channel ID utilities, introduced ShortChannelIdError enum, expanded Namespace enum in fake_scid module.

Poem

In the land of code where the lightning flows,
A rabbit hopped, tweaking the nodes.
Short IDs parsed with a new-found grace,
Overflow errors now firmly in place. 🌩️🐇💻

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>.
    • Generate unit-tests for this file.
  • 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 tests 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 generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between d2242f6 and f7c8d0d.
Files selected for processing (2)
  • lightning/src/util/mod.rs (2 hunks)
  • lightning/src/util/scid_utils.rs (3 hunks)
Additional comments: 7
lightning/src/util/mod.rs (2)
  • 23-23: The scid_utils module has been made public. This change aligns with the PR's objective to allow the use of utilities across other crates.
  • 23-23: The transaction_utils module has been changed from public to pub(crate). Ensure that this change does not affect any external crates that might be depending on this module.
lightning/src/util/scid_utils.rs (5)
  • 27-31: The ShortChannelIdError enum has been introduced with variants for different overflow errors. This is a good practice for error handling and makes the code more robust.
  • 99-103: New variants have been added to the Namespace enum within the fake_scid module. Ensure that the addition of these variants is reflected wherever the Namespace enum is used.
  • 10-10: The file-level documentation comment has been added to describe the purpose of the utilities in scid_utils.rs. This is a good practice for code maintainability.
  • 27-31: The error handling for scid_from_parts using the ShortChannelIdError enum is well-implemented, providing clear error messages for different types of overflow.
  • 99-103: The addition of new variants to the Namespace enum is consistent with the PR's objectives to support different use cases for fake SCIDs.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks! This is now super trivial and just exposes a handful of nice utilities. There's no reason to wait on a second reviewer.

@TheBlueMatt TheBlueMatt merged commit 51d9ee3 into lightningdevkit:main Jan 25, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants