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

achow101 · 2023-08-09T11:43:57Z

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 · 2023-08-09T11:44:00Z

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.

src/script/solver.h

ariard

Concept ACK

ariard · 2023-08-10T01:52:09Z

src/script/solver.h

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.

leaving as is since it is only moved.

murchandamus

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

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?

src/script/script.h

src/script/solver.h

Sjors · 2023-08-10T13:13:26Z

src/script/standard.cpp

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.

src/script/standard.h

Sjors · 2023-08-10T14:40:24Z

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

ajtowns

Approach ACK

src/script/interpreter.h

src/script/standard.h

src/wallet/rpc/coins.cpp

src/interfaces/wallet.h

portlandhodl · 2023-08-12T00:16:57Z

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

server=1
dbcache=16384
par=16
rpcthreads=8
rpcuser=bitcoin
rpcpassword=bitcoin
checkblocks=1
reindex=1
assumevalid=0
checklevel=4
maxconnections=0
listen=0
blocksonly=1
port=23766
rpcport=25765

Result:

…olicy.h

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.

achow101 · 2023-08-14T21:42:33Z

Did you use some script for b3af9ce?

Ran IWYU with manual cleanup.

theStack · 2023-08-15T23:32:39Z

Concept ACK

Sjors · 2023-08-16T08:43:58Z

ACK 91d924e

maflcko

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==

maflcko · 2023-08-16T09:58:47Z

src/script/standard.cpp


 CKeyID ToKeyID(const WitnessV0KeyHash& key_hash)
 {
    return CKeyID{static_cast<uint160>(key_hash)};


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

Fixed in #28284

maflcko · 2023-08-16T10:13:48Z

src/addresstype.cpp

+
+#include <vector>
+
+typedef std::vector<unsigned char> valtype;


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

maflcko · 2023-08-16T10:14:22Z

src/addresstype.cpp

+#include <uint256.h>
+#include <util/hash_type.h>
+
+#include <vector>


7a172c7: nit:

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

Fixed in #28284

maflcko · 2023-08-16T10:23:25Z

src/script/standard.cpp

+#include <span.h>

 #include <string>
+#include <algorithm>


bacdb2e: nit:

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

Fixed in #28284

maflcko · 2023-08-16T10:24:25Z

src/addresstype.cpp

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <addresstype.h>


7a172c7:nit: missing newline?

Fixed in #28284

maflcko · 2023-08-16T10:27:34Z

src/Makefile.am

 libbitcoin_common_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
 libbitcoin_common_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libbitcoin_common_a_SOURCES = \
+  addresstype.cpp \


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

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

murchandamus

ACK 91d924e

ajtowns · 2023-08-17T00:36:43Z

ACK 91d924e

darosior

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

darosior · 2023-08-17T07:59:42Z

src/Makefile.am

 libbitcoin_common_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
 libbitcoin_common_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libbitcoin_common_a_SOURCES = \
+  addresstype.cpp \


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

theStack

Code-review ACK 91d924e

sipa · 2023-08-17T14:03:27Z

Posthumous concept ACK

ariard · 2023-08-30T02:14:19Z

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

achow101 force-pushed the ctxdest-addr branch from bb14106 to cc898bf Compare August 9, 2023 11:56

DrahtBot added the CI failed label Aug 9, 2023

fanquake reviewed Aug 9, 2023

View reviewed changes

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

achow101 force-pushed the ctxdest-addr branch from cc898bf to 3a6e49e Compare August 9, 2023 13:08

achow101 mentioned this pull request Aug 9, 2023

wallet: Use CTxDestination in CRecipient instead of just scriptPubKey #28246

Merged

This was referenced Aug 9, 2023

Add Import to Wallet GUI bitcoin-core/gui#650

Closed

rpc: Add MaybeArg() and Arg() default helper #28230

Merged

refactor: Remove unused includes from wallet.cpp #28200

Merged

test: add script compression coverage for not-on-curve P2PK outputs #28193

Merged

DrahtBot removed the CI failed label Aug 9, 2023

ariard reviewed Aug 10, 2023

View reviewed changes

This was referenced Aug 10, 2023

policy: Ephemeral anchors #26403

Closed

Update MANDATORY_SCRIPT_VERIFY_FLAGS #26291

Merged

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

Closed

Implement BIP 370 PSBTv2 #21283

Open

murchandamus approved these changes Aug 10, 2023

View reviewed changes

Sjors approved these changes Aug 10, 2023

View reviewed changes

ajtowns reviewed Aug 10, 2023

View reviewed changes

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

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

src/wallet/rpc/coins.cpp Outdated Show resolved Hide resolved

src/interfaces/wallet.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 11, 2023

Silent Payments: Implement BIP352 #28122

Draft

ajtowns and others added 4 commits August 14, 2023 17:38

Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/p…

cba69dd

…olicy.h

Remove ScriptHash from CScriptID constructor

b81ebff

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.

Move CScriptID to script.{h/cpp}

86ea8be

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

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

145f36e

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

achow101 added 5 commits August 14, 2023 17:38

Move CTxDestination to its own file

7a172c7

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.

MOVEONLY: Move datacarrier defaults to policy.h

8bbe257

Clean up things that include script/standard.h

f3c9078

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

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

bacdb2e

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

91d924e

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

achow101 force-pushed the ctxdest-addr branch from 3a6e49e to 91d924e Compare August 14, 2023 21:41

DrahtBot added the CI failed label Aug 15, 2023

DrahtBot mentioned this pull request Aug 15, 2023

policy: make unstructured annex standard #27926

Closed

1 task

DrahtBot removed the CI failed label Aug 15, 2023

DrahtBot mentioned this pull request Aug 15, 2023

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

DrahtBot requested a review from murchandamus August 16, 2023 08:44

maflcko approved these changes Aug 16, 2023

View reviewed changes

murchandamus reviewed Aug 16, 2023

View reviewed changes

darosior reviewed Aug 17, 2023

View reviewed changes

theStack approved these changes Aug 17, 2023

View reviewed changes

fanquake merged commit 7ef2d4e into bitcoin:master Aug 17, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2023

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

1ee9e39

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 17, 2023

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

8ba3ec9

ryanofsky mentioned this pull request Aug 21, 2023

Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051

Merged

josibake mentioned this pull request Sep 26, 2023

BIP352 tracking issue #28536

Open

15 tasks

bitcoin locked and limited conversation to collaborators Aug 29, 2024


		#include <vector>

		typedef std::vector<unsigned char> valtype;

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

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

Uh oh!

Conversation

achow101 commented Aug 9, 2023

Uh oh!

DrahtBot commented Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murchandamus 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Aug 10, 2023

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

portlandhodl commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Aug 14, 2023

Uh oh!

theStack commented Aug 15, 2023

Uh oh!

Sjors commented Aug 16, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Aug 17, 2023

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

DrahtBot commented Aug 9, 2023 •

edited

Loading

portlandhodl commented Aug 12, 2023 •

edited

Loading