RPC: Restore backward compatibility, in multiwallet mode

[luke-jr]

Appears to have been broken by #10849

[ryanofsky]

The check was written this way so explicitly specifying a wallet is required if more than one is loaded. So if, for example, you are writing a script that runs a series of RPCs, and you accidentally forget the wallet option in one of the calls, you get an error message instead of performing an operation or getting data returned from the wrong wallet.

If only one wallet is loaded, providing a wallet parameter should not be necessary, so there should be 100% backwards compatibility with previous releases the way this is written now.

[jonasschnelli]

Didn't we agree that when running Core with multiple wallets one needs to specify the wallet (instead of default using the first one)? IMO we did agree on that.

PR seems NACKish.

[sipa]

Yes, this is intentional and changing it looks like a footgun. NACK

[ryanofsky]

Maybe if #10615 is updated, and there's a way to specify default wallets or restricted wallets per user, that could avoid the need to explicitly specify the wallet on the client side when multiple wallets are loaded on the server side.

[luke-jr]

@jonasschnelli No, I don't recall agreeing to that. It breaks compatibility with existing software (and also means there is literally no future-supported method to access your wallet when multiple wallets are loaded).

The primary target with multiwallet in this release is to support having other wallets loaded in the background, so they're kept up to date with a pruning node. That is broken by this behaviour...

@ryanofsky I can rebase #10615 on top of this, but AFAIK it's too late for 0.15 since it adds new features. This focuses only on the existing bug.

[jonasschnelli]

@jonasschnelli No, I don't recall agreeing to that. It breaks compatibility with existing software (and also means there is literally no future-supported method to access your wallet when multiple wallets are loaded).

Why? 0.15 is the first release of Core with multiwallet. No versions before could run multiple wallets.

The primary target with multiwallet in this release is to support having other wallets loaded in the background, so they're kept up to date with a pruning node. That is broken by this behaviour...

This seems very dangerous. One quickly messes up the argument order making another wallet the "default" wallet which can result in lost funds.

[ryanofsky]

Right now, if you are using bitcoin-cli you can specify rpcwallet=wallet.dat in your config file, and you won't have to specify any wallet on the command line. If you are using another RPC client you can send requests to /wallet/wallet.dat instead of /, which might require some work if you have preexisting code, but at least you shouldn't have to change the content of the requests.

For the future, I suggested in #10868 (comment) that we could add a server-side -defaultwallet option, which would let you explicitly specify a default wallet for the server, instead of having it implicitly chose one based on the order options were parsed (and no way to prevent it if you don't want a default wallet). Rebasing #10615 and adding per-user default or restricted wallets could be another option.

[promag]

Unless a new option is added to allow backward compatibility? For instance, -defaultwallet=...?

Edit: ops @ryanofsky, you said the same above :)

[jnewbery]

NACK for the reasons other people have already mentioned:

• multiwallet is a new feature so backwards-compatibility is not an issue
• having an implicit default wallet in multiwallet is a footgun. Perhaps a future change could add an explicit default wallet as @ryanofsky suggests.

[luke-jr]

That multiwallet is a new feature doesn't make backward compatibility a non-issue. Just because someone enables new feature XYZ doesn't mean existing features should suddenly break completely.

[sipa]

No existing functionality is broken. As long as you only have a single wallet, everything keeps working as it was. Once you use multiple wallets - something that didn't exist before - some things stop working because of safety reasons.

Please stop arguing that this is about backward compatibility, it isn't. There are several ideas about how to accomplish your goal (having wallets loaded in the background with a primary one that RPC operates on). Focus on enabling those features that don't risk people shooting themselves in the foot.

[jonasschnelli]

Closing due to the NACK & comments above.