BasicBitcoinJsonRPCClient uses hard-coded wallet name

[darwin]

Tracked it down to this line:

eclair/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BasicBitcoinJsonRPCClient.scala

Line 40 in 662e0c4

private val serviceUri = uri"$scheme://$host:$port/wallet/" // wallet/ specifies to use the default bitcoind wallet, named ""

Background story

Today I wanted to fix broken tests in simverse.

Here are my findings (note that I don't follow bitcoin core development that closely, so I may be missing something):

1. bitcoind used to create a default wallet with empty name. It seems to be no longer the case. Newly I have to first create some wallet via bitcoin-cli createwallet <name>. See my commit here.

2. eclair started failing with "bitcoind must have wallet support enabled", I double checked that my bitcoind really had wallet support enabled and started googling the issue. I found the same problem reported here.

3. I looked at the place where the error is raised and decided to investigate further.

4. I found out that the issue is that eclair has hardcoded empty wallet name.

5. bitcoind logic for selecting wallet for RPC request is implemented here.

Possible suggestions

1. BasicBitcoinJsonRPCClient should be configurable to allow wallet name selection (not sure how to propagate this configurability further)

2. By default BasicBitcoinJsonRPCClient should have unspecified name (which is different from empty string), If I understand the rpcwallet.cpp correctly, it would be more flexible in this case not to add "/wallet/" into endpoint url. Bitcoin RPC would then pick the only wallet it has loaded regardless of its name (or return an error about multi-wallet scenario via RPC_WALLET_NOT_SPECIFIED or an error about no-wallet scenario via RPC_METHOD_NOT_FOUND or any other error which could be implemented on bitcoind side in the future). This would be more flexible than assuming default wallet name to be empty string.

3. Secondary problem is that Setup.scala#L150 swallows original RPC error. Even if you didn't resolve points 1 or 2, the code should be updated to include original RPC error. It should not blindly return "bitcoind must have wallet support enabled" which may be misleading, because it assumes particular behaviour of bitcoind which may be subject of future changes.

[darwin]

Just found a mention about this new behaviour in Bitcoin Optech:
https://bitcoinops.org/en/newsletters/2020/09/23/#bitcoin-core-15454

Direct link to the relevant PR:
bitcoin/bitcoin#15454

[t-bast]

Thanks for the detailed issue, I'll have a look at this in more details.

We made some modifications to this in #1334, and during this PR I suggested adding the ability to configure which wallet to use. In the end we didn't merge that code because there were risks of footguns for users, but maybe it's worth considering it again (and accept that users will have to know what they're doing when they change their wallet).

I'll make some tests, but I'm not sure removing the "/wallet" part will work with bitcoind 0.18.1 (and we currently have backwards-compatibility until that version). We'll need to drop backwards-compatibility with older versions of bitcoin core for fee bumping, maybe at that point it will make sense to also drop the "/wallet" suffix.