Replace CScriptID and CKeyID in CTxDestination with dedicated types #15452

instagibbs · 2019-02-20T22:01:15Z

The current usage seems to be an overloading of meanings. CScriptID is used in the wallet as a lookup key, as well as a destination, and CKeyID likewise. Instead, have all destinations be dedicated types.

New types:
CScriptID->ScriptHash
CKeyID->PKHash

DrahtBot · 2019-02-20T23:34:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15093 (rpc: Change importwallet to return additional errors by marcinja)
#12578 (gui: Add transaction record type Fee by promag)

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.

src/script/standard.h

sipa · 2019-02-22T02:17:56Z

Concept ACK. I believe @ryanofsky suggested something like this before.

src/script/standard.h

instagibbs · 2019-02-26T14:14:23Z

Not sure how to read appveyor error tbh

maflcko · 2019-02-26T14:34:18Z

The appveyor sync failure was fixed earlier this week

Sjors · 2019-02-27T08:16:37Z

Concept ACK. Maybe enumerate the new types in the PR description?

~~Shouldn't CKeyID GetID() in pubkey.h also return a PKHash ? That breaks a few other things from the looks of it, but otherwise it seems inconsistent.~~ (no actually that's the point :-)

Sjors · 2019-02-27T08:28:06Z

src/script/standard.h

+{
+    PKHash() : uint160() {}
+    explicit PKHash(const uint160& hash) : uint160(hash) {}
+    explicit PKHash(const CPubKey& pubkey);


Inline definition might be more clear but I don't want to upset any strong c++ preferences out there.

It's a mixed bag here(definition for other constructors are in .cpp), I'll leave as-is unless someone else chimes in

stevenroose · 2019-03-14T15:08:40Z

utACK 3330398

instagibbs · 2019-03-28T16:13:46Z

rebased

instagibbs · 2019-04-23T13:54:21Z

rebased and added description of new destination types in OP @Sjors

ryanofsky

Conditional utACK 5148213 if fixes are added for travis failures. I'm guessing this will just require rebasing and adding some more CKeyID/CScriptID replacements.

This change was also suggested a while ago in: #11403 (comment) and I think it could be simplified a little by taking the other suggestion in that comment and making ScriptHash inherit from CScriptID and PKHash inherit from CKeyID. This would avoid the need to add explicit conversions several places in this PR. I think it's important for conversions from CScriptID to ScriptHash and CKeyID to PKHash to be explicit, but it seems fine for conversions in the opposite direction to be implicit.

ryanofsky · 2019-04-23T15:48:12Z

src/wallet/wallet.cpp

    for (const auto& entry : mapKeyMetadata) {
        if (entry.second.nCreateTime) {
-            mapKeyBirth[entry.first] = entry.second.nCreateTime;
+            mapKeyBirth[PKHash(entry.first)] = entry.second.nCreateTime;


This change and the 2 changes below will work but don't really make sense logically. Would suggest reverting these and just changing the map key type to:

boost::variant<CKeyID, CScriptID>

or maybe

CKeyID

if you wanted to drop the "TODO: include scripts in GetKeyBirthTimes() output instead of separate" comment.

instagibbs · 2019-04-23T16:51:46Z

fixed a single dangling test case that was expecting a PKHash, and added a commit to change GetKeyBirthTimes to returning CKeyID instead of CTxDestination.

ryanofsky

utACK ddc21f2. Changes since last review are fixing test case and switching GetKeyBirthTimes away from CTxDestination.

Sjors · 2019-04-24T10:39:10Z

tACK ddc21f2. Compiled, opened a wallet and ran test suite and bench on macOS 10.14.4. Not sure what else to test here.

src/rpc/rawtransaction.cpp

src/test/script_standard_tests.cpp

instagibbs · 2019-04-29T14:16:04Z

all comments addressed

ryanofsky

utACK 78e407a. Only changes are removing extra CScriptID()s and fixing the test case.

Sjors

utACK 78e407a

meshcollider

utACK 78e407a

By removal of the // TODO: include scripts in GetKeyBirthTimes() output instead of separate comment, I assume you think that shouldn't be done, rather than because you implemented it.

…icated types 78e407a GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders) 70946e7 Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders) Pull request description: The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types. New types: `CScriptID`->`ScriptHash` `CKeyID`->`PKHash` ACKs for commit 78e407: ryanofsky: utACK 78e407a. Only changes are removing extra CScriptID()s and fixing the test case. Sjors: utACK 78e407a meshcollider: utACK 78e407a Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9

instagibbs · 2019-05-09T17:00:47Z

@meshcollider payoff was very low, and descriptor wallets would have changed whatever someone was going to do

…ith dedicated types 78e407a GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders) 70946e7 Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders) Pull request description: The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types. New types: `CScriptID`->`ScriptHash` `CKeyID`->`PKHash` ACKs for commit 78e407: ryanofsky: utACK 78e407a. Only changes are removing extra CScriptID()s and fixing the test case. Sjors: utACK 78e407a meshcollider: utACK bitcoin@78e407a Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9

Update chainActive to ::ChainActive() in Namecoin code. Changed the Namecoin code for the upstream refactoring of CKeyID and CScriptID in the wallet (bitcoin/bitcoin#15452). Explicitly serialise block without witness, so that we do not have to activate segwit at height 432 first in name_multiupdate.py.

Update chainActive to ::ChainActive() in Xaya code. Changed the Xaya code (esp. the modified verifymessage) for the upstream refactoring of CKeyID and CScriptID in the wallet (bitcoin/bitcoin#15452). Explicitly serialise block without witness, so that we do not have to activate segwit at height 432 first in xaya_gameblocks.py.

…dedicated types Summary: bitcoin/bitcoin@70946e7 --- Depends on D6402 Partial backport of Core [[bitcoin/bitcoin#15452 | PR15452]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6403

…ions Summary: bitcoin/bitcoin@78e407a --- Depends on D6403 Concludes backport of Core [[bitcoin/bitcoin#15452 | PR15452]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6404

…ith dedicated types 78e407a GetKeyBirthTimes should return key ids, not destinations (Gregory Sanders) 70946e7 Replace CScriptID and CKeyID in CTxDestination with dedicated types (Gregory Sanders) Pull request description: The current usage seems to be an overloading of meanings. `CScriptID` is used in the wallet as a lookup key, as well as a destination, and `CKeyID` likewise. Instead, have all destinations be dedicated types. New types: `CScriptID`->`ScriptHash` `CKeyID`->`PKHash` ACKs for commit 78e407: ryanofsky: utACK 78e407a. Only changes are removing extra CScriptID()s and fixing the test case. Sjors: utACK 78e407a meshcollider: utACK bitcoin@78e407a Tree-SHA512: 437f59fc3afb83a40540da3351507aef5aed44e3a7f15b01ddad6226854edeee762ff0b0ef336fe3654c4cd99a205cef175211de8b639abe1130c8a6313337b9

maflcko added Refactoring Wallet labels Feb 20, 2019

laanwj reviewed Feb 21, 2019

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

instagibbs force-pushed the key_script_dest branch from 07fb599 to 3330398 Compare February 22, 2019 02:06

practicalswift reviewed Feb 25, 2019

View reviewed changes

src/script/standard.h Show resolved Hide resolved

Sjors reviewed Feb 27, 2019

View reviewed changes

DrahtBot added the Needs rebase label Mar 28, 2019

instagibbs force-pushed the key_script_dest branch from 3330398 to 61ccb14 Compare March 28, 2019 16:13

DrahtBot removed the Needs rebase label Mar 28, 2019

DrahtBot added the Needs rebase label Apr 16, 2019

instagibbs force-pushed the key_script_dest branch from 61ccb14 to 5148213 Compare April 23, 2019 13:53

DrahtBot removed the Needs rebase label Apr 23, 2019

ryanofsky approved these changes Apr 23, 2019

View reviewed changes

instagibbs force-pushed the key_script_dest branch from 5148213 to ddc21f2 Compare April 23, 2019 16:50

ryanofsky approved these changes Apr 23, 2019

View reviewed changes

meshcollider reviewed Apr 27, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/test/script_standard_tests.cpp Outdated Show resolved Hide resolved

src/test/script_standard_tests.cpp Outdated Show resolved Hide resolved

instagibbs force-pushed the key_script_dest branch from ddc21f2 to 3488d45 Compare April 29, 2019 13:13

instagibbs added 2 commits April 29, 2019 10:15

Replace CScriptID and CKeyID in CTxDestination with dedicated types

70946e7

GetKeyBirthTimes should return key ids, not destinations

78e407a

instagibbs force-pushed the key_script_dest branch from 3488d45 to 78e407a Compare April 29, 2019 14:15

ryanofsky approved these changes Apr 29, 2019

View reviewed changes

fanquake requested review from Sjors and meshcollider May 8, 2019 13:51

Sjors approved these changes May 8, 2019

View reviewed changes

meshcollider approved these changes May 9, 2019

View reviewed changes

laanwj merged commit 78e407a into bitcoin:master May 9, 2019

meshcollider mentioned this pull request Sep 15, 2019

Replace GetScriptForWitness with GetScriptForDestination #16841

Merged

furszy mentioned this pull request Sep 7, 2021

[Refactor] Differentiate destinations from lookup keys and disallow automatic conversion between them PIVX-Project/PIVX#2543

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Replace CScriptID and CKeyID in CTxDestination with dedicated types #15452

Replace CScriptID and CKeyID in CTxDestination with dedicated types #15452

Uh oh!

Conversation

instagibbs commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

sipa commented Feb 22, 2019

Uh oh!

Uh oh!

instagibbs commented Feb 26, 2019

Uh oh!

maflcko commented Feb 26, 2019

Uh oh!

Sjors commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Mar 14, 2019

Uh oh!

instagibbs commented Mar 28, 2019

Uh oh!

instagibbs commented Apr 23, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Apr 23, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Apr 23, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Apr 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Apr 29, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented May 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

instagibbs commented Feb 20, 2019 •

edited

Loading

DrahtBot commented Feb 20, 2019 •

edited

Loading

Sjors commented Feb 27, 2019 •

edited

Loading