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

Feat/unified invoices #6421

Merged
merged 11 commits into from
Oct 26, 2023
Merged

Feat/unified invoices #6421

merged 11 commits into from
Oct 26, 2023

Conversation

chrisguida
Copy link
Contributor

@chrisguida chrisguida commented Jul 21, 2023 •
edited
Loading

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.

@chrisguida chrisguida force-pushed the feat/unified-invoices branch from f07bc60 to f5604f5 Compare July 21, 2023 22:50
@niftynei niftynei force-pushed the feat/unified-invoices branch from 64d2e9f to 8ca8dca Compare July 24, 2023 06:10
@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
Contributor

@rustyrussell done, see #6435

@chrisguida chrisguida force-pushed the feat/unified-invoices branch from e4a7424 to 283f5b3 Compare August 1, 2023 03:36
@chrisguida chrisguida marked this pull request as ready for review August 15, 2023 18:33
@chrisguida chrisguida requested a review from cdecker as a code owner August 16, 2023 19:12
@rustyrussell rustyrussell added this to the v23.11 milestone Aug 21, 2023
rustyrussell
rustyrussell reviewed Sep 19, 2023
View reviewed changes
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
wallet/walletrpc.c Outdated
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

@chrisguida chrisguida Sep 19, 2023 •
edited
Loading

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.

@rustyrussell rustyrussell force-pushed the feat/unified-invoices branch from 0fac5fe to 6c3b2b1 Compare October 25, 2023 03:53
@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 rustyrussell force-pushed the feat/unified-invoices branch from 6c3b2b1 to 009f525 Compare October 26, 2023 02:06
@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
@niftynei @rustyrussell
fallback: json add fallback cleanup
f269974 
Expand to handle P2TR addresses, use the common script encoding utility
function instead of reimplementing it.
@chrisguida @rustyrussell
wallet: add newaddr_inner() helper
df26825
@chrisguida @rustyrussell
bitcoin: expose fmt_bitcoin_txid
34b83e8
@chrisguida @rustyrussell
add invoices-onchain-fallback config option and warnings
d5fdd4c 
Changelog-Added: Config: `invoices-onchain-fallback` to automatically add an onchain p2tr address to invoices, and allow that for payment.
@chrisguida @rustyrussell
wallet: add invoice_fallbacks table and paid_outpoint to invoices
fc8d9a4
@chrisguida @rustyrussell
track fallbacks for invoices, pt 1
6fe2804
@chrisguida @rustyrussell
add autogenerated fallbacks (not custom fallbacks) to invoice_fallbac…
f9388f5 
…ks table
@chrisguida @rustyrussell
lightningd: adapt invoice_try_pay declaration for onchain payment.
32b678c 
i.e. `set` may be NULL, so we need to explicitly hand the amount.
We also add an outpoint.
@chrisguida @rustyrussell
mark invoice as paid when we detect an on-chain payment to a fallback
d3ea451
@chrisguida @rustyrussell
display paid_outpoint on invoices when present
3781993 
Changelog-Added: JSON-RPC: `listinvoices` new field `paid_outpoint` if an invoice is paid onchain.
@rustyrussell
Copy link
Contributor

YA rebase!

@rustyrussell rustyrussell force-pushed the feat/unified-invoices branch from 009f525 to c760744 Compare October 26, 2023 03:34
@rustyrussell rustyrussell force-pushed the feat/unified-invoices branch from c760744 to bab6ca6 Compare October 26, 2023 03:35
@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
Reviewers

@rustyrussell rustyrussell rustyrussell left review comments

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v23.11
Development

Successfully merging this pull request may close these issues.

c-lightning should support on-chain fallback for BOLT11 invoices
3 participants
@chrisguida @rustyrussell @niftynei