allow overriding min/max payment sizes

[johncantrell97]

C= will need to provide a dynamic max payment size based on the current total capacity a user has open with us at the time of the request. In order to do that we need to be able to provide a value at the time of fee request.

I wasn't sure if we should just remove these from the config and make these required when calling opening_fee_params_generated.

[tnull]

So I think for most users these are one-time configured values which means they probably should remain in the config.

I think generally an override pattern is fine here, however, you'll also need to find a solution for the variable amount case that was fixed in #91. This might mean we need to introduce a per-channel/request ServiceConfig that is stored (and will be persisted) in the OutboundJITChannel that allows to override the defaults set via LSPS2ServiceConfig.

[johncantrell97]

I think generally an override pattern is fine here, however, you'll also need to find a solution for the variable amount case that was fixed in #91. This might mean we need to introduce a per-channel/request ServiceConfig that is stored (and will be persisted) in the OutboundJITChannel that allows to override the defaults set via LSPS2ServiceConfig.

Hm yeah. It's tricky because currently we don't even create the OutboundJITChannel until the buy request is handled.

There's really no way to link the GetInfo request/response with the BuyRequest that comes in. So we don't really know what min/max payment we gave them in the GetInfo when they make the BuyRequest. I guess we could just match on counterparty_node_id and just always store the "last one we gave out to this pubkey". I guess another way is to include min/max payment size (or timestamp?) into the computation of the promise we give out and store map from promise to min/max payment and then we can use the promise as a key to fetching the config/min_max payment sizes

Thoughts?

[tnull]

Hm yeah. It's tricky because currently we don't even create the OutboundJITChannel until the buy request is handled.

There's really no way to link the GetInfo request/response with the BuyRequest that comes in. So we don't really know what min/max payment we gave them in the GetInfo when they make the BuyRequest. I guess we could just match on counterparty_node_id and just always store the "last one we gave out to this pubkey". I guess another way is to include min/max payment size (or timestamp?) into the computation of the promise we give out and store map from promise to min/max payment and then we can use the promise as a key to fetching the config/min_max payment sizes

Thoughts?

Ah, yeah, that's kind of what I meant with "you'll also need to find a solution" above. 😁

I think the issue really is that {min,max}_payment_size_msat are (potentially dynamic, per-request) parameters chosen by the LSP and therefore should be clearly communicated and commited to via the OpeningFeeParameters. If we did that, there would also be no requirement to keep state between a client just querying our rates and then actually issuing a buy request. I think this should be fixed in the spec: BitcoinAndLightningLayerSpecs/lsp#82

[johncantrell97]

@tnull Just updated this PR assuming the spec change you proposed gets merged. It looks like it will since it has two "LGTM" comments. I moved the min/max into the promised params and then copy them into outbound jit channel state where they can be used to enforce limits on the payment size that were previously using the config.

This makes it pretty pointless to have these fields in the config so I removed them.

[tnull]

LGTM, although we'll want to wait until the change is actually merged in the spec, which was deferred until the next meeting.

Can you in the meantime squash commits so that we only have changes of the new approach?

[tnull]

LGTM, BitcoinAndLightningLayerSpecs/lsp#82 has just been merged.