Merge bitcoin/bitcoin#25642: Don't wrap around when deriving an extended key at a too large depth

fb9faffae3a26b8aed8b671864ba679747163019 extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670ce159c2b080e9f735c6603a601d40b6ac descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e7613d6cf6b534df6e551238b80678c70d (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a5ace798c4e54308aa8a09b1ab3302cd7e descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c22bd4c6b3cfaaadd675e95ebe3b3cb1749 spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot)

Pull request description:

  We would previously  silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.

  An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke.

  Fixes #25751.

ACKs for top commit:
  achow101:
    re-ACK fb9faffae3a26b8aed8b671864ba679747163019
  instagibbs:
    utACK  https://github.com/bitcoin/bitcoin/pull/25642/commits/fb9faffae3a26b8aed8b671864ba679747163019

Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343

1 files changed, 11 insertions, 4 deletions

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 9d88b4632e..e2cf081259 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1080,6 +1080,13 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, CHDChain& hd_c
     return pubkey;
 }
 
+//! Try to derive an extended key, throw if it fails.
+static void DeriveExtKey(CExtKey& key_in, unsigned int index, CExtKey& key_out) {
+    if (!key_in.Derive(key_out, index)) {
+        throw std::runtime_error("Could not derive extended key");
+    }
+}
+
 void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& metadata, CKey& secret, CHDChain& hd_chain, bool internal)
 {
     // for now we use a fixed keypath scheme of m/0'/0'/k
@@ -1097,11 +1104,11 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
 
     // derive m/0'
     // use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
-    masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);
+    DeriveExtKey(masterKey, BIP32_HARDENED_KEY_LIMIT, accountKey);
 
     // derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
     assert(internal ? m_storage.CanSupportFeature(FEATURE_HD_SPLIT) : true);
-    accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0));
+    DeriveExtKey(accountKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0), chainChildKey);
 
     // derive child key at next index, skip keys already known to the wallet
     do {
@@ -1109,7 +1116,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
         if (internal) {
-            chainChildKey.Derive(childKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            DeriveExtKey(chainChildKey, hd_chain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
             metadata.hdKeypath = "m/0'/1'/" + ToString(hd_chain.nInternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(1 | BIP32_HARDENED_KEY_LIMIT);
@@ -1117,7 +1124,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
             hd_chain.nInternalChainCounter++;
         }
         else {
-            chainChildKey.Derive(childKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
+            DeriveExtKey(chainChildKey, hd_chain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT, childKey);
             metadata.hdKeypath = "m/0'/0'/" + ToString(hd_chain.nExternalChainCounter) + "'";
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);
             metadata.key_origin.path.push_back(0 | BIP32_HARDENED_KEY_LIMIT);