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

Revert "lightningd: always require "jsonrpc": "2.0" in request." #5783

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

rustyrussell
Copy link
Contributor

This reverts commit 43b037a.

@NicholasDorier says BTC Payserver still wants this for another year or so.

Changelog-Added: JSON-RPC: reverts requirement for "jsonrpc" "2.0" inside requests (still deprecated though, just for a while longer!)
Fixes: #5782

This reverts commit 43b037a.

Nicholas Dorier says BTC Payserver still wants this for another year
or so.

Changelog-Added: JSON-RPC: reverts requirement for "jsonrpc" "2.0" inside requests (still deprecated though, just for a while longer!)
@rustyrussell rustyrussell added this to the v22.11.1 milestone Dec 5, 2022
@NicolasDorier
Copy link
Collaborator

I think we are fine without it.
I tried to add jsonrpc 2.0 to our library, then tested against old node. Old node accepted it fine. I was worried to be in a situation where our library wouldn't be able to support old nodes + new nodes because of this.

I believe allowing it to be bypassed with allow-deprecated-api is the right call though, even if it doesn't seem we need it.

There are several projects in our deployment stack such as Charge, Spark and RTL that may need it, but I didn't had time to check.

@rustyrussell
Copy link
Contributor Author

Agreed, I say merge this, it's simple (it's @cdecker decision, as Release Captain), and we can revisit once everyone has reasonably upgraded BTCPayServer.

@cdecker cdecker enabled auto-merge (rebase) December 6, 2022 09:43
@cdecker
Copy link
Member

cdecker commented Dec 6, 2022

ACK 7255cd0

@cdecker cdecker merged commit 1d4f7f0 into ElementsProject:master Dec 6, 2022
@NicolasDorier
Copy link
Collaborator

@rustyrussell I am suprised allow-deprecated-apis default is true.
I didn't see this change before because my tests wasn't setting this.
In bitcoin core, this is the reverse, and I believe this make sense: as integrator,

  1. you bump,
  2. you test,
  3. you see it blows up,
  4. set the flag for now
  5. migrate to the new thing later

@cdecker
Copy link
Member

cdecker commented Dec 7, 2022

It is defaulting to true because most users will not want their nodes to just die when a deprecated feature is being used. This is in contrast to app devs which should always run with it set to false, so they can be notified early about any issues.

If we were to set it to false by default we'd get the worst experience for users, and no heads up for app devs.

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.

JSON-RPC should not require "jsonrpc" "2.0" in deprecated mode yet.
3 participants