Refactor: split CKeyID/CScriptID/CTxDestination from CBitcoinAddress #1357

pull sipa wants to merge 2 commits into bitcoin:master from sipa:keyid changing 28 files +579 −400

sipa commented at 10:55 PM on May 18, 2012: member
This introduces internal types:
- CPubKey: encapsulated a public key
- CKeyID: reference (hash160) of a key
- CScriptID: reference (hash160) of a script
- CTxDestination: a boost::variant of the former two
CBitcoinAddress is retrofitted to be a Base58 encoding of a CTxDestination. This allows all internal code to only use the internal types, and only have RPC and GUI depend on the base58 code.

Furthermore, the header dependencies are a lot saner now. base58.h is at the top (right below rpc and gui) instead of at the bottom. For the rest: wallet -> script -> keystore -> key. Only keystore still requires a forward declaration of CScript. Solving that would require splitting script into two layers.
gavinandresen commented at 11:19 PM on May 19, 2012: contributor

What do other people think about using boost::variant?

I'm not a fan of the "write a little class to do simple things" style of programming that it brings, I think it just obfuscates the flow of control and makes debugging harder.

(oh, and creates god-awful compiler errors if you screw up)
sipa commented at 11:40 PM on May 19, 2012: member

The main motivation for this patch is to make a clear separation between key identifiers and addresses, split internal representation from their base58 encodings, and sanitize header dependencies.

If people have a problem with boost::variant, I have absolutely no problem to write an ad-doc CTxDestination implementation that doesn't use it.

RE: "write a little class to do simple things"; if you're referring to this case in particular: it just replaces CBitcoinAddress which was for all intents and purposes already a variant of key or script references, only done via its base58 representation.
jgarzik commented at 6:54 PM on May 21, 2012: contributor

ACK
sipa commented at 9:01 PM on May 22, 2012: member

@gavinandresen what say you?
gavinandresen commented at 5:07 PM on May 23, 2012: contributor

I like the refactor.

It would be really nice to have English-language rationale for the design of CKeyID/CScriptID/CPubKey/CTxDestination in the .h file(s) and not just in the commit comment.
Encapsulate public keys in CPubKey fd61d6f506

Refactor: split CKeyID/CScriptID/CTxDestination from CBitcoinAddress

This introduces internal types:
* CKeyID: reference (hash160) of a key
* CScriptID: reference (hash160) of a script
* CTxDestination: a boost::variant of the former two

CBitcoinAddress is retrofitted to be a Base58 encoding of a
CTxDestination. This allows all internal code to only use the
internal types, and only have RPC and GUI depend on the base58 code.

Furthermore, the header dependencies are a lot saner now. base58.h is
at the top (right below rpc and gui) instead of at the bottom. For the
rest: wallet -> script -> keystore -> key. Only keystore still requires
a forward declaration of CScript. Solving that would require splitting
script into two layers.

1025440184

sipa commented at 6:28 PM on May 24, 2012: member

Rebased and fixed unit tests. @gavinandresen: added some comments, as well
sipa referenced this in commit a52c7a1b65 on May 26, 2012
sipa merged this on May 26, 2012
sipa closed this on May 26, 2012
coblee referenced this in commit b015e1e079 on Jul 17, 2012
sipa deleted the branch on Jun 23, 2017
lateminer referenced this in commit 33ff4b074d on Jan 22, 2019
lateminer referenced this in commit 85497fed21 on May 6, 2020
MarcoFalke referenced this in commit 5f72ddb7ee on Jun 19, 2020
sidhujag referenced this in commit bff30e54f0 on Jul 7, 2020
Fabcien referenced this in commit a112f4d8d3 on Aug 26, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

sipa

gavinandresen

jgarzik

Linked (view graph)

#18863 refactor: Make CScriptVisitor stateless