Merge bitcoin/bitcoin#31514: wallet: allow label for non-ranged external descriptor (if internal=false) & disallow label for ranged descriptors

664657ed134365588914c2cf6a3975ce368a4f49 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)

Pull request description:

  Motivation:
  * ranged descriptors MUST not be able to have label (current impl allows it)
  * external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)

  Repro steps:
  * create blank wallet and import descriptors
  * external has `label=test` (not internal)
  ```
      conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
                                    passphrase=None, avoid_reuse=False, descriptors=True)
      descriptors = [
          {
              "timestamp": "now",
              "label": "test",
              "active": True,
              "desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
              "internal": False
          },
          {
              "desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
              "active": True,
              "internal": True,
              "timestamp": "now"
          },
      ]
      r = conn.importdescriptors(descriptors)
      print(r)
  ```
  response:
  ```
  [{'error': {'code': -8,
              'message': 'Internal addresses should not have a label'},
    'success': False,
    'warnings': ['Range not given, using default keypool range']},
   {'success': True,
    'warnings': ['Range not given, using default keypool range']}]
  ```
  But in above, ONLY external has a label.

  If you remove `internal: False` from external descriptor import object - it will import no problem:
  ```
  [{'success': True,
    'warnings': ['Range not given, using default keypool range']},
   {'success': True,
    'warnings': ['Range not given, using default keypool range']}]

  ```
  Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.

ACKs for top commit:
  achow101:
    ACK 664657ed134365588914c2cf6a3975ce368a4f49
  rkrux:
    lgtm crACK 664657ed134365588914c2cf6a3975ce368a4f49

Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2

src/wallet/rpc/backup.cpp

@@ -167,6 +167,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
         }
 
         // Range check
+        std::optional<bool> is_ranged;
         int64_t range_start = 0, range_end = 1, next_index = 0;
         if (!parsed_descs.at(0)->IsRange() && data.exists("range")) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
@@ -181,6 +182,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
                 range_end = wallet.m_keypool_size;
             }
             next_index = range_start;
+            is_ranged = true;
 
             if (data.exists("next_index")) {
                 next_index = data["next_index"].getInt<int64_t>();
@@ -202,12 +204,13 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
         }
 
         // Ranged descriptors should not have a label
-        if (data.exists("range") && data.exists("label")) {
+        if (is_ranged.has_value() && is_ranged.value() && data.exists("label")) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Ranged descriptors should not have a label");
         }
 
+        bool desc_internal = internal.has_value() && internal.value();
         // Internal addresses should not have a label either
-        if (internal && data.exists("label")) {
+        if (desc_internal && data.exists("label")) {
             throw JSONRPCError(RPC_INVALID_PARAMETER, "Internal addresses should not have a label");
         }
 
@@ -223,7 +226,6 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
 
         for (size_t j = 0; j < parsed_descs.size(); ++j) {
             auto parsed_desc = std::move(parsed_descs[j]);
-            bool desc_internal = internal.has_value() && internal.value();
             if (parsed_descs.size() == 2) {
                 desc_internal = j == 1;
             } else if (parsed_descs.size() > 2) {

test/functional/wallet_importdescriptors.py

@@ -115,6 +115,9 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                              error_code=-8,
                              error_message="Internal addresses should not have a label")
 
+        self.log.info("External non-ranged addresses can have labels")
+        self.test_importdesc({**import_request, "internal": False}, success=True)
+
         self.log.info("Internal addresses should be detected as such")
         key = get_generate_key()
         self.test_importdesc({"desc": descsum_create("pkh(" + key.pubkey + ")"),
@@ -216,6 +219,16 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                               error_code=-8,
                               error_message='Ranged descriptors should not have a label')
 
+        self.log.info("Ranged descriptors cannot have labels - even if range not provided by user and only implied by asterisk (*)")
+        self.test_importdesc({"desc":descsum_create("wpkh(" + xpub + "/100/0/*)"),
+                              "timestamp": "now",
+                              "label": "test",
+                              "active": True},
+                              success=False,
+                              warnings=['Range not given, using default keypool range'],
+                              error_code=-8,
+                              error_message='Ranged descriptors should not have a label')
+
         self.log.info("Private keys required for private keys enabled wallet")
         self.test_importdesc({"desc":descsum_create(desc),
                               "timestamp": "now",

test/functional/wallet_rescan_unconfirmed.py

@@ -62,7 +62,8 @@ class WalletRescanUnconfirmed(BitcoinTestFramework):
         assert tx_parent_to_reorg["txid"] in node.getrawmempool()
 
         self.log.info("Import descriptor wallet on another node")
-        descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}]
+        # descriptor is ranged - label not allowed
+        descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0}]
 
         node.createwallet(wallet_name="w1", disable_private_keys=True)
         w1 = node.get_wallet_rpc("w1")