Skip to content

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Dec 12, 2018

I found the name m_script_arg to be confusing while reviewing #14646 (comment). @sipa let me know if m_subdescriptor_arg is completely wrong.

I also added an explanation of why we call GetPubKey when we don't ask it for a public key.

@Sjors Sjors force-pushed the 2018/12/rename_m_script_arg branch from 5925770 to b2211c6 Compare December 12, 2018 09:10
@fanquake
Copy link
Member

fanquake commented Dec 12, 2018

Might as well fixup the other (trivial) nits.

@Sjors Sjors force-pushed the 2018/12/rename_m_script_arg branch from b2211c6 to acb6f9d Compare December 12, 2018 09:15
@Sjors
Copy link
Member Author

Sjors commented Dec 12, 2018

@fanquake done, I snuck those into the second commit.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 12, 2018

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

Conflicts

No conflicts as of last run.

@Sjors Sjors force-pushed the 2018/12/rename_m_script_arg branch from acb6f9d to 5cb58b8 Compare December 12, 2018 11:22
@fanquake fanquake requested a review from sipa December 13, 2018 14:26
@laanwj
Copy link
Member

laanwj commented Jan 22, 2019

Ping @sipa, as this is your code it'd help if you give your opinion on this.

@meshcollider
Copy link
Contributor

m_script_arg refers to the defined SCRIPT in the descriptor doc, to make it distinct from KEY or ADDR. But subdescriptor is also a clear name so I have no strong preference either way.

-BEGIN VERIFY SCRIPT-
sed -i -e 's/m_script_arg/m_subdescriptor_arg/g' src/script/descriptor.cpp
-END VERIFY SCRIPT-
@Sjors
Copy link
Member Author

Sjors commented Jan 29, 2019

Rebased.

@meshcollider I added a comment referencing SCRIPT in the descriptor docs.

By the way, I noticed that neither DescriptorImpl nor m_subdescriptor_arg show up in the docs, not even in the docs for script/descriptor.cpp. I don't understand Doxygen well enough to know if that's a feature (i.e. because these functions are private).

@Sjors Sjors force-pushed the 2018/12/rename_m_script_arg branch from 5cb58b8 to ee15d27 Compare January 29, 2019 15:32
@Sjors Sjors force-pushed the 2018/12/rename_m_script_arg branch from ee15d27 to 2e68ffa Compare January 29, 2019 15:56
@Sjors
Copy link
Member Author

Sjors commented Feb 21, 2019

Miraculously this doesn't need another rebase...

@practicalswift
Copy link
Contributor

FWIW: I've verified that a disassembly of the bitcoind binary built with this patch applied is identical to a disassembly of the bitcoind binary built against master (as expected).

@laanwj
Copy link
Member

laanwj commented Aug 14, 2019

ACK 2e68ffa

@laanwj laanwj merged commit 2e68ffa into bitcoin:master Aug 14, 2019
laanwj added a commit that referenced this pull request Aug 14, 2019
2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost)
2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost)

Pull request description:

  I found the name `m_script_arg` to be confusing while reviewing #14646 (comment). @sipa let me know if `m_subdescriptor_arg` is completely wrong.

  I also added an explanation of why we call `GetPubKey` when we don't ask it for a public key.

ACKs for top commit:
  laanwj:
    ACK 2e68ffa

Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
@Sjors Sjors deleted the 2018/12/rename_m_script_arg branch August 14, 2019 11:48
fanquake added a commit to fanquake/bitcoin that referenced this pull request Aug 14, 2019
maflcko pushed a commit that referenced this pull request Aug 14, 2019


3963856 descriptor: fix missed m_script_arg arg renaming in #14934 (fanquake)

Pull request description:

  Fixes a missed renaming from #14934.

ACKs for top commit:
  Sjors:
    ACK 3963856

Tree-SHA512: da2972301b2b83556f1f3f3b72758bb69570422cdd57c65aa7657b4e3dcc597dde03e7f1cf8988e8c3b6737737b4c0bee4aefb329376e24e7dedc2cab73fcf88
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2019
2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost)
2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost)

Pull request description:

  I found the name `m_script_arg` to be confusing while reviewing bitcoin#14646 (comment). @sipa let me know if `m_subdescriptor_arg` is completely wrong.

  I also added an explanation of why we call `GetPubKey` when we don't ask it for a public key.

ACKs for top commit:
  laanwj:
    ACK 2e68ffa

Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2019
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2020
Summary:
2e68ffaf205866e4cea71f64e79bbfb89e17280a [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost)
2290269759ad10cc2e35958c7b0a63f3a7608621 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost)

Pull request description:

  I found the name `m_script_arg` to be confusing while reviewing bitcoin/bitcoin#14646 (comment). @sipa let me know if `m_subdescriptor_arg` is completely wrong.

  I also added an explanation of why we call `GetPubKey` when we don't ask it for a public key.

ACKs for top commit:
  laanwj:
    ACK 2e68ffaf205866e4cea71f64e79bbfb89e17280a

---

Backport of Core [[bitcoin/bitcoin#14934 | PR14934]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7806
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Nov 30, 2021
2e68ffa [doc] descriptor: explain GetPubKey() usage with cached public key (Sjors Provoost)
2290269 scripted-diff: rename DescriptorImpl m_script_arg to m_subdescriptor_arg (Sjors Provoost)

Pull request description:

  I found the name `m_script_arg` to be confusing while reviewing bitcoin#14646 (comment). @sipa let me know if `m_subdescriptor_arg` is completely wrong.

  I also added an explanation of why we call `GetPubKey` when we don't ask it for a public key.

ACKs for top commit:
  laanwj:
    ACK 2e68ffa

Tree-SHA512: 06698e9a91cdda93c043a82732793f0ad3cd91daa2513565953e9fa048d5573322fb534e9d0ea9ab736e6366be5921e2b8699c4f4b3693edab48039aaae06f78
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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.

8 participants