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

Unified Version Proposal: NO VERSIONING #63

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

ZmnSCPxj-jr
Copy link
Contributor

@ZmnSCPxj-jr ZmnSCPxj-jr commented Oct 15, 2023

What is better than unifying versions? NOT HAVING TO IMPLEMENT VERSIONS AT ALL.

The advantages are:

  • No more versions.
    • Less code: no need to check versions.
    • Less roundtrips: no questioning of versions and selecting the version.
  • Much less amibiguity in specifications:
    • Do we have separate "For implementation", "Stable" etc. states for each VERSION or for each LSPS?

In particular, we SHOULD in fact separate LSPS2 from LSPS4 (YES @SeverinAlexB I AM CHANGING MY MIND ABOUT THIS). The reason is that LSPS2 has fundamentally different flow from LSPS4:

  • LSPS2:
    • Get quotes on how much JIT channels cost.
    • Buy an SCID.
    • Issue invoice
    • Receive payment and get JIT channel opened.
  • LSPS4:
    • Get the permanent SCID
    • Issue invoice.
    • Receive payment notification. If it fits in current inbound liquidity, use it and end the flow. Otherwise continue...
    • Get quotes on how much JIT channels cost.
    • Buy a JIT channel open.
    • Reeive payment and get JIT channel opened.

Thus, we MUST in fact put them as separate specifications completely, because they the fundamental differences in flow.

Even with no-version, we can still upgrade individual specifications:

  • We can add optional parameters to API requests:
    • If not specified, run back-compatibly with previous versions.
    • If specified, new version. If LSP does not recognize, use standard "parameter error" -32602 from JSON-RPC 2.0 so that client knows LSP does not support new version.
  • We can add new fields in API responses or LSP notifications:
    • LSPS0 specifically requires that clients ignore unrecognized fields in responses and notifications.
    • If client knows it but it does not appear, then LSP is on older version.

Basically:

  • If the changes are big enough that they cannot fit in the above back-compatibility, then we really MUST put them in a new LSPS.
  • Otherwise, the changes are small enough that they do not need a separate LSPS but can fit in the same flow as an existing LSPS.

Putting LSPS2 and LSPS4 specifically in separate LSPS numbers also makes it much easier to reason about the LSPS states "For Implementation" "Stable" and "Deprecated". Until LSPS4 is stable, we should not deprecate LSPS2. If we move LSPS4 as LSPS2v2 then we need separate states for each version of an LSPS.

No-version FTW.

@kingonly
Copy link
Collaborator

Ack

@SeverinAlexB
Copy link
Collaborator

SeverinAlexB commented Oct 15, 2023

So this proposal removes versioning from LSPS2 and keeps it at LSPS1? Every LSPS does it's own thing?

I am happy to go the "every breaking change is its own LSPS" route which is very similar to "we run all versions". But having consistency in the approach on how to handle breaking changes is a requirement.

At the end, you always have versions. Implicit or explicit. A section in the LSPS0 on the approach taken is definitely needed though.

Let's merge #62 verbosity with "every breaking change is a new LSPS".

@ErikDeSmedt
Copy link
Contributor

CONCEPT_ACK

To merge this PR we still need to remove versioning from LSPS1 and possibly other places.

@ZmnSCPxj-jr
Copy link
Contributor Author

As noted in Telegram group: We now need to define the -32602 error code (invalid parameters) in detail. Rationale Suppose we have a future version of an RPC method that adds the optional parameter v2param, Then, suppose have an even later version of that RPC method that adds the optional parameter v3param. If the client provides both v2param and v3param, and the LSP returns with a -32602 invalid parameter error (meaning it probably does not support the latest version), how does the client know if the LSP supports v2param but not v3param, or if the LSP does not support v2param nor v3param? Thus, we need to add an error.data object with a field error.data.unrecognized, a JSON array of strings representing the set of unrecognized parameters.

@ZmnSCPxj-jr
Copy link
Contributor Author

updated: Describe unrecognized field of data of -32602 "Invalid params" errors, remove versioning in LSPS1.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Should this PR also remove the Version row in the headers of all LSPSs? Or do we want to keep it to signal spec changes and just keep versioning out of the actual protocol?

@ZmnSCPxj-jr
Copy link
Contributor Author

Or do we want to keep it to signal spec changes and just keep versioning out of the actual protocol?

I think keep it to signal spec changes.

Copy link
Collaborator

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

Apart from my one comment, this PR looks good to me.

LSPS1/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Expand LSPS0 "LSPS Extension and Versioning" section to include client and LSP MUSTs, remove remaining reference in LSPS0 re: versioning API calls in other LSPS.

@ZmnSCPxj-jr ZmnSCPxj-jr force-pushed the no-version branch 3 times, most recently from dc51b0a to 32e851c Compare November 16, 2023 12:46
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Explicitly state in LSPS0 that new LSPS must be back-compatible, remove restriction on options in LSPS1.

@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: Add a few more details in what future LSPS is allowed to do, and what client must do.

LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: rebase to latest, avoid "version" and use "revision" (for specifications) or "feature" (for params/retvals) instead, use "An LSP MUST" instead of "LSPs MUST", move client MUST log to SHOULD log.

Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

Let's do it

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

ACK

Should functionally be good to go, just found a few nits regarding consistency.

LSPS0/README.md Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS0/README.md Outdated Show resolved Hide resolved
LSPS2/README.md Outdated Show resolved Hide resolved
@ZmnSCPxj-jr
Copy link
Contributor Author

Updated: updated as per nits from @tnull

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

ACK

As discussed in the last meeting, we should land this soon and then, as a follow-up, double-check if we want to do something about the "Invalid params" error to make implementor's life easier.

@rbndg rbndg merged commit 7dcdf0a into BitcoinAndLightningLayerSpecs:main Dec 11, 2023
@SeverinAlexB
Copy link
Collaborator

Merged. Decision made in the meeting of the 7th of December 2023.

@ZmnSCPxj-jr ZmnSCPxj-jr deleted the no-version branch December 18, 2023 08:15
ErikDeSmedt added a commit to Blockstream/cln-lsps that referenced this pull request Mar 4, 2024
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.

None yet

7 participants