wallet: Add deriveHDKey interface #36070

This case passes even when the derivation comes from a different key: origin.path just echoes the argument, and IsValid() holds for any derived key. I made SelectHDKey return a CExtKey the wallet doesn't hold and all six cases still passed, while wallet_derivehdkey.py failed on the same mutation.

Comparing against the wallet's own active HD key would catch this, the way bip32_derive_ext_key does for DeriveExtKey:

    LOCK(wallet->cs_wallet);
    const auto active{wallet->GetHDPubKeys(CWallet::HDKeyFilter::Active)};
    BOOST_REQUIRE_EQUAL(active.size(), 1U);
    const auto xprv{wallet->GetExtKey(active.begin()->first)};
    BOOST_REQUIRE(xprv);
    const auto expected{DeriveExtKey(*xprv, path)};
    BOOST_REQUIRE(expected);
    BOOST_CHECK(result->first == expected->first.Neuter());
    BOOST_CHECK(result->second == expected->second);

Is this branch reachable? EnsureWalletIsUnlocked runs a few lines above with cs_wallet already held, and vMasterKey is GUARDED_BY(cs_wallet), so nothing can lock the wallet in between. Asking because the commit message says the RPC maps it.

Right, removed this branch

in 5de0c29e06f41f6901fd0d0ec69c9359612371e5 wallet: Return WalletError from SelectHDKey

This is dead code, it can never be executed because if the wallet is locked, EnsureWalletIsUnlocked(*wallet); (L1006) will throw before.

in 11953500e91920bc6357190bcd8956212913743d wallet: Move derivehdkey HD key selection into CWallet

I would make this return Expected since the beginning. Probably commit 1 and 2 should be squashed, you are already changing the return type here, so why not go directly to the "final" version?

Squashed, SelectHDKey returns util::Expected from the start now.

in a1250d2f413016c4b4524d50823ef816af9aff40 wallet: Add deriveHDKey interface

All this is duplicated from RPC code. Why not just make both the interface and the RPC rely on the same code? Because of both using different code functions, they both return different error codes for same errors.

Also probably this commit could be split in different commits. First a preparatory commit that creates the DeriveHDKey function + testing, then a commit rebasing the RPC to use it, then a commit creating the interface. You can check #34861 for a commit structure idea :)

Done. Split into "Add CWallet::DeriveHDKey" and "Add deriveHDKey interface".

On sharing code with the RPC: it already calls SelectHDKey(). What's left is the watch-only, hardened-path and unlock guards, which pin the error codes wallet_derivehdkey.py asserts (-4, -8, -13).

DeriveHDKey() returns GenericError for every failure, so routing the rest through it would turn "Unable to derive HD key at the requested path" from -8 into -5. Keeping -8 would need a new WalletErrorCode just so one caller can pick a different number, which src/wallet/types.h:48-50 warns against.

Now both the interface and the RPC rely on the same CWallet::DeriveHDKey

Same problem the interface test had: origin.path just echoes the argument, and IsValid() holds for any key. With SelectHDKey returning a key the wallet doesn't hold, this one passes while the interface test fails; same for blanking the origin fingerprint or returning the key before derivation.

    const auto active{wallet->GetHDPubKeys(CWallet::HDKeyFilter::Active)};
    BOOST_REQUIRE_EQUAL(active.size(), 1U);
    const auto xprv{wallet->GetExtKey(active.begin()->first)};
    BOOST_REQUIRE(xprv);
    const auto expected{DeriveExtKey(*xprv, path)};
    BOOST_REQUIRE(expected);
    BOOST_CHECK(result->first == expected->first);
    BOOST_CHECK(result->second == expected->second);

Fixed

In wallet: Add CWallet::DeriveHDKey -

There's a lot going on in this commit - extracting to wallet.cpp, changing the RPC error contracts. It would still be beneficial to have 2 commits

• Extract the wallet method whilst preserving existing behavior, including RPC code.
• Adopt WalletError, making the error related changes explicit.

I've split this into separate commits for better readability

I don't think the current structure is what Ram meant. First commit duplicates code now, and the 2ndo removes it which is confusing. Let me suggest another approach:

A way to do this in a clean and easily readable way is to first extract the RPC functionality into some helper function inside the rpc/wallet.cpp, then have another move-only commit that moves that helper function into wallet.cpp. And then another one changing the error codes.

So tldr;

1. Extract in the same rpc file the functionality of DeriveHDKey into a DeriveHDKey helper function
2. Change the error codes.
3. Move the DeriveHDKey function to wallet.cpp.
4. Add the interface

This is easy to review because the extracting code is in the same file and does not change much. Both old code and the new function are in the same commit. The change errors is pretty straight forward. The move commit can easily be reviewed with --color-moved=dimmed-zebra

Left a proposal here: https://github.com/polespinasa/bitcoin/commits/pr-36070/

Now that #35436, can you rebase to that this doesn't look like a new file is added :)

i.e. this commit might be easier to review after rebasing on master.

Rebased and resolved.

In wallet: Add CWallet::DeriveHDKey

I'm not sure if we're okay with changing the error contract since derivehdkey will ship in v32. If not, we likely have to duplicate the checks to retain the error codes. @achow101

Restored the watch-only and unlock checks in the RPC, so -4 and -13 are unchanged from v32.

-8 and -5 are now collapsed to -4, or we can duplicate the checks in the RPC to preserve the v32 behaviour.

Haven't reviewed yet, but re changing error codes: #35690 (comment)

At this point we should add an invalid parameter error code to at least distinguish caller problems from wallet problems. I'm fine with collapsing INVALID_ADDRESS_OR_KEY with INVALID_PARAMETER, but collapsing those into WALLET_ERROR makes the error code to the user too vague. WALLET_ERROR is really for "something bad happened in the wallet internally", as opposed to "the user gave us a bad argument".

Agree, after importdescriptors was merged this should be easy. Those errors already exist now: https://github.com/bitcoin/bitcoin/blob/e7aef7e86da79000aa42f5ba6d13b0d123d9da7c/src/wallet/types.h#L66-L83

I'll remap them based on invalid param or wallet error