Skip to content

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Aug 17, 2022

This PR removes the unused norm_prv parameter in src/test/descriptor_tests.cpp.

@fanquake fanquake added the Tests label Aug 17, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 18, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24149 (Signing support for Miniscript Descriptors by darosior)
  • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@w0xlt
Copy link
Contributor Author

w0xlt commented Aug 19, 2022

It seems the original intention was to use norm_prv in the lines:

std::string norm1;
BOOST_CHECK(parse_priv->ToNormalizedString(keys_priv, norma1));
BOOST_CHECK(EqualDescriptor(norm1, norm_pub));

But even descriptors that were parsed from an xpriv return the xpub in ToNormalizedString.
So this variable norm_prv is not being used anywhere.

Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. The checks against norm_prv were removed in commit 75530c9 (PR #21329) when the posisbility to serialize to normalize private string was removed. But the then-unused norm_priv wasn't removed.

@w0xlt w0xlt force-pushed the remove_norm_prv branch 2 times, most recently from 6992292 to e5d77ce Compare August 20, 2022 00:14
@w0xlt
Copy link
Contributor Author

w0xlt commented Aug 20, 2022

Rebased.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, good catch!

Seems like some comments have been changed unnecessarily, see below.

@achow101
Copy link
Member

ACK 57d1367

@fanquake fanquake merged commit 2d83a20 into bitcoin:master Aug 24, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 24, 2022
…criptor_tests.cpp`.

57d1367 test: remove unused `norm_prv` parameter (w0xlt)

Pull request description:

  This PR removes the unused `norm_prv` parameter in `src/test/descriptor_tests.cpp`.

ACKs for top commit:
  achow101:
    ACK 57d1367

Tree-SHA512: 5b16b6bea94db0b5f2c3675b6529312b50e120d4ec7633e4184dd4ba6fc04e086efb273b9e61f748c8a15cbdc243450b09fc58ec7343379f3151a3b9e7e37106
@bitcoin bitcoin locked and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants