Skip to content

Invoice chanman utility #895

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 29, 2021
Merged

Invoice chanman utility #895

merged 5 commits into from
Apr 29, 2021

Conversation

@valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Apr 23, 2021 •
edited
Loading

TODO: tests

Based on #893

@codecov
Copy link

codecov bot commented Apr 23, 2021 •
edited
Loading

Codecov Report

Merging #895 (7794708) into main (3be185a) will decrease coverage by 0.01%.
The diff coverage is 64.28%.

❗ Current head 7794708 differs from pull request most recent head 4c7be7e. Consider uploading reports for the commit 4c7be7e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   90.38%   90.36%   -0.02%     
==========================================
  Files          57       58       +1     
  Lines       29500    29505       +5     
==========================================
- Hits        26663    26662       -1     
- Misses       2837     2843       +6
Impacted Files Coverage Δ
lightning-invoice/src/de.rs 80.99% <ø> (-0.15%) ⬇️
lightning-invoice/src/lib.rs 83.28% <ø> (-0.06%) ⬇️
lightning-invoice/src/ser.rs 91.76% <ø> (-0.19%) ⬇️
lightning/src/chain/channelmonitor.rs 95.45% <ø> (ø)
lightning/src/chain/keysinterface.rs 93.01% <0.00%> (-0.62%) ⬇️
lightning/src/ln/chan_utils.rs 97.34% <ø> (ø)
lightning/src/ln/chanmon_update_fail_tests.rs 97.64% <ø> (ø)
lightning/src/ln/channel.rs 87.41% <0.00%> (-0.04%) ⬇️
lightning/src/ln/channelmanager.rs 83.38% <ø> (-0.05%) ⬇️
lightning/src/ln/functional_test_utils.rs 95.06% <ø> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3be185a...4c7be7e. Read the comment docs.

TheBlueMatt
TheBlueMatt reviewed Apr 24, 2021
View reviewed changes
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.

Looks good.

@TheBlueMatt TheBlueMatt mentioned this pull request Apr 26, 2021
Merged
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 3 times, most recently from fec5fa5 to 7d5ebb2 Compare April 28, 2021 16:50
@valentinewallace valentinewallace marked this pull request as ready for review April 28, 2021 16:50
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 4 times, most recently from 78b8298 to 4d5f112 Compare April 28, 2021 18:47
jkczyz
jkczyz reviewed Apr 28, 2021
View reviewed changes
TheBlueMatt
TheBlueMatt reviewed Apr 28, 2021
View reviewed changes
@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Apr 28, 2021

Can now be rebased after the merge of #893.

@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 2 times, most recently from b3d0434 to 5dfb86a Compare April 28, 2021 21:43
TheBlueMatt
TheBlueMatt reviewed Apr 28, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 2 times, most recently from 8cd714c to 5a478c7 Compare April 28, 2021 22:16
ariard
ariard reviewed Apr 28, 2021
View reviewed changes
lightning-invoice/src/lib.rs Outdated
L::Target: Logger,
{
// Marshall route hints.
let our_channels = channelmanager.list_usable_channels();
Copy link

@ariard ariard Apr 28, 2021

Choose a reason for hiding this comment

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

IIUC this utility returns as routing hints the complete set of our channels, including the non-announced subset one ?

Sounds pretty harmful for privacy, maybe the utility should have a boolean all or otherwise a Vec<short_channel_id> to precise which ones to disclose ?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Apr 29, 2021

Choose a reason for hiding this comment

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

I think this is the right default to this - in most cases if we're a "client" node we only have one or two channels anyway (and in non-"client" cases we have several channels that are public), and the "real" fix is to generate random short channel ids.

Copy link

@ariard ariard Apr 29, 2021

Choose a reason for hiding this comment

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

Agree for the "client" node but for a big hub you will disclose all your private channels with spoke if you're also a merchant which is quite the case of a bunch of such nodes. Though opening an issue for now, we'll adjust in function of users feedback.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Apr 29, 2021

Choose a reason for hiding this comment

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

Right, we ultimately need some heuristic of "we have > 5 public channels, we shouldn't include any route hints at all", probably.

ariard
ariard reviewed Apr 28, 2021
View reviewed changes
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Otherwise sounds good to me :)

jkczyz
jkczyz reviewed Apr 29, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 3 times, most recently from 5dd106d to 9ab3a9c Compare April 29, 2021 16:29
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Apr 29, 2021

Oops, need to switch sign_invoice to return a Result. Otherwise I think this is roughly what it's gonna look like.

@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 2 times, most recently from 1e34c16 to e2f81e3 Compare April 29, 2021 17:01
jkczyz
jkczyz reviewed Apr 29, 2021
View reviewed changes
TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from 3c58411 to fbdb7a1 Compare April 29, 2021 18:12
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Apr 29, 2021

Gonna have lunch then come back to this

TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
@TheBlueMatt TheBlueMatt added this to the 0.0.14 milestone Apr 29, 2021
TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch 3 times, most recently from c1f9f87 to 50c0a99 Compare April 29, 2021 20:36
TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from 7794708 to a614b9b Compare April 29, 2021 20:44
jkczyz
jkczyz reviewed Apr 29, 2021
View reviewed changes
TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
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.

Aside from Jeff's comments and #895 (comment) this LGTM.

@TheBlueMatt TheBlueMatt mentioned this pull request Apr 29, 2021
Merged
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from a614b9b to 6173c2c Compare April 29, 2021 22:37
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from 6173c2c to a6a6130 Compare April 29, 2021 22:39
jkczyz
jkczyz approved these changes Apr 29, 2021
View reviewed changes
TheBlueMatt
TheBlueMatt requested changes Apr 29, 2021
View reviewed changes
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from a6a6130 to 8385915 Compare April 29, 2021 23:04
@valentinewallace
Copy link
Contributor Author

valentinewallace commented Apr 29, 2021

Gah my bad, fixed

TheBlueMatt
TheBlueMatt reviewed Apr 29, 2021
View reviewed changes
@valentinewallace
Add utility to create an invoice using the ChannelManager
4c7be7e 
This also allows the ChannelManager to track information for inbound payments
to check the PaymentSecret on receive.
@valentinewallace valentinewallace force-pushed the invoice-chanman-utility branch from 8385915 to 4c7be7e Compare April 29, 2021 23:14
@TheBlueMatt TheBlueMatt merged commit 3b67be2 into lightningdevkit:main Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devrandom devrandom devrandom left review comments

@TheBlueMatt TheBlueMatt TheBlueMatt requested changes

@jkczyz jkczyz jkczyz approved these changes

+1 more reviewer

@ariard ariard ariard left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.14

Development

Successfully merging this pull request may close these issues.

5 participants

@valentinewallace @TheBlueMatt @devrandom @jkczyz @ariard