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

Feat/unified invoices #6421

Merged
merged 11 commits into from Oct 26, 2023

Conversation

chrisguida
Copy link
Contributor

@chrisguida chrisguida commented Jul 21, 2023

Support for tracking payments to fallback addresses (fixes #542)

Currently implemented behind the invoices_onchain_fallback config option.

When this option is enabled, populate new invoices with a P2TR fallback address, and add the script to our txfilter so we can watch for payments to it, and store it in the new invoice_fallbacks child table. Adding fallbacks manually in the invoice rpc call will disable the new tracking behavior, and there is a warning to this effect.

When we detect a payment to one of our fallback addresses, check the invoice_fallbacks table to see if there's an invoice associated with that script. If so, mark the invoice as paid and send out any hooks / notifications for this.

@rustyrussell
Copy link
Contributor

Hmm,. this is too late foir current release, but can we get that bolt11 bugfix in a PR by itself (ideally with a Changelog-Fixed: line?)

@niftynei
Copy link
Collaborator

@rustyrussell done, see #6435

@chrisguida chrisguida marked this pull request as ready for review August 15, 2023 18:33
@rustyrussell rustyrussell added this to the v23.11 milestone Aug 21, 2023
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

This looks great! Complaints are in form, not function!

  • Commits need some cleaning up
  • Remove all the //

lightningd/invoice.c Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
const char *buffer,
const jsmntok_t *obj UNNEEDED,
const jsmntok_t *params)
bool newaddr_inner(struct command *cmd, struct pubkey *pubkey, enum addrtype *addrtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should take ld not cmd, and addrtype is a constant. In effect:

bool issue_new_address(struct lightningd *ld, enum addrtype addrtype, struct pubkey *pubkey);

wallet/db.c Outdated
Comment on lines 203 to 208
{SQL("CREATE TABLE invoice_fallbacks ("
" scriptpubkey BLOB,"
" invoice_id BIGINT REFERENCES invoices(id) ON DELETE CASCADE,"
" PRIMARY KEY (scriptpubkey)"
");"),
NULL},
Copy link
Contributor

Choose a reason for hiding this comment

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

This MUST go at the end of the dbmigrations array!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was moved to the bottom in a later commit.

lightningd/invoice.c Outdated Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
lightningd/invoice.c Outdated Show resolved Hide resolved
wallet/invoices.c Show resolved Hide resolved
doc/lightning-listconfigs.7.md Outdated Show resolved Hide resolved
@chrisguida
Copy link
Contributor Author

Thanks so much for the detailed review, @rustyrussell ! I'll get on this soon :)

@rustyrussell
Copy link
Contributor

Rebased and cleaned up:

  1. Renamed option.
  2. Documented it in lightningd-config.md
  3. Fixed serialization order
  4. Documented new notification field.
  5. Added Changelog lines
  6. Removed many // and fprintf.

@chrisguida
Copy link
Contributor Author

Looks great @rustyrussell, thanks again for the assist on this!

Looks like there are just a couple of things left:

  1. the rest of the review comments
  2. renaming the rest of the unified_invoices references to invoices_onchain_fallback
  3. making the new option default to true

I'll get started on this unless I hear back!

@rustyrussell
Copy link
Contributor

I think I got them all, except for minor internal code things which can be fixed later...

niftynei and others added 10 commits October 26, 2023 14:02
Expand to handle P2TR addresses, use the common script encoding utility
function instead of reimplementing it.
Changelog-Added: Config: `invoices-onchain-fallback` to automatically add an onchain p2tr address to invoices, and allow that for payment.
i.e. `set` may be NULL, so we need to explicitly hand the amount.
We also add an outpoint.
Changelog-Added: JSON-RPC: `listinvoices` new field `paid_outpoint` if an invoice is paid onchain.
@rustyrussell
Copy link
Contributor

YA rebase!

@rustyrussell
Copy link
Contributor

We can make it per-invoice next release, if we want, since it would be perfectly backward compatible. For now it's getting close to release...

@rustyrussell rustyrussell merged commit 88ac2dc into ElementsProject:master Oct 26, 2023
38 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.

c-lightning should support on-chain fallback for BOLT11 invoices
3 participants