Concept ACK, you have to update test/functional/rpc_users.py.

Release notes updated, debug msg changed, and tests updated.

review ACK 044df97

I prefer failing here to be honest. But I guess my dislike for ignoring invalid options is well known.

I prefer failing here to be honest. But I guess my dislike for ignoring invalid options is well known.

In #20461 (comment) I've suggested a -strict (or similar) option.

I mean, if the entire problem here is that users running older bitcoin core might run into this error when upgrading and be confused, I think the error needs to be clearer. All in all it will result in removing cruft from their configuration. Even more, they will likely also be alerted to mistakes they made. Possibly critical security mistakes.
(…why would one add entries on purpose just to be ignored?)

I've suggested a -strict (or similar) option.

Not sure this kind of meta-configuration makes sense. It just adds more code paths where it's better to make a principled decision. And I think bitcoin core should be strict and unambigious about input handling.

I totally agree with @laanwj here. We should fail on invalid configuration.

This still fails, just at runtime when it makes more sense to do so (and without disrupting other usage).

By tolerating unknown extra rpcauth parameters (and ignoring the rpcauth), we can ensure a limited forward compatibility by not forcing users to downgrade their config file to switch back to older versions (perhaps temporarily).

For the past couple releases, hasn't Bitcoin Core refused to start on unknown configuration settings?

$ bitcoind -not-a-real-setting
Error: Error parsing command line arguments: Invalid parameter -not-a-real-setting

Future releases, or spinoffs, are likely to support new configuration settings that old releases will not, so any users including those settings in their configuration files will need to edit those files anyway to use an old or baseline release. Rejecting unknown parameters seems like a simple and logically consistent extension of this previous policy.

The policy of failing on unknown settings is quite beneficial at eliminating the annoyance of figuring out that your configuration is broken and then restarting the node---which is something that can take a long time (e.g. if you have a high db cache or are using an underpowered computer) and which inexperienced users might not know how to do (or might do wrong, e.g. sending SIGKILL rather than SIGTERM, which could result in a need to reindex).

Based on my own misadventures, wrongly configuring a node is a much more common problem than wanting to use a future/spinoff configuration file with an old/baseline node, so I think we should optimize for helping users fix misconfigurations as fast and painlessly as possible.

Closing for now due to controversy

For the past couple releases, hasn't Bitcoin Core refused to start on unknown configuration settings?

On the command line only, not in config files where they would reasonably be expected.

On the command line only, not in config files

Confirmed. That seems to me like an unnecessary half measure. If it's planned to keep it that way, there may indeed be merit in a -strict option that also validates options in the configuration file.

For the configuration files we've been going in the same direction, e.g. rpcport causes failure outside of network sections. That's not quite the same as it's blacklisting known-meaningless settings rather than whitelisting just known-meaningful ones, but in general becoming more strict in configuration files too makes sense.

Indeed, this is a bug that simply hasn't been fixed yet. See #15021

Right, I now remember. The reason rejecting unknown config options isn't done yet is because it would require all binaries to be aware of each other's options so they can be ignored.

That would just be gratuitously breaking things for no reason...

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• common: Disallow calling IsArgSet() on ALLOW_LIST options #17783 by ryanofsky
• refactor: Remove settings merge reverse precedence code #17581 by ryanofsky
• refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580 by ryanofsky
• util: Forbid ambiguous multiple assignments in config file #17493 by ryanofsky

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}