Refactor: split CKeyID/CScriptID/CTxDestination from CBitcoinAddress #1357
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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) |
|
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. |
|
ACK |
|
@gavinandresen what say you? |
|
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. |
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.
|
Rebased and fixed unit tests. @gavinandresen: added some comments, as well |
sipa
added a commit
that referenced
this pull request
May 26, 2012
Refactor: split CKeyID/CScriptID/CTxDestination from CBitcoinAddress
coblee
pushed a commit
to litecoin-project/litecoin
that referenced
this pull request
Jul 17, 2012
Refactor: split CKeyID/CScriptID/CTxDestination from CBitcoinAddress
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
Jan 22, 2019
Prevent the re-requesting of txns we already have
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
May 6, 2020
96b4732 [Qt] Replace deprecated Qt methods (Fuzzbawls) Pull request description: `qSort`, `qStableSort`, `qLowerBound`, and `qUpperBound` have been marked as deprecated since Qt 5.2. Also, `QSslSocket::setDefaultCaCertificates()` and `QSslSocket::systemCaCertificates()` member functions are obsolete as of Qt 5.12 This PR replaces the former with their `std::` equivalents, and the latter with `QSslConfiguration` equivalents. Changes maintain backwards compatibility with Qt 5.5.1 ACKs for top commit: random-zebra: utACK 96b4732 furszy: utACK 96b4732 Tree-SHA512: 0631d1930ccc45dc574c354f0de2406000af82e5203a6153a0e50e5e96f027eb829a08c30d109cd7d3df267ebd84c7c1fa838cd6f3798ca037bc6f3ecc49785d
maflcko
pushed a commit
that referenced
this pull request
Jun 19, 2020
3351c91 refactor: Make CScriptVisitor stateless (João Barbosa) Pull request description: `CScriptVisitor` was added in 1025440 (#1357) and the visitor return type was never used. Now `CScriptVisitor` is stateless and `CScript` is the return type. ACKs for top commit: MarcoFalke: ACK 3351c91 🏤 sipa: utACK 3351c91 Tree-SHA512: d158ad2ebe8ea4dc8cc090b943dd66fa5421a84f9443e16ab2d661df38e1a85de16ff13cbaa56924489d8d43cba25fa3cd8b6904bbbcbf356b886ffe8ffba19a
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 7, 2020
3351c91 refactor: Make CScriptVisitor stateless (João Barbosa) Pull request description: `CScriptVisitor` was added in 1025440 (bitcoin#1357) and the visitor return type was never used. Now `CScriptVisitor` is stateless and `CScript` is the return type. ACKs for top commit: MarcoFalke: ACK 3351c91 🏤 sipa: utACK 3351c91 Tree-SHA512: d158ad2ebe8ea4dc8cc090b943dd66fa5421a84f9443e16ab2d661df38e1a85de16ff13cbaa56924489d8d43cba25fa3cd8b6904bbbcbf356b886ffe8ffba19a
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Aug 26, 2021
Summary: > CScriptVisitor was added in [[bitcoin/bitcoin#1357 | core#1357]] and the visitor return type was never used. Now CScriptVisitor is stateless and CScript is the return type. This is a backport of [[bitcoin/bitcoin#18863 | core#18863]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9939
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This introduces internal types:
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.