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
Break up script/standard.{h/cpp} #28244
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
* | ||
* @param[in] scriptPubKey Script to parse | ||
* @param[out] vSolutionsRet Vector of parsed pubkeys and hashes | ||
* @return The script type. TxoutType::NONSTANDARD represents a failed solve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be noted what represents a “failed solve” is up to the caller. Actually TxoutType::WITNESS_UNKNOWN
is considered as a failure in AreInputsStandard
, as witness unknown is a special case of non-standardness for undefined segwit output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving as is since it is only moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3a6e49e
TBH, I’m not super familiar with the files that are being touched, but I verified that it’s almost exclusively code moves, and looked over the minuscule other changes (like adding and removing includes, extracting something into a function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
However 34aca2d seems to move in the exact opposite direction of #26291. Thoughts @ajtowns & @instagibbs? We can easy move that constant again though.
Otherwise code review ACK 3a6e49e.
I dream of a future where validation.cpp
is split between consensus and policy (and the former doesn't depend on policy.h
).
What do kernel folks think of this separation? cc @TheCharlatan
Nit: maybe rename addresstype.{h,cpp}
to address_type
.
Did you use some script for b3af9ce?
src/script/standard.cpp
Outdated
@@ -17,7 +17,6 @@ | |||
typedef std::vector<unsigned char> valtype; | |||
|
|||
CScriptID::CScriptID(const CScript& in) : BaseHash(Hash160(in)) {} | |||
CScriptID::CScriptID(const ScriptHash& in) : BaseHash(static_cast<uint160>(in)) {} | |||
|
|||
ScriptHash::ScriptHash(const CScript& in) : BaseHash(Hash160(in)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
582708b: Note to other reviewers: we don't want to move ScriptHash
into script.h
because it has no business in consensusland. Since the constructor here needs ScriptHash
, dropping it avoids the circular dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK
tACK - Using Full Settings
Result: |
Replaces the constructor in CScriptID that converts a ScriptHash with a function ToScriptID that does the same. This prepares for a move of CScriptID to avoid a circular dependency.
CScriptID should be next to CScript just as CKeyID is next to CPubKey
TaprootSpendData and TaprootBuilder are used in signing in SigningProvider contexts, so they should live near that.
CTxDestination is really our internal representation of an address and doesn't really have anything to do with standard script types, so move them to their own file.
Remove standard.h from files that don't use anything in it, and include it in files that do.
Since script/standard only contains things that are used by the Solver and its callers, rename the files to script/solver.
Ran IWYU with manual cleanup. |
Concept ACK |
ACK 91d924e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 91d924e 😇
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇
hmHg3Qo/GQGeCKFKxGmHrg0EmWcpbYzmscRpSuTyu3LmtxqJWEsxftLwps2A5BuOvoJEocpFYxa/YP/1kWeZBw==
src/script/standard.cpp
Outdated
@@ -38,6 +37,11 @@ CKeyID ToKeyID(const WitnessV0KeyHash& key_hash) | |||
return CKeyID{static_cast<uint160>(key_hash)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated: IIUC this creates a useless copy. Might be better to just retain the reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #28284
|
||
#include <vector> | ||
|
||
typedef std::vector<unsigned char> valtype; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7a172c7: nit: Any reason to put this in this scope, instead of in the only scope that uses it (one function)?
Also, could use using
(C++11)?
#include <uint256.h> | ||
#include <util/hash_type.h> | ||
|
||
#include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7a172c7: nit:
addresstype.cpp should add these lines:
#include <cassert>
#include "crypto/sha256.h" // for CSHA256
#include "span.h" // for Span
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #28284
src/script/standard.cpp
Outdated
|
||
#include <string> | ||
#include <algorithm> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bacdb2e: nit:
script/solver.cpp should add these lines:
#include <cassert> // for assert
#include "prevector.h" // for operator-, prevector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #28284
// Distributed under the MIT software license, see the accompanying | ||
// file COPYING or https://www.opensource.org/licenses/mit-license.php. | ||
|
||
#include <addresstype.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7a172c7:nit: missing newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #28284
@@ -657,6 +658,7 @@ libbitcoin_consensus_a_SOURCES = \ | |||
libbitcoin_common_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS) | |||
libbitcoin_common_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | |||
libbitcoin_common_a_SOURCES = \ | |||
addresstype.cpp \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit in 7a172c7: Could keep this in the script
directory, next to script/descriptor.cpp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes or maybe have a new directory for everything that's "working with scripts" (addresstype
, descriptor
, signingprovider
, ..).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 91d924e
ACK 91d924e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 91d924e.
However i think you can just drop the largest commit here (7a172c7) and save a (+291, -247). It would also make more sense IMO. You state:
TaprootSpendData
andTaprootBuilder
(and their utility functions and structs) are moved to SigningProvider as these are used only during signing.standard.{cpp/h}
is renamed tosolver.{cpp/h}
since that's all that's left in the file after the above moves
But Taproot{Builder, SpendData}
are more related to solving Taproots generally rather than to the "keystores" in signingprovider.h
specifically. Since the file you move it from eventually becomes solver.h
, i don't think it makes a lot of sense to move it.
@@ -657,6 +658,7 @@ libbitcoin_consensus_a_SOURCES = \ | |||
libbitcoin_common_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS) | |||
libbitcoin_common_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | |||
libbitcoin_common_a_SOURCES = \ | |||
addresstype.cpp \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes or maybe have a new directory for everything that's "working with scripts" (addresstype
, descriptor
, signingprovider
, ..).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK 91d924e
Posthumous concept ACK |
Post-merge code review ACK 91d924e. |
…t scriptPubKey ad0c469 wallet: Use CTxDestination in CRecipient rather than scriptPubKey (Andrew Chow) 07d3bdf Add PubKeyDestination for P2PK scripts (Andrew Chow) 1a98a51 Allow CNoDestination to represent a raw script (Andrew Chow) 8dd0670 Make WitnessUnknown members private (Andrew Chow) Pull request description: For silent payments, we want to provide a `SilentPaymentsDestination` to be used as the recipient, which requires `CRecipient` to use something other than just the `scriptPubKey` as we cannot know the output script for a silent payment prior to transaction creation. `CTxDestination` seems like the obvious place to add a `SilentPaymentsDestination` as it is our internal representation of an address. In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to `fundrawtransaction`), `CNoDestination` is changed to contain raw scripts. Additionally, P2PK scripts are now interpreted as a new `PubKeyDestination` rather than `PKHash`. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct. `ExtractDestination`'s behavior is slightly changed for the above. It now returns `true` for those destinations that have addresses, so P2PK scripts now result in `false`. Even though it returns false for `CNoDestination`, the script will now be included in that `CNoDestination`. Builds on #28244 ACKs for top commit: josibake: ACK ad0c469 Tree-SHA512: ef3f8f3c7284779d9806c77c85b21caf910a79a1f7e7f1b51abcc0d7e074f14e00abf30f625a13075e41d94dad6202c10ddff462c0ee74c2ca4aab585b145a52
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
CTxDestination
is moved to a new filesrc/addresstype.{cpp/h}
TaprootSpendData
andTaprootBuilder
(and their utility functions and structs) are moved toSigningProvider
as these are used only during signing.CScriptID
is moved toscript/script.h
to be next toCScript
.MANDATORY_SCRIPT_VERIFY_FLAGS
is moved tointerpreter.h
DEFAULT_ACCEPT_DATACARRIER
andMAX_OP_RETURN_RELAY
are moved topolicy.h
standard.{cpp/h}
is renamed tosolver.{cpp/h}
since that's all that's left in the file after the above moves