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

scgbckbone · 2024-12-17T10:35:53Z

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 label 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.

DrahtBot · 2024-12-17T10:35:57Z

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.

Type	Reviewers
ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

scgbckbone · 2024-12-17T10:41:02Z

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

[{'error': {'code': -8,
            'message': 'Ranged descriptors 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']}]

furszy · 2024-12-17T15:12:34Z

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.

scgbckbone · 2024-12-17T15:31:30Z

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

scgbckbone

@achow101 - looks like logic bugs to me

luke-jr · 2025-01-14T04:12:32Z

It would be nice to have a test that checks all 4 success/failure cases.

scgbckbone · 2025-03-26T15:01:31Z

added tests & rebased on master

achow101 · 2025-04-16T22:49:35Z

ACK dda90a9

Nice find!

external non-ranged descriptor MUST be able to have label (current impl disallows it)

Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?

…nged 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

scgbckbone · 2025-04-22T19:02:10Z

ACK dda90a9

Nice find!

external non-ranged descriptor MUST be able to have label (current impl disallows it)

Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?

the test in question:

        self.log.info("Should import a p2pkh descriptor")
        import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),
                 "timestamp": "now",
                 "label": "Descriptor import test"}
        self.test_importdesc(import_request, success=True)

passes as the bug does not manifest if "internal" is not provided by user

scgbckbone · 2025-04-22T19:03:09Z

re-based on current master

achow101 · 2025-04-23T00:29:20Z

ACK 664657e

passes as the bug does not manifest if "internal" is not provided by user

I see. Can you mention that in the PR description?

re-based on current master

There's no need to rebase if there are no conflicts.

scgbckbone · 2025-04-23T08:19:46Z

PR description (and title) updated

DrahtBot changed the title ~~allow lable for external descriptor & disallow label for ranged descriptors~~ allow lable for external descriptor & disallow label for ranged descriptors Dec 17, 2024

DrahtBot added the CI failed label Dec 17, 2024

scgbckbone force-pushed the ProcessDescriptorImport_fixes branch from 26b5394 to 3251036 Compare December 17, 2024 14:46

scgbckbone force-pushed the ProcessDescriptorImport_fixes branch from 3251036 to 9131075 Compare December 18, 2024 15:58

maflcko removed the CI failed label Dec 19, 2024

scgbckbone changed the title ~~allow lable for external descriptor & disallow label for ranged descriptors~~ wallet: allow lable for external descriptor & disallow label for ranged descriptors Dec 23, 2024

DrahtBot added the Wallet label Dec 23, 2024

scgbckbone commented Dec 23, 2024

View reviewed changes

fanquake requested a review from achow101 February 21, 2025 16:26

scgbckbone force-pushed the ProcessDescriptorImport_fixes branch from 9131075 to dda90a9 Compare March 26, 2025 13:20

scgbckbone changed the title ~~wallet: allow lable for external descriptor & disallow label for ranged descriptors~~ wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors Mar 26, 2025

bugfix: disallow label for ranged descriptors & allow external non-ra…

664657e

…nged 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

scgbckbone force-pushed the ProcessDescriptorImport_fixes branch from dda90a9 to 664657e Compare April 22, 2025 18:57

scgbckbone changed the title ~~wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors~~ wallet: allow label for non-ranged external descriptor (if internal=false) & disallow label for ranged descriptors Apr 23, 2025

achow101 mentioned this pull request Apr 29, 2025

Cannot import descriptors with label and internal:false #32376

Open

1 task

DrahtBot added CI failed and removed CI failed labels May 16, 2025

DrahtBot mentioned this pull request Jul 14, 2025

Silent Payments: Receiving #32966

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

scgbckbone commented Dec 17, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Dec 17, 2024 •

edited

Loading

Uh oh!

scgbckbone commented Dec 17, 2024

Uh oh!

furszy commented Dec 17, 2024

Uh oh!

scgbckbone commented Dec 17, 2024

Uh oh!

scgbckbone left a comment

Uh oh!

luke-jr commented Jan 14, 2025

Uh oh!

scgbckbone commented Mar 26, 2025

Uh oh!

achow101 commented Apr 16, 2025 •

edited

Loading

Uh oh!

scgbckbone commented Apr 22, 2025

Uh oh!

scgbckbone commented Apr 22, 2025

Uh oh!

achow101 commented Apr 23, 2025

Uh oh!

scgbckbone commented Apr 23, 2025

Uh oh!

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

scgbckbone commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

scgbckbone commented Dec 17, 2024

Uh oh!

furszy commented Dec 17, 2024

Uh oh!

scgbckbone commented Dec 17, 2024

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jan 14, 2025

Uh oh!

scgbckbone commented Mar 26, 2025

Uh oh!

achow101 commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scgbckbone commented Apr 22, 2025

Uh oh!

scgbckbone commented Apr 22, 2025

Uh oh!

achow101 commented Apr 23, 2025

Uh oh!

scgbckbone commented Apr 23, 2025

Uh oh!

Uh oh!

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

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

scgbckbone commented Dec 17, 2024 •

edited

Loading

DrahtBot commented Dec 17, 2024 •

edited

Loading

achow101 commented Apr 16, 2025 •

edited

Loading