Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wallet] remove redundant KeyOriginInfo access, already done in CreateSig #14678

Merged
merged 2 commits into from Nov 23, 2018

Conversation

Projects
None yet
6 participants
@instagibbs
Copy link
Member

commented Nov 7, 2018

This redundancy is confusing as it looks like pubkeyhashes are special in some way based on where it's called.

@instagibbs

This comment has been minimized.

@instagibbs instagibbs changed the title remove redundant KeyOriginInfo access, already done in CreateSig [wallet] remove redundant KeyOriginInfo access, already done in CreateSig Nov 7, 2018

@MarcoFalke MarcoFalke added the Wallet label Nov 7, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13932 (Additional utility RPCs for PSBT 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.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

LGTM 9ae01b9

src/script/sign.cpp Outdated
@@ -64,10 +64,6 @@ static bool GetPubKey(const SigningProvider& provider, SignatureData& sigdata, c
}
// Query the underlying provider
if (provider.GetPubKey(address, pubkey)) {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 9, 2018

Member

Use return provider.GetPubKey(address, pubkey); instead :-)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 12, 2018

Author Member

good one, done.

@instagibbs instagibbs force-pushed the instagibbs:redundant_keypath branch to f7beb95 Nov 12, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Is this redundant for all possible code paths that lead to GetPubKey?

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

@laanwj It's the only instance of it, yes.

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

so this is SignStep that calls GetPubKey then CreateSig?
I see, utACK f7beb95

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

made sigdata const to make the intention of function clearer

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

re-utACK b81a186

@laanwj laanwj merged commit b81a186 into bitcoin:master Nov 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 23, 2018

Merge #14678: [wallet] remove redundant KeyOriginInfo access, already…
… done in CreateSig

b81a186 GetPubKey: make sigdata const (Gregory Sanders)
f7beb95 remove redundant KeyOriginInfo access, already done in CreateSig (Gregory Sanders)

Pull request description:

  This redundancy is confusing as it looks like pubkeyhashes are special in some way based on where it's called.

Tree-SHA512: a980b7c774c6d69322945227a2b156489fb1991ebf57fe6f26096d5f8047f246a133debc241b05af67810f604b040079add3ab3d30d9e2928095905a2afe17eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.