Encode fields in `opening_fee_params` promise

JssDWt commented 7 months ago

This PR makes the opening_fee_params objects extensible for future backward compatible upgrades. Fixes https://github.com/BitcoinAndLightningLayerSpecs/lsp/issues/94

The problem

If this spec ever decides to add an additional field in the opening_fee_params object, this upgrade path is impossible to roll out in the current spec, without placing too much logic on the client side.

LSP is upgraded, client is not

The LSP constructs a new opening_fee_params object, containing the new field. Commits to the object in the promise.
The client deserializes the object, not knowing about the new field
The client serializes the object again and sends it back to the LSP without the new field
The LSP is now unable to verify the promise

Note that it would be possible for the client to send back the opening_fee_params object using exactly the bytes it received from the LSP. But this is a 'hard' constraint, because this would force the client to pass to the UI layer the deserialized object plus an additional field containing the raw opening_fee_params bytes.

The solution

Encode all fields into the promise itself. This way, the promise alone is enough for the LSP to retrieve what it committed to. And the client really only needs to send back the promise to the LSP.

LSP is upgraded, client is not

The LSP constructs a new opening_fee_params object, containing the new field. Commits to the object in the promise.
The client deserializes the object, not knowing about the new field.
The client sends promise to the LSP, this promise contains the new field encoded.
The LSP is now able to verify the promise.

Notes

I removed the constraint that the LSP is not allowed to add any fields to the opening_fee_params object, because the LSP could commit to any data unknown to the client, even outside of these communications. SO I don't think it adds much value to have this constraint in the spec.
I modified the name of the invalid_opening_fee_params error to invalid_promise, keeping the same error number
This is a breaking change, we discussed during the in-person meeting in Viareggio, this would be the last breaking change to LSPS2. Thankfully this change may help avoid breaking changes in the future when LSPS2 may already be used by multiple implementations.

JssDWt commented 7 months ago

Note that the proposed change makes the actual buy request a human unreadable message and also introduces additional complexity and asymmetry in the sense that the client now wouldn't just mirror back the object it receives.

Could you elaborate what you mean by this? In my view the complexity is in passing back the human-readable/deserializable message back to the LSP.

tnull commented 7 months ago

Could you elaborate what you mean by this? In my view the complexity is in passing back the human-readable/deserializable message back to the LSP.

Well you still need to keep the human-readable message around to calculate the fee, validate the LSP didn't withhold more than we expect it to, and show to the user which of the opening fee params was picked. As the client still won't to know how to deal with any new fields when the LSP upgrades, in my view nothing about the 'problem' is actually resolved, but we now reply with a human-unreadable message that is different, i.e., needs to be implemented as yet another object for serialization, where previously we could just reuse the one from GetInfoResponse (granted, not a lot additional complexity).

tnull commented 7 months ago

In any case, while I'm still a bit dubious if this gains us much, I definitely won't block this PR as it also doesn't have a lot of drawbacks. Happy to ACK if others think this is beneficial.

SeverinAlexB commented 7 months ago

I reviewed the changes and I agree with tnull's comment https://github.com/BitcoinAndLightningLayerSpecs/lsp/pull/111#pullrequestreview-1993877624. The changes look good but I am unsure if necessary. Because we're already working on the successor of this spec (LSPS4), I am inclined to error on simplicity and hope that there will be no breaking change in LSPS2. On the other hand, I don't mind acking this if Jesse think this is necessary.

So TLDR: I have no strong opinion either way.

JssDWt commented 7 months ago

It was discussed in the LSP spec group call that

changing the opening_fee_params fields would have effects on fee calculation for clients, so backward compatible changes are 'hard' anyway.
if it is decided a field needs to be added to the opening_fee_params object, we can decide to encode the fields into the promise then. This still makes the LSP backwards compatible. So we don't have to break anything now.

So I'm closing this PR.

BitcoinAndLightningLayerSpecs / lsp