allow lable for external descriptor & disallow label for ranged descriptors #31514

1. 
   scgbckbone commented at 10:35 am on December 17, 2024: none

   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)

   Repro steps:

   • create blank wallet and import descriptors
   • external has label=test (not internal)

    0    conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
    1                                  passphrase=None, avoid_reuse=False, descriptors=True)
    2    descriptors = [
    3        {
    4            "timestamp": "now",
    5            "label": "test",
    6            "active": True,
    7            "desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
    8            "internal": False
    9        },
   10        {
   11            "desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
   12            "active": True,
   13            "internal": True,
   14            "timestamp": "now"
   15        },
   16    ]
   17    r = conn.importdescriptors(descriptors)
   18    print(r)
   

   response:

   0[{'error': {'code': -8,
   1            'message': 'Internal addresses should not have a label'},
   2  'success': False,
   3  'warnings': ['Range not given, using default keypool range']},
   4 {'success': True,
   5  'warnings': ['Range not given, using default keypool range']}]
   

   But in above, ONLY external has a label.

   If you remove label from external descriptor import object - it will import no problem:

   0[{'success': True,
   1  'warnings': ['Range not given, using default keypool range']},
   2 {'success': True,
   3  '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.

2. 
   DrahtBot commented at 10:35 am on December 17, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31514.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. DrahtBot renamed this:
   allow lable for external descriptor & disallow label for ranged descriptors
   allow lable for external descriptor & disallow label for ranged descriptors
   on Dec 17, 2024
4. 
   scgbckbone commented at 10:41 am on December 17, 2024: none

   after fix if you have label and ranged descriptor (even without providing range via api):

   0[{'error': {'code': -8,
   1            'message': 'Ranged descriptors should not have a label'},
   2  'success': False,
   3  'warnings': ['Range not given, using default keypool range']},
   4 {'success': True,
   5  'warnings': ['Range not given, using default keypool range']}]
   

5. DrahtBot added the label CI failed on Dec 17, 2024
6. scgbckbone force-pushed on Dec 17, 2024
7. 
   furszy commented at 3:12 pm on December 17, 2024: member

   Could you please add the motivation for the behavior change at the beginning of the PR description? For example, could describe how the process currently behaves, why you think that it is not adequate and what your change proposal does to improve the functionality.
8. 
   scgbckbone commented at 3:31 pm on December 17, 2024: none

   Could you please add the motivation for the behavior change at the beginning of the PR description? For example, could describe how the process currently behaves, why you think that it is not adequate and what your change proposal does to improve the functionality.

   I thought it’s clear - but added

9. 
   maflcko commented at 6:45 pm on December 17, 2024: member

   Lint CI: [14:49:00.393] The subject line of commit hash 3251036f5836b04f48f0bb5cce6cde770a1d44d2 is followed by a non-empty line. Subject lines should always be followed by a blank line.
10. bugfix: disallow label for ranged descriptors & allow external descriptors to have label

    * do not only check user provided range data to decide whether descriptor is ranged
    * properly handle std::optional<bool> when checking if descriptor is internal

    91310753da
11. scgbckbone force-pushed on Dec 18, 2024
12. maflcko removed the label CI failed on Dec 19, 2024

Contributors
scgbckbone DrahtBot furszy maflcko