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.

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

Invalid flags after payinvoice's pay_req are being ignored #9440

Open
arturgontijo opened this issue Jan 23, 2025 · 5 comments · May be fixed by #9453
Open

Invalid flags after payinvoice's pay_req are being ignored #9440

arturgontijo opened this issue Jan 23, 2025 · 5 comments · May be fixed by #9453
Labels
enhancement Improvements to existing features / behaviour

Comments

@arturgontijo
Copy link

arturgontijo commented Jan 23, 2025 •
edited
Loading

Background

Flags after lncli payinvoice [command options] pay_req are being ignored.

Your environment

  • version of lnd: lnd-darwin-arm64-v0.18.4-beta
  • which operating system (uname -a on *Nix): Darwin 24.2.0
  • version of btcd, bitcoind, or other backend: [email protected]
  • any other relevant environment details

Steps to reproduce

These fail as expected:

lncli -n=signet payinvoice --last_hop=123 lntbs60030...hj32cppyeeac
>>> [lncli] invalid vertex string length of 3, want 66

lncli -n=signet payinvoice lntbs60030...hj32cppyeeac --last_hop=123
>>> [lncli] invalid vertex string length of 3, want 66

lncli -n=signet payinvoice --last_hop_non_existing=123 lntbs60030...hj32cppyeeac
>>> [lncli] flag provided but not defined: -last_hop_non_existing

But this doesn't:

lncli -n=signet payinvoice lntbs60030...hj32cppyeeac --last_hop_non_existing=123
>>> Payment hash: ...
Description: ...
Amount (in satoshis): 1
Fee limit (in satoshis): 300
Destination: ...
Confirm payment (yes/no): no
[lncli] payment not confirmed

Expected behaviour

Shouldn't we do the same check as we do for sendpayment here?

lncli -n=signet sendpayment --last_hop=123 --pay_req=lntbs60030...hj32cppyeeac
>>> [lncli] invalid vertex string length of 3, want 66

lncli -n=signet sendpayment --pay_req=lntbs60030...hj32cppyeeac --last_hop=123
>>> [lncli] invalid vertex string length of 3, want 66

lncli -n=signet sendpayment --last_hop_non_existing=123 --pay_req=lntbs60030...hj32cppyeeac
>>> [lncli] flag provided but not defined: -last_hop_non_existing

lncli -n=signet sendpayment --pay_req=lntbs60030...hj32cppyeeac --last_hop_non_existing=123
>>> [lncli] flag provided but not defined: -last_hop_non_existing

I think we should output the same error for payinvoice, eg:

lncli -n=signet payinvoice lntbs60030...hj32cppyeeac --last_hop_non_existing=123
>>> [lncli] flag provided but not defined: -last_hop_non_existing

If it makes sense, I can try to implement that check.
@arturgontijo arturgontijo added bug Unintended code behaviour needs triage labels Jan 23, 2025
@arturgontijo arturgontijo changed the title [bug]: [bug]: Invalid flags after payinvoice's pay_req are being ignored Jan 23, 2025
@ViktorTigerstrom
Copy link
Collaborator

The behavior you're describing for payinvoice actually applies to sendpayment as well. For example, if you use the sendpayment command like this:

lncli sendpayment "024e44d23895414408da994c33b790bda76fc138ec6fef3663d786c2fcf524ff96" 100000 "da64ecb9ac16f2b4b60daccf546e1c82f406931ea5b0a097837a00c0c189cf43" 120 "c55d13d3e1d72e3ec049a68b63fb42893581bd3125efad217e12dea5b0639056" --last_hop_non_existing=123

You’ll notice that the --last_hop_non_existing=123 flag is ignored for this command as well. The issue lies in how lncli differentiates between command options (marked with --) and command arguments (passed without --). These are handled and extracted differently in the lncli logic.

As specified in the lncli command descriptions:

lncli payinvoice [command options] pay_req

and

lncli sendpayment [command options] dest amt payment_hash final_cltv_delta pay_addr | --pay_req=R [--pay_addr=H]

it’s described that command options should be included before the command arguments. Therefore, I wouldn’t classify this as a bug. Instead, it appears to be expected behavior based on the way the commands are designed. I'll update this issue by removing the "bug" label and marking it as a potential enhancement instead.

If you'd like to work on a solution that detects non-existing option flags provided after the command arguments, you’re of course more than welcome to do so! :)

@ViktorTigerstrom ViktorTigerstrom added enhancement Improvements to existing features / behaviour and removed bug Unintended code behaviour needs triage labels Jan 27, 2025
@arturgontijo arturgontijo changed the title [bug]: Invalid flags after payinvoice's pay_req are being ignored Invalid flags after payinvoice's pay_req are being ignored Jan 27, 2025
@arturgontijo
Copy link
Author

Oh I see, makes sense!
Ok, I'll take a look on that logic and see if I'm able to come up with something that covers that case.
Thanks!

@arturgontijo
Copy link
Author

@ViktorTigerstrom so the issue is in the urfave/cli (v1) (urfave/cli#1950), they fixed it in v3 (urfave/cli#1987).

I'll check the amount of work to port the current implementation to the v3 and post it here.

@arturgontijo arturgontijo linked a pull request Jan 28, 2025 that will close this issue
Open
8 tasks
@arturgontijo
Copy link
Author

arturgontijo commented Jan 28, 2025 •
edited
Loading

Here it is: #9453

@ViktorTigerstrom
Copy link
Collaborator

Here it is: #9453

Awesome, thanks a lot @arturgontijo 🎉! I'm communicating internally within Lightning Labs to get your PR reviewed whenever possible.

@saubyk saubyk linked a pull request Jan 28, 2025 that will close this issue
cmd: use urfave/cli/v3 #9453
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Improvements to existing features / behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cmd: use urfave/cli/v3 arturgontijo/lnd
2 participants
@ViktorTigerstrom @arturgontijo