Implement invalid-parameters-error for lightning-liquidity

[martinsaposnic]

Closes #3476

As per spec, https://github.com/BitcoinAndLightningLayerSpecs/lsp/blob/main/LSPS0/README.md#-32602-invalid-parameters-error, if an unknown param is received, error code -32602 must be returned, and the unrecognized parameter must be informed to the caller.

Other implementors before mentioned that it's not trivial to do with serde, if it's too cumbersome we might want to bring it back to the spec group.

I tried a few things (see Notes) and I agree, it's not trivial, mainly because serde does not provide this out of the box.

I also think that this is probably too much complexity for the thing that is accomplished, but I would like others to chip in.

Notes:

things I tried:

• import a new crate like serde-ignore or similar, or copy their approach. I don't really trust these kinds of crates
• serde's deny_unknown_fields is ok but it does not give you a structured response with the unknown fields that were unknown so we can't use it
• add an unknown_fields field with #[serde(flatten)] on all structs. the unknown fields would be deserialized on this new field. kind of hard to maintain and adds a lot of noise. I think it added some complexity with nested structs.
• deserialize, then serialize, and then check diff (which is what I did in a derive macro)

[ldk-reviews-bot]

🎉 This PR is now ready for review!
Please choose at least one reviewer by assigning them on the right bar.
If no reviewers are assigned within 10 minutes, I'll automatically assign one.
Once the first reviewer has submitted a review, a second will be assigned if required.

[codecov]

Codecov Report

❌ Patch coverage is 91.33574% with 72 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.79%. Comparing base (bbfc694) to head (db4c5ac).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps0/ser.rs	79.03%	44 Missing and 4 partials ⚠️
lightning-liquidity/src/lsps5/msgs.rs	94.44%	9 Missing ⚠️
lightning-liquidity/src/lsps2/msgs.rs	96.55%	8 Missing ⚠️
lightning-liquidity/src/lsps1/msgs.rs	95.70%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4039      +/-   ##
==========================================
+ Coverage   88.74%   88.79%   +0.04%     
==========================================
  Files         176      176              
  Lines      129012   129703     +691     
  Branches   129012   129703     +691     
==========================================
+ Hits       114491   115166     +675     
- Misses      11926    11945      +19     
+ Partials     2595     2592       -3     

Flag	Coverage Δ
fuzzing	21.90% <0.00%> (-0.01%)	⬇️
tests	88.62% <91.33%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinsaposnic]

converted to draft because there are some edge cases that are not covered.

[tnull]

Hmm, this is great work, but I do wonder whether we really should take on the around +1400/-250 LoC just to implement this error type.

For context, there was some discussion in the spec group around the potential complexity increase when the invalid-params error was first introduced, and we concluded that we'd attempt to implement it but revisit if it would be too painful to implement/support.

FWIW, I think we're pretty much at the 'too painful' level here, so I'd actually lean in favor of changing the spec here to not require the error to exactly return which fields it wasn't able to parse.

@martinsaposnic It seems that you agree with that conclusion, even though you already went and did the work now?

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

[martinsaposnic]

@martinsaposnic It seems that you agree with that conclusion, even though you already went and did the work now?

I do agree with that conclusion. I mostly wanted to give it a try in case I found a simpler approach, but at least now we have a concrete reference showing why it's complex, and it also gives us some justification to push back on the spec.

I'm happy to open an issue or PR to BLIP-50 with proposed changes if you think that's the right next step @tnull

[tnull]

I'm happy to open an issue or PR to BLIP-50 with proposed changes if you think that's the right next step @tnull

Cool, yes, please feel free to open a PR to bLIP-50 to simplify the error (I think we should be able to drop the dedicated section entirely?)

[tnull]

Btw, when you then update this PR for the simplified version, mind adding a test to make sure we simply ignore the additional data attachment in the error message, just to make sure we stay compatible with the original spec (although I doubt there is any chance anybody ever implemented this, tbh)?

[martinsaposnic]

we simply ignore the additional data attachment in the error message

just to clarify, the proposed BLIP change would be either:

a) we ignore any unknown params and still succeed as long as deserialization works, OR
b) we fail the request if unknown params are found, but without reporting which fields were unknown.

both options are super easy to implement, just want to make sure we are on the same page

@tnull

[tnull]

we simply ignore the additional data attachment in the error message

just to clarify, the proposed BLIP change would be either:

a) we ignore any unknown params and still succeed as long as deserialization works, OR b) we fail the request if unknown params are found, but without reporting which fields were unknown.

both options are super easy to implement, just want to make sure we are on the same page

@tnull

I think we should do b) as a) might lead to unexpected outcomes. My point above was that, if somebody ever sends us the 'old style' invalid-parameters-error with reporting the unknown fields, we should still treat it just like any other invalid-parameters-error (this is likely already the case and trivial, might still be good to double check).

[martinsaposnic]

closing this. will reopen once I rework this and implement the simpler logic described above