Support additional TLV records in SendPayentRequest

[araspitzu]

This PR adds the possibility to attach extra TLV records when sending a payment, the feature is meant to be used by plugin developers and experimentally via API (although this PR is missing the API part). It will allow to support features like keysend (invoiceless payments) and/or custom messages attached as TLV record to final onion packet.
I've chosen to not support this feature in trampoline/mpp payments for the moment because i'm not aware of any custom protocol built on top them.
Users willing to experiment with this feature must take care of properly sorting the extra TLV records.

[t-bast]

Great, this doesn't require much code to integrate.
I'd also add a test (if there isn't already one) to verify that the encoding of those user-enriched onions works properly.

[codecov-io]

Codecov Report

Merging #1367 into master will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
+ Coverage   86.30%   86.60%   +0.29%     
==========================================
  Files         119      119              
  Lines        9306     9478     +172     
  Branches      387      397      +10     
==========================================
+ Hits         8032     8208     +176     
+ Misses       1274     1270       -4     

Impacted Files	Coverage Δ
...clair/payment/send/MultiPartPaymentLifecycle.scala	98.27% <100.00%> (ø)
...r/acinq/eclair/payment/send/PaymentInitiator.scala	96.00% <100.00%> (+0.10%)	⬆️
...re/src/main/scala/fr/acinq/eclair/wire/Onion.scala	96.34% <100.00%> (ø)
...rc/main/scala/fr/acinq/eclair/wire/TlvCodecs.scala	100.00% <100.00%> (ø)
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala	89.74% <0.00%> (-5.13%)	⬇️
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala	88.88% <0.00%> (-0.40%)	⬇️
...chain/bitcoind/rpc/BasicBitcoinJsonRPCClient.scala	100.00% <0.00%> (ø)
...nq/eclair/blockchain/electrum/ElectrumWallet.scala	81.00% <0.00%> (+0.25%)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	85.71% <0.00%> (+0.45%)	⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala	92.65% <0.00%> (+0.66%)	⬆️
... and 2 more

[araspitzu]

I've added an extra test for the encoding/decoding of a TLV based FinalPayload with some custom records.

There is another incompatibility that i found when developing keysend against LND, they have a different interpretation of the spec by which the "custom records" (as in those with type > 2^16) don't have be checked against the even/odd rule, in fact the keysend record type is defined as even and is not usable in eclair at the moment.

[t-bast]

they have a different interpretation of the spec by which the "custom records" (as in those with type > 2^16) don't have be checked against the even/odd rule, in fact the keysend record type is defined as even and is not usable in eclair at the moment.

I think it makes sense to have the keysend record be even. If you include it, you really need the receiver to understand it (otherwise your payment won't work).

Are you saying that eclair fails to include this record because we don't allow writing an unknown even type? Or are you saying that eclair will not allow receiving that keysend record (which is the right behavior)?

[araspitzu]

I'm still having some troubles while choosing the right format for the final payload, with the current logic we generate the TLV payload if there is a payment secret or there are user defined TLV records, however when paying an invoice with optional MPP (but we have MPP disabled) we should still generate a legacy payload but this is not the case because the invoice contains the payment secret (see test).

If we match only on the user defined TLV records and append the optional payment secret as TLV record we break the node relayer.

[araspitzu]

Are you saying that eclair fails to include this record because we don't allow writing an unknown even type? Or are you saying that eclair will not allow receiving that keysend record (which is the right behavior)

Yes eclair fails to produce the record because we don't allow writing an unknown even type, is this behavior correct? (I imagined we only want that when reading to make sure we understand the payment data)

[t-bast]

Yes eclair fails to produce the record because we don't allow writing an unknown even type, is this behavior correct?

Ok that's what I feared, I think for unknown records we should allow even types to go through.
Let me have a look at it to see how painful that would be.

[araspitzu]

Ok that's what I feared, I think for unknown records we should allow even types to go through.
Let me have a look at it to see how painful that would be.

So it's okay to allow unknown records to have an even type? If yes then something like this should do the trick

[t-bast]

Here is my proposal: 4c26529

I changed the codecs to allow encoding unknown even tlvs, but still reject decoding such types.
I also did some clean-up/harmonization on the field names.

That means user will be able to use eclair to send payments to endpoints that support keysend, but not to receive. If we want receive support, that needs to be implemented inside eclair and the type needs to be explicitly added. Maybe we can allow decoding unknown even types in the high range as you suggest?

[araspitzu]

Here is my proposal: 4c26529

I changed the codecs to allow encoding unknown even tlvs, but still reject decoding such types.
I also did some clean-up/harmonization on the field names.

That means user will be able to use eclair to send payments to endpoints that support keysend, but not to receive. If we want receive support, that needs to be implemented inside eclair and the type needs to be explicitly added. Maybe we can allow decoding unknown even types in the high range as you suggest?

ACK,i was just merging your proposed change with mines. I think it would be nice if we allow decoding unkown even types in the high range (especially for keysend) and the spec is not very clear here, in fact there is one interpretation by which the tlv_records must adhere the odd/even rule but there is no mention of the rule for the custom records (so they don't support the odd/even rule?). I'll push a change with your proposal + the decoding of unknown even types if in the high range.

[t-bast]

e98af20 is a good first step!

As discussed offline, the next commits should:

• add a whitelist of unknown tlv records to the config
• allow plugins register unknown tlv records that they handle
• pass that whitelist to the TLV codecs so that they allow encoding/decoding these white-listed records

Note that I think that once we have this whitelist, we should use it for both encoding and decoding.
The current code allows encoding any unknown even tlv record, but if we do implement a whitelist mechanism we should revert that and only allow encoding the white-listed tlv records.

[araspitzu]

Latest commit allows to encode unknown even types but not to decode them. I gave a few shots to allow plugins to export a list of whitelisted even TLV types in 217862b although i'm not very satisfied with it. We can have a new plugin hook that is used in Boot/Setup to inject a list of UInt64, however all our codecs are declared in singleton objects and it's impossible to inject anything into it. Possible ways are making it an instance (trait/class) or having the whitelist as implicit parameter, both of them are very impacting on the codebase and i'm not sure we want to go ahead with that.

Some possible alternatives:

• Allow decoding even unknown types in the high range
• Adding the keysend record on the receiving side in eclair (after all it might make it into the spec)
• Not allowing to decode unknown even types at all: this would prevent from writing a keysend plugin
  for the receiving side

[t-bast]

LGTM!