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

experimental: Add extra-tlv support to keysend and cli option to allow extra TLV types #4610

Merged
merged 11 commits into from
Jun 26, 2021

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jun 18, 2021

This is an experimental change to the way we handle unknown TLVs in the main daemon and the keysend plugin:

  • We add a command line flag that can be used to allow additional even TLV types to be accepted by the main daemon despite it not being able to handle them internally.
  • Based on this allowlist we then add the extra TLVs that were received by with the first HTLC in a payment (this being keysends we will most likely only ever have one total HTLC anyway) to the invoice_payment hook. This allows plugins to extract and react to the extra TLV fields that were not handled internally yet.
  • We also extend the keysend plugin to inject these extra TLV fields into the destination TLV, representing the write path of this change
  • Finally we also change the way that keysend reacts to unknown even TLV types: previously it'd throw up its arms and give up, now it does its preimage extraction and invoice creation logic, then hands off to other plugins. This allows us to incrementally build things like messaging without burdening each plugin to implement this logic over and over again. More in the way of a chain of responsibility rather than simple fail-over

All of this is still guarded behind --experimental-features flag for now since it changes a bit of the logic, but it should only ever impact the noise plugin which was doing this itself.

I kept the two fixups unmerged, since they are just the gen-files caused by the previous commit, to facilitate reviews.

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.

Minor feedback only: mainly that this is 'extra-onion-tlvs', and some JSON formatting. The idea is fine, AFAICT.

/* If we opened an array at all, now is the time to close it. */
if (prevtype != 0)
json_array_end(stream);
json_object_end(stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, JSON schemas has taught me that variable member names are an anti-pattern. So make extra_tlvs an array of objects, with each object "type": u64, "data": hex? Code will be simpler too...

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue here is that the spec technically allows a field to be present multiple times, hence the TLV-type-to-array-of-values indirection. More than happy not to support this :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, then you just have duplicate fields:

[ {"type": 8, "data": "001122"}, {"type": 8, "data": "00112233"} ]

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, they can but it's invalid:

 - MUST order `tlv_record`s in a `tlv_stream` by strictly-increasing `type`,
   hence MUST not produce more than a single TLV record with the same `type`

And:

 - if decoded `type`s are not strictly-increasing (including situations when
   two or more occurrences of the same `type` are met):
   - MUST fail to parse the `tlv_stream`.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I was going by the validation code, should have checked the spec itself :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will amend 👍

@@ -150,6 +163,9 @@ static struct command_result *json_keysend(struct command *cmd, const char *buf,
p_opt_def("exemptfee", param_msat, &exemptfee, AMOUNT_MSAT(5000)),
#if DEVELOPER
p_opt_def("use_shadow", param_bool, &use_shadow, true),
#endif
#if EXPERIMENTAL_FEATURES
p_opt("extra-tlvs", param_extra_tlvs, &extra_fields),
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually use _ not - in JSON, or omit it altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed altogether since I don't like the underscores at all, especially when using the command line.

@cdecker
Copy link
Member Author

cdecker commented Jun 21, 2021

Ok, addressed the comments by removing the dash in the option and by making the dict-of-lists into a list-of-dicts which I agree is much more flexible and easier to code as well.

See the range-diff here

@rustyrussell
Copy link
Contributor

rustyrussell commented Jun 25, 2021

Not sure this should be tagged for v10.0.1 since it's not a bugfix, but pinging @niftynei now?

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.

Ack 0cabe48

@rustyrussell
Copy link
Contributor

rustyrussell commented Jun 25, 2021

Trivial rebase to fix conflict.

Ack 794f8d3

Incoming HTLCs are rejected by the HTLC logic if the payload contains
an even type that `lightningd` doesn't recognize. This is to prevent
us from accidentally accepting a payment that has extra semantics
attached (for example if we get a keysend payment and don't know what
to do with the TLV field containing the message we should reject it,
otherwise the overall semantics of the message delivery fail).
We want to show the fields in the invoice_payment hook.
So far we didn't need this since we were adding them in the correct
order. Now we have multiple possible origins so we better sort them.
Changelog-Added: keysend: You can now add extra TLVs to a payment sent via `keysend`
This is because we might allow the main daemon to transparently handle
these extra TLVs too.
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