The PR branch HEAD was 110239f at the time of this review club meeting.
Notes
Bitcoin script allows users to attach spending conditions to transaction
outputs. A âlockingâ script in a transaction output specifies some spending
condition, such as providing a public key that hashes to a specified digest and
a valid signature for that public key. An âunlockingâ script in a transaction
input supplies the operations and/or data to satisfy that condition.
Transaction validation includes interpreting these scripts and running their
specified operations such as verifying signatures, hashing data, checking
timestamps, etc.
The Bitcoin script
interpreter
includes code that understands all types of scripts and accepts script
verification
flags
to adjust which rules to enforce. Determining which flags to apply is very
important.
Example 1: When a node validates a block, it determines what the
consensus rules are (see
GetBlockScriptFlags,
which also serves as a little tour of Bitcoin consensus history) before
validation. During Initial sync, the consensus rules to apply will change
as the block chain advances.
Example 2: Bitcoin Core nodes also enforce
policy (or
âstandardnessâ) rules in addition to consensus rules when accepting transactions
into their mempool by applying additional script verification
flags.
The SCRIPT_VERIFY_DISCOURAGE_UPGRADEABLE_NOPS policy flag prevents a
node from accepting a transaction into its mempool if that transaction uses
an upgradeable OP_NOP. During activation for CHECKLOCKTIMEVERIFY, for
example, an in-mempool transaction using NOP2 could have been invalidated
between blocks. âDiscouragingâ transactions in this way is preferable to
re-validating all transactions in the mempool after every block.
PR #10699 describes a
procedure for adding new consensus rules via upgradable NOPs or witness
versions which incorporates this idea.
PR #10699 established the precedent
that all script verification flags must be soft forks, i.e. more script
verification flags = more strict spending rules.
PR #19698 extends the
current transaction unit tests by asserting that each test applies flags
precisely by asserting an opposite result when flags are removed or added. The
goal is to ensure that valid transaction tests donât pass due to a lack of
flags and invalid transaction tests donât fail due to an excess of flags.
Questions
What is the difference between PolicyScriptChecks and
ConsensusScriptChecks?
Since the code for taproot is already in interpreter.cpp, why arenât taproot
rules being enforced yet (what condition in the code would it exit on)? How
would we invoke VerifyWitnessProgram to make it apply taproot spending rules?
This PR edits some of the
CheckLockTimeVerify
tests by removing the 1 (push a 1 to the stack) at the end of the scripts.
However, it doesnât do this for all of them
(example).
Why? (Hint: try removing it and running the transaction tests,
src/test/test_bitcoin --log_level=all --run_test=transaction_tests)
<jnewbery> If you've been coming to the meetings for a few weeks/months, then you're ready to host! Take a look at all the people who've hosted before you here: https://bitcoincore.reviews/meetings-hosts/.
<jnewbery> I find that teaching something is one of the best ways of learning. Hosting a review club will force you to understand the PR that week. So if you're serious about improving your understanding of Bitcoin Core, then I'd highly recommend hosting a review club.
<glozow> I just hope that everyone learns something, so feel free to ask any questions :) a lot of the questions today are also more conceptual than implementation
<willcl_ark> As I understand it, policies are soft local rules regarding what our node will create and accept. Consensus rules by contrast are hard limits which define what we accept as valid transactions and blocks on the network.
<pinheadmz> policy is the ruleset that validates entry into the mempool, consensus is the ruleset that validates entry into blocks -- confusing but mempool rules are always *more* restrictive!
<pinheadmz> and IIRC sipa, for soft forks the rules apply to the mempool (policy) before the actual soft fork activation. Does that hold true for taproot? For example with the next release of bitcoind, taproot rules will actually apply in the mempool even though they are not enforced in blocks yet?
<glozow> what I wanted to get to was `PolicyScriptChecks` and `ConsensusScriptChecks` both call `CheckInputScripts`, but with different script verification flags :)
<glozow> ok yes, next question: Since the code for taproot is already in interpreter.cpp, why arenât taproot rules being enforced yet (what condition in the code would it exit on)? How would we invoke `VerifyWitnessProgram` to make it apply taproot spending rules?
<pinheadmz> well this is interesting, because if you look at SCRIPT_VERIFY_TAPROOT -- it is included in the standard verify flags in the code i posted, but then those txs are actually not relayed because of the code sipa posted -- right?
<robot-dreams> I'm guessing a taproot transaction would not make it into the mempool due to the policy flag `SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION`
<elle> im assuming that the SCRIPT_VERIFY_TAPROOT bit is not yet added to the flags variable. Meaning that this line is false: if (VersionBitsState(pindex->pprev, consensusparams, Consensus::DEPLOYMENT_TAPROOT, versionbitscache) == ThresholdState::ACTIVE) {
<sipa> in the mempool, taproot script validation currently happens in 0.21... except that as long as it isn't active on the network, a pre-script policy check will reject it before the script interpreter even runs
<glozow> Letâs dive into the test JSON files :D This PR edits some of the CTV and CSV tests by removing the `1` (push a 1 to the stack) at the end of the scripts. However, it doesnât do this for all of them. Why?
<nehan> why are the policy script checks defined in STANDARD_SCRIPT_VERIFY_FLAGS in policy.h but the consensus script checks are enumerated separately in the function GetBlockScriptFlags?
<pinheadmz> if the CLTV failed though, these examples would fail technically with n empty stack because the 1 at the end would never get pushed to the stack
<glozow> and yes, sorry nehan, i forgot to get back to you on your question - block script flags change from block to block, so we re-calculate them every time
<sipa> pinheadmz: if the CLTV fails the script immediately aborts entirely with failure; no code after it is executed anymore, but that's not really relevant if we've already aborted
<glozow> NEXT! What does `TrimFlags` do? (Extra credit question: why does SCRIPT_VERIFY_CLEANSTACK need to be used with SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS?)
<robot-dreams> I'm a bit behind... I understand why we cannot take off the 1 at the end of "0 CLTV 1", but just to confirm, why DO we take off the 1 at the end of "499999999 CLTV 1"?
<murch> So previously the test was merely considering the tx valid because we didn't enforce the cleanstack rule, and now that we're defaulting to all flags, it would fail on the cleanstack rule
<murch> glozow: Valid txes should pass with all flags that are not explicitly excluded in this specific transactino, and invalid txes should fail on exactly the specific flag that's tested
<robot-dreams> I had a question about that, would it be helpful to try every valid *combination* of unset flags, or would that blow up the test time way too much?
<michaelfolkson> Just to confirm sipa. For CLEANSTACK you have to have a stack with a single element in it which has to be nonzero. Without CLEANSTACK a non-empty stack with multiple elements is ok as long as the top element is nonzero?
<robot-dreams> michaelfolkson: Thanks! Just to confirm, in this PR would it be necessary to change `0 CLTV 1` into `0 CLTV DROP 1` as sipa mentioned above?
<michaelfolkson> sipa: And so without CLEANSTACK you could have a horrible non-empty stack with TRUE FALSE 3 and it would pass. I don't think I understand this...
<felixweis> michaelfolkson: maybe some fancy script designers wanted to do multiple conditional execution paths and the shortest definition to do both didn't leave the stack clean
<sipa> michaelfolkson: i think you have to see bitcoin script as a programming language, and that language used to be defined without cleanstack... so you write scripts that do what you want given those semantics
<michaelfolkson> Ok I guess I'm surprised because I always thought a dirty stack (in the context of CLEANSTACK) would always fail. I misunderstood the other ways a script could still pass
<sipa> dhruvm: i expect that in practice this is not a problem; if there were an actual softforkability bug somewhere that these tests catch, you'll suddenly have tons of combinations fail
<robot-dreams> sdaftaur: Is this related to the comment at the start of `GetBlockScriptFlags` that says "one historical block violated the P2SH rules"?