Break up script/standard.{h/cpp}

achow101 commented at 11:43 am on August 9, 2023: member

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 file src/addresstype.{cpp/h}
TaprootSpendData and TaprootBuilder (and their utility functions and structs) are moved to SigningProvider as these are used only during signing.
CScriptID is moved to script/script.h to be next to CScript.
MANDATORY_SCRIPT_VERIFY_FLAGS is moved to interpreter.h
The parameters DEFAULT_ACCEPT_DATACARRIER and MAX_OP_RETURN_RELAY are moved to policy.h
standard.{cpp/h} is renamed to solver.{cpp/h} since that’s all that’s left in the file after the above moves

DrahtBot commented at 11:44 am on August 9, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, MarcoFalke, murchandamus, ajtowns, darosior, theStack
Concept ACK	ariard, russeree

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
#28230 (rpc: Add Arg() default helper by MarcoFalke)
#28200 (refactor: Remove unused includes from wallet.cpp by MarcoFalke)
#28193 (test: add script compression coverage for not-on-curve P2PK outputs by theStack)
#28122 (Silent Payments: Implement BIP352 by josibake)
#27926 (policy: make unstructured annex standard by joostjager)
#27850 (test: Add unit & functional test coverage for blockstore by pinheadmz)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)
#26573 (Wallet: don’t underestimate the fees when spending a Taproot output by darosior)
#26567 (Wallet: estimate the size of signed inputs using descriptors by darosior)
#26403 (policy: Ephemeral anchors by instagibbs)
#26291 (Update MANDATORY_SCRIPT_VERIFY_FLAGS by ajtowns)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)
#21283 (Implement BIP 370 PSBTv2 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.

achow101 force-pushed on Aug 9, 2023

DrahtBot added the label CI failed on Aug 9, 2023

in src/script/solver.h:6 in cc898bfb5e outdated

0@@ -0,0 +1,64 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2022 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#ifndef BITCOIN_SCRIPT_STANDARD_H

fanquake commented at 12:48 pm on August 9, 2023:

0src/script/solver.h seems to be missing the expected include guard:
1  #ifndef BITCOIN_SCRIPT_SOLVER_H
2  #define BITCOIN_SCRIPT_SOLVER_H
3  ...
4  #endif // BITCOIN_SCRIPT_SOLVER_H

achow101 commented at 1:08 pm on August 9, 2023:

Done

achow101 force-pushed on Aug 9, 2023

DrahtBot removed the label CI failed on Aug 9, 2023

in src/script/solver.h:52 in 3a6e49e80f outdated

45+ * the type. For example, for a P2SH script, vSolutionsRet will contain the
46+ * script hash, for P2PKH it will contain the key hash, etc.
47+ *
48+ * @param[in]   scriptPubKey   Script to parse
49+ * @param[out]  vSolutionsRet  Vector of parsed pubkeys and hashes
50+ * @return                     The script type. TxoutType::NONSTANDARD represents a failed solve.

ariard commented at 1:52 am on August 10, 2023:

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.

achow101 commented at 9:26 pm on August 14, 2023:

leaving as is since it is only moved.

ariard commented at 1:52 am on August 10, 2023: member

Concept ACK

in src/script/script.h:580 in 6ec6e5e0e8 outdated

576@@ -575,6 +577,15 @@ struct CScriptWitness
577     std::string ToString() const;
578 };
579 
580+/** A reference to a CScript: the Hash160 of its serialization (see script.h) */

Sjors commented at 10:27 am on August 10, 2023:

6ec6e5e0e8390d9dca77dfc64bd273860bdbdd93 nit: drop (see script.h)

achow101 commented at 9:41 pm on August 14, 2023:

Done

in src/script/solver.h:4 in 3a6e49e80f outdated

2@@ -3,8 +3,8 @@
3 // Distributed under the MIT software license, see the accompanying
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

Sjors commented at 10:57 am on August 10, 2023:

3a6e49e80fb6c1a7b0ed792cd0611545df63d3da: this is the only file whose scope feels a bit ambiguous. One might be tempted to think it’s a wallet thing, but policy.h uses it too. Suggested comment:

0// Solver methods are used by both policy and the wallet, but not consensus.

Ideally we’d cleanly split policy and wallet related stuff, but this warning should at least prevent someone accidentally changing policy when working on the wallet.

achow101 commented at 9:41 pm on August 14, 2023:

Added comment as suggested

in src/script/standard.cpp:21 in 582708b9e0 outdated

16@@ -17,7 +17,6 @@
17 typedef std::vector<unsigned char> valtype;
18 
19 CScriptID::CScriptID(const CScript& in) : BaseHash(Hash160(in)) {}
20-CScriptID::CScriptID(const ScriptHash& in) : BaseHash(static_cast<uint160>(in)) {}
21 
22 ScriptHash::ScriptHash(const CScript& in) : BaseHash(Hash160(in)) {}

Sjors commented at 1:13 pm on August 10, 2023:

582708b9e0003c1dbe0b4bec1aada8dde9feed24: 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.

in src/script/standard.h:91 in 582708b9e0 outdated

87@@ -90,6 +88,7 @@ struct ScriptHash : public BaseHash<uint160>
88     explicit ScriptHash(const CScript& script);
89     explicit ScriptHash(const CScriptID& script);
90 };
91+CScriptID ToScriptID(const ScriptHash& script_hash);

Sjors commented at 1:17 pm on August 10, 2023:

582708b9e0003c1dbe0b4bec1aada8dde9feed24: can be const (seems to compile fine).

ajtowns commented at 11:16 pm on August 10, 2023:

What would be const? Returning a const object (rather than a const reference/pointer) is largely meaningless.

murchandamus approved

murchandamus commented at 1:53 pm on August 10, 2023: contributor

ACK 3a6e49e80fb6c1a7b0ed792cd0611545df63d3da

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).

Sjors approved

Sjors commented at 2:23 pm on August 10, 2023: member

Concept ACK

However 34aca2d5f26f0441a366942dc56bd220e806d63c 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 3a6e49e80fb6c1a7b0ed792cd0611545df63d3da.

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 b3af9cea5806d26dc3e8f397a1de870065611648?

Sjors commented at 2:40 pm on August 10, 2023: member

Maybe cherry-pick 461259c4ecc1e48d028eaea56061e72a6667ce4f instead of 34aca2d5f26f0441a366942dc56bd220e806d63c. Seems to compile and pass all tests.

in src/script/interpreter.h:157 in 3a6e49e80f outdated

152+ * but in the future other flags may be added.
153+ *
154+ * Failing one of these tests may trigger a DoS ban - see CheckInputScripts() for
155+ * details.
156+ */
157+static const unsigned int MANDATORY_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH;

ajtowns commented at 11:14 pm on August 10, 2023:

Contrary to its comment, this is only used when validating mempool transactions, so is better placed in policy.h

achow101 commented at 9:41 pm on August 14, 2023:

Cherry picked 461259c4ecc1e48d028eaea56061e72a6667ce4f from #26291

in src/wallet/rpc/coins.cpp:675 in 582708b9e0 outdated

671@@ -672,7 +672,7 @@ RPCHelpMan listunspent()
672             std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
673             if (provider) {
674                 if (scriptPubKey.IsPayToScriptHash()) {
675-                    const CScriptID& hash = CScriptID(std::get<ScriptHash>(address));
676+                    const CScriptID& hash = ToScriptID(std::get<ScriptHash>(address));

ajtowns commented at 11:17 pm on August 10, 2023:

Seems strange for this to be a reference (and rely on lifetime extension)?

achow101 commented at 9:42 pm on August 14, 2023:

Dropped the reference

in src/interfaces/wallet.h:11 in 3a6e49e80f outdated

 4@@ -5,10 +5,11 @@
 5 #ifndef BITCOIN_INTERFACES_WALLET_H
 6 #define BITCOIN_INTERFACES_WALLET_H
 7 
 8+#include <addresstype.h>
 9 #include <consensus/amount.h>
10 #include <interfaces/chain.h>          // For ChainClient
11 #include <pubkey.h>                    // For CKeyID and CScriptID (definitions needed in CTxDestination instantiation)

ajtowns commented at 11:55 pm on August 10, 2023:

Maybe fixup the comment on pubkey.h here too

fanquake commented at 12:11 pm on August 14, 2023:

We can just drop any // For ... comments entirely

achow101 commented at 9:42 pm on August 14, 2023:

Dropped those comments.

ajtowns commented at 11:56 pm on August 10, 2023: contributor

Approach ACK

russeree commented at 0:16 am on August 12, 2023: contributor

tACK - Using checklevel of 4, assume valid=0, reindex=1 I was able to sync to the chain tip. This doesn’t guarantee future consensus conflicts but should prove that there are not conflicts validating the ‘main’ chain. Testnet also validated successfully.

Full Settings

 0server=1
 1dbcache=16384
 2par=16
 3rpcthreads=8
 4rpcuser=bitcoin
 5rpcpassword=bitcoin
 6checkblocks=1
 7reindex=1
 8assumevalid=0
 9checklevel=4
10maxconnections=0
11listen=0
12blocksonly=1
13port=23766
14rpcport=25765

Result:

Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h cba69dda3d

Remove ScriptHash from CScriptID constructor

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.

b81ebff0d9

Move CScriptID to script.{h/cpp}

CScriptID should be next to CScript just as CKeyID is next to CPubKey

86ea8bed54

Move Taproot{SpendData/Builder} to signingprovider.{h/cpp}

TaprootSpendData and TaprootBuilder are used in signing in
SigningProvider contexts, so they should live near that.

145f36ec81

Move CTxDestination to its own file

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.

7a172c76d2

MOVEONLY: Move datacarrier defaults to policy.h 8bbe257bac

Clean up things that include script/standard.h

Remove standard.h from files that don't use anything in it, and include
it in files that do.

f3c9078b4c

Clean up script/standard.{h/cpp} includes bacdb2e208

Rename script/standard.{cpp/h} to script/solver.{cpp/h}

Since script/standard only contains things that are used by the Solver
and its callers, rename the files to script/solver.

91d924ede1

achow101 force-pushed on Aug 14, 2023

achow101 commented at 9:42 pm on August 14, 2023: member

Did you use some script for b3af9ce?

Ran IWYU with manual cleanup.

DrahtBot added the label CI failed on Aug 15, 2023

DrahtBot removed the label CI failed on Aug 15, 2023

theStack commented at 11:32 pm on August 15, 2023: contributor

Concept ACK

Sjors commented at 8:43 am on August 16, 2023: member

ACK 91d924ede1b421df31c895f4f43359e453a09ca5

DrahtBot requested review from murchandamus on Aug 16, 2023

in src/script/standard.cpp:37 in b81ebff0d9 outdated

36@@ -38,6 +37,11 @@ CKeyID ToKeyID(const WitnessV0KeyHash& key_hash)
37     return CKeyID{static_cast<uint160>(key_hash)};

MarcoFalke commented at 9:58 am on August 16, 2023:

unrelated: IIUC this creates a useless copy. Might be better to just retain the reference.

MarcoFalke commented at 2:00 pm on August 17, 2023:

Fixed in https://github.com/bitcoin/bitcoin/pull/28284

in src/addresstype.cpp:15 in 7a172c76d2 outdated

10+#include <uint256.h>
11+#include <util/hash_type.h>
12+
13+#include <vector>
14+
15+typedef std::vector<unsigned char> valtype;

MarcoFalke commented at 10:13 am on August 16, 2023:

7a172c76d2361fc3cdf6345590e26c79a7821672: 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)?

in src/addresstype.cpp:13 in 7a172c76d2 outdated

 8+#include <hash.h>
 9+#include <pubkey.h>
10+#include <uint256.h>
11+#include <util/hash_type.h>
12+
13+#include <vector>

MarcoFalke commented at 10:14 am on August 16, 2023:

7a172c76d2361fc3cdf6345590e26c79a7821672: nit:

0addresstype.cpp should add these lines:
1#include <cassert>
2#include "crypto/sha256.h"   // for CSHA256
3#include "span.h"            // for Span

MarcoFalke commented at 2:01 pm on August 17, 2023:

Fixed in https://github.com/bitcoin/bitcoin/pull/28284

in src/script/standard.cpp:13 in bacdb2e208 outdated

12 #include <script/script.h>
13-#include <util/strencodings.h>
14+#include <span.h>
15 
16 #include <string>
17+#include <algorithm>

MarcoFalke commented at 10:23 am on August 16, 2023:

bacdb2e208531124e85ed2d4ea2a4b508fbb5088: nit:

0script/solver.cpp should add these lines:
1#include <cassert>              // for assert
2#include "prevector.h"           // for operator-, prevector

MarcoFalke commented at 2:01 pm on August 17, 2023:

Fixed in https://github.com/bitcoin/bitcoin/pull/28284

in src/addresstype.cpp:5 in 7a172c76d2 outdated

0@@ -0,0 +1,150 @@
1+// Copyright (c) 2023 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
4+
5+#include <addresstype.h>

MarcoFalke commented at 10:24 am on August 16, 2023:

7a172c76d2361fc3cdf6345590e26c79a7821672:nit: missing newline?

MarcoFalke commented at 2:01 pm on August 17, 2023:

Fixed in https://github.com/bitcoin/bitcoin/pull/28284

in src/Makefile.am:661 in 7a172c76d2 outdated

657@@ -657,6 +658,7 @@ libbitcoin_consensus_a_SOURCES = \
658 libbitcoin_common_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
659 libbitcoin_common_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
660 libbitcoin_common_a_SOURCES = \
661+  addresstype.cpp \

MarcoFalke commented at 10:27 am on August 16, 2023:

nit in 7a172c76d2361fc3cdf6345590e26c79a7821672: Could keep this in the script directory, next to script/descriptor.cpp?

darosior commented at 7:59 am on August 17, 2023:

Yes or maybe have a new directory for everything that’s “working with scripts” (addresstype, descriptor, signingprovider, ..).

MarcoFalke approved

MarcoFalke commented at 10:28 am on August 16, 2023: member

ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇
3hmHg3Qo/GQGeCKFKxGmHrg0EmWcpbYzmscRpSuTyu3LmtxqJWEsxftLwps2A5BuOvoJEocpFYxa/YP/1kWeZBw==

murchandamus commented at 8:02 pm on August 16, 2023: contributor

ACK 91d924ede1b421df31c895f4f43359e453a09ca5

ajtowns commented at 0:36 am on August 17, 2023: contributor

ACK 91d924ede1b421df31c895f4f43359e453a09ca5

darosior commented at 8:30 am on August 17, 2023: member

Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.

However i think you can just drop the largest commit here (7a172c76d2361fc3cdf6345590e26c79a7821672) and save a (+291, -247). It would also make more sense IMO. You state:

TaprootSpendData and TaprootBuilder (and their utility functions and structs) are moved to SigningProvider as these are used only during signing.

standard.{cpp/h} is renamed to solver.{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.

theStack approved

theStack commented at 10:24 am on August 17, 2023: contributor

Code-review ACK 91d924ede1b421df31c895f4f43359e453a09ca5

fanquake merged this on Aug 17, 2023

fanquake closed this on Aug 17, 2023

sipa commented at 2:03 pm on August 17, 2023: member

Posthumous concept ACK

sidhujag referenced this in commit 1ee9e390dd on Aug 17, 2023

sidhujag referenced this in commit 8ba3ec962b on Aug 17, 2023

ariard commented at 2:14 am on August 30, 2023: member

Post-merge code review ACK 91d924ed.

Break up script/standard.{h/cpp} #28244

Reviews

Conflicts