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

BOLT update, including payment metadata support #5086

Conversation

rustyrussell
Copy link
Contributor

Thanks to Joost, bolt11 spec now supports metadata (see lightning/bolts#912). To get there, I updated the spec (in stages, there were other changes).

We don't issue invoices with metadata, but we do pay them properly if asked.

@rustyrussell rustyrussell added this to the v0.11 milestone Mar 11, 2022
@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch 3 times, most recently from 707be20 to 396bc96 Compare March 15, 2022 03:26
@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 396bc96 to 7bbe38e Compare March 22, 2022 04:00
@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 7bbe38e to ace8213 Compare March 24, 2022 04:01
@rustyrussell
Copy link
Contributor Author

Trivial rebase

@rustyrussell
Copy link
Contributor Author

Rebase, crash fix for offers (which don't have payment_metadata yet!).

@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 730a995 to 6d142ff Compare March 28, 2022 02:54
@rustyrussell
Copy link
Contributor Author

OK, this time I added a much bigger expiry to the canned invoice!!

@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 6d142ff to 1402e90 Compare March 29, 2022 00:22
@rustyrussell
Copy link
Contributor Author

Trivial rebase, to fix simple conflict and get master flake fixes.

@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch 2 times, most recently from 21707f2 to 78b5a37 Compare March 30, 2022 04:01
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Regenerate from current BOLTS via `make extract-bolt-csv`

1. The remote_addr field was added manually into peer_wire.csv: this
   needs to be a patch otherwise it vanishes on regen.
2. We never brought into the channel_disabled fields, because it was
   too much hassle (we never actually generate this!).  Do it now.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
… payment_secret.

The signatures on the new examples are sometimes different from what we produce though?
They're valid, however.

And one example has an unneeded feature 5-bit; it's not *wrong*, but
it's not optimal.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…outputs.

This doesn't have an effect now (except in experimental mode), but it
will when we support anchors.  So we deprecate the use of those in the
close command too.

For experimental mode we have to avoid using p2pkh; adapt that test.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: JSON-RPC: `shutdown` no longer allows p2pkh or p2sh addresses.
We do this (send warnings) in almost all cases anyway, so mainly this
is a textual update, but there are some changes:

1. Send ERROR not WARNING if they send a malformed commitment secret.
2. Send WARNING not ERROR if they get the shutdown_scriptpubkey wrong (vs upfront)
3. Send WARNING not ERROR if they send a bad shutdown_scriptpubkey (e.g. p2pkh in future)
4. Rename some vars 'err' to 'warn' to make it clear we send a warning.

This means test_option_upfront_shutdown_script can be made reliable, too,
and it now warns and doesn't automatically close channel.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 75b0af1 to 5f89486 Compare March 31, 2022 09:17
@rustyrussell
Copy link
Contributor Author

Trivial rebase, and a real fix for subd memleak report!

Ack 5f89486

@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 5f89486 to 9f4dfc8 Compare March 31, 2022 23:57
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And document the missing arguments.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Protocol: `pay` (and decode, etc) supports bolt11 payment_metadata a-la lightning/bolts#912
Just typo fixes and the like.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
After this, we can exactly reproduce the vectors (in DEVELOPER mode).

1. Move payment_metadata position to match test vector.
2. Create flag to suppress `c` field production.
3. Some vectors put secret before payment_hash, hack that in.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I have a separate branch which fixes this race properly, but it's not anything
to do with this PR.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/bolt11-add-payment_metadata branch from 9f4dfc8 to 91932be Compare April 1, 2022 04:51
@rustyrussell
Copy link
Contributor Author

rustyrussell commented Apr 1, 2022

Rebase again: we didn't set the new field to NULL in the newly re-introduced legacy onion path.

Removed subd memleak report fix, since it made more problems (another branch coming for this!)

Ack 9f4dfc8

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

ACK 91932be

@@ -684,11 +685,11 @@ static struct command_result *json_close(struct command *cmd,
channel->peer->their_features,
OPT_SHUTDOWN_ANYSEGWIT);
if (!valid_shutdown_scriptpubkey(channel->shutdown_scriptpubkey[LOCAL],
anysegwit)) {
anysegwit, !deprecated_apis)) {
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that we'll always use zerof-fee anchor outputs once we phase out deprecated apis, as if that feature were mandatory, is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future work! This flag will be always set when we implement it, which may Just Happen if we're slow enough:)

@@ -911,7 +911,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
&pending->node_id_2,
&pending->bitcoin_key_1,
&pending->bitcoin_key_2)) {
err = towire_warningfmt(rstate, NULL,
warn = towire_warningfmt(rstate, NULL,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: indentation of the next two lines.

@@ -991,7 +991,7 @@ u8 *handle_channel_announcement(struct routing_state *rstate,
}

/* Note that if node_id_1 or node_id_2 are malformed, it's caught here */
err = check_channel_announcement(rstate,
warn = check_channel_announcement(rstate,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: indentation

@rustyrussell rustyrussell merged commit 01e5f18 into ElementsProject:master Apr 1, 2022
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.

2 participants