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

New deprecation infrastructure #6936

Merged

Conversation

rustyrussell
Copy link
Contributor

Each deprecation has a specific name, and start and end. There's a document explaining the mechanism:

For each deprecation:

  1. The deprecation is listed in doc/developers-guide/deprecations.md, and in the CHANGELOG.md file.
  2. We try to give at least 2 versions before removal.
  3. Then one version where we issue a warning message if we detect a deprecated feature being used (not possible for deprecated Field types).
  4. At least one version where the deprecated feature can be explicit re-enabled using i-promise-to-fix-broken-api-user=FEATURENAME.

Copy link
Collaborator

@adi-shankara adi-shankara left a comment

Choose a reason for hiding this comment

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

The doc changes look good to me.

@rustyrussell rustyrussell force-pushed the guilt/deprecate-slowly branch 6 times, most recently from d0b8e10 to 2e1576c Compare January 24, 2024 12:44
The `struct notification` lost type-safety, but avoided a redundant
string.  The string is better, I think.

Since all notifications now contain an object of same name (some have
deprecated fields outside that), we can add helpers to do that, too.

Also, add some const (easy to do now we're typesafe!)

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Removed: JSON-RPC: `invoice`, `sendonion`, `sendpay`, `pay`, `keysend`, `fetchinvoice`, `sendinvoice`: `msatoshi` argument (deprecated 0.12.0). Use `amount_msat`.
Signed-off-by: Rusty Russell <[email protected]>
This was for "msatoshi" and "amount_msat" in routes.

Signed-off-by: Rusty Russell <[email protected]>
We currently don't have any!

Signed-off-by: Rusty Russell <[email protected]>
Seriously, it's taproot time, let's get rid of p2sh wrapped segwit.

Changelog-Removed: wallet: removal of p2sh-segwit addresses; newaddr won't issue them, we won't watch them for new funds (deprecated in *23.02*)
Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Experimental: Plugins: `funder` option "lease-fee-base-msat" removed (deprecated in v0.11, use "lease-fee-base-sat")
Changelog-Removed: Config: `disable-ip-discovery` (deprecated in v23.02): use `announce-addr-discovered`
Signed-off-by: Rusty Russell <[email protected]>
And clean up weird indent.

Signed-off-by: Rusty Russell <[email protected]>
Each feature has a name, and says when deprecation begins and ends.

There's an API coming to allow you to re-enable on a per-feature basis
even if it's ended (as long as it's not been removed from the code ofc!).

Default end is 6 months after deprecation, i.e. we complain about it
at that point, if we can detect its use.

e.g, a standard deprecation in v24.05:

v24.02: allowed
v24.02 with mods: allowed

master after v24.02: allowed unless deprecated APIs disabled.
v24.05: allowed unless deprecated APIs disabled.
v24.08: allowed unless deprecated APIs disabled.

v24.11: allowed unless deprecated APIs disabled, but logs at BROKEN level.

v25.02: allowed only if --i-promise-to-fix-broken-api-user=FEATURE.
v25.05: code is actually removed.

Signed-off-by: Rusty Russell <[email protected]>
Updating this every release was just busywork, and it turns out we don't actually
care: if something is marked deprecated we want to make it optional, whenever
it is for, and the only real test is if it was added before our lowest-supported
version we can consider it non-optional.

Signed-off-by: Rusty Russell <[email protected]>
Don't assume removal is +6 months, but have a start deprecation/end support range.

Signed-off-by: Rusty Russell <[email protected]>
`struct column` has a dynamically allocated member, which is neater if
it is a tal object itself (we allocated the member off the array, instead).
We're about to add two new members, so clean this up first.

Signed-off-by: Rusty Russell <[email protected]>
When we allow deprecation to be set per-connection, we need to
generalize this approach.  Instead of filtering out deprecated
fields at schema loading, we need to load them, then refuse
to serve deprecated fields on a per-request basis.

Signed-off-by: Rusty Russell <[email protected]>
This allows the user to specify the feature *by name*, and hopefully
complain to the developer to fix their code, knowing it will be removed entirely
in the next release!

Changelog-Added: config: `i-promise-to-fix-broken-api-user` allows for a one-release re-enablement of long-deprecated features.
Signed-off-by: Rusty Russell <[email protected]>
…ok()

Generic helpers for libplugin and lightningd.

Signed-off-by: Rusty Russell <[email protected]>
In this case the cmd is `sql` but the field we're talking about is from
a different command, so we need a new libplugin API.

Note: there are still no deprecations in any tables used by `sql`, so this
is a bit moot for now.

Signed-off-by: Rusty Russell <[email protected]>
This command allows more fine-grained testing, without having to change the config of the
lightning node.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `deprecations` to enable/disable deprecated APIs from this caller.
This infrastructure is use by both libplugin and lightningd.

Signed-off-by: Rusty Russell <[email protected]>
… versions.

We still accept boolean: the plugin may not want to commit to a deprecation schedule.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugins: rpcmethods and options can set `deprecated` to a pair of version strings, not just a boolean.
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugin: options and commands can specify deprecation start (and optional end) versions.
Type-checking in Python is so loose you could already do this, but this updates the
mypy type annotations.

Signed-off-by: Rusty Russell <[email protected]>
I did some CHANGELOG and git digging to see when these were deprecated, and
some were very old (v0.8.2!).  But since they didn't warn users loudly, I
chose to do so this release only.

I renamed ld's `deprecated_apis` to `deprecated_ok` to make sure I
caught them all.

Signed-off-by: Rusty Russell <[email protected]>
This means it now covers plugin parameters too.

Signed-off-by: Rusty Russell <[email protected]>
Unfortunately, this is awkward: we just copy through most requests,
so we can't easily add a "deprecation" field to each one.  So we do
a notification if the next command has a different deprecation status
than the global one, but that requires opt-in from the plugin.

We didn't previously document the subscriptions array, so do that now.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: Plugins: `deprecated_oneshot` notifiction subscription to change deprecated status for a single command.
In this case we don't have a matching "command", so we need a special
API.

Signed-off-by: Rusty Russell <[email protected]>
And we don't need to handle 0.9 lightningd which didn't include
allow-deprecated-apis in getmanifest call.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell rustyrussell merged commit 040af90 into ElementsProject:master Jan 26, 2024
38 checks passed
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.

2 participants