Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY

sdaftuar commented at 8:31 pm on November 20, 2017: member

As discussed at the IRC meeting back in October (https://botbot.me/freenode/bitcoin-core-dev/2017-10-12/?msg=92231929&page=2), I had looked into the feasibility of enforcing P2SH and SCRIPT_VERIFY_WITNESS back to the genesis block.

The P2SH change is pretty straightforward – there was only one historical block on mainnet that violated the rule, so I carved out an exception to it, similar to the way we have exceptions for the BIP30 violators.

The segwit change is not entirely as clear. The code changes themselves are relatively straightforward: we can just always turn on SCRIPT_VERIFY_WITNESS whenever P2SH is active. However conceptually, this amounts to splitting up BIP141 into two parts, the part that implements new script rules, and the part that handles witness commitments in blocks.

Arguably though the script rules are really defined in BIP 143 anyway, and so this really amounts to backdating BIP 143 – script rules for v0 segwit outputs – back to genesis. So maybe conceptually this isn’t so bad…

I don’t feel strongly about this change in either direction; I started working on it because I was searching for a way to simplify the way we understand and implement the consensus rules around segwit, but I’m not yet sure whether I think this achieves anything toward that goal.

ping @TheBlueMatt

sdaftuar commented at 8:36 pm on November 20, 2017: member

Forgot to flag this: there is one mainnet block that violated P2SH, and one testnet block as well. For now I carved out both, but perhaps we could consider migration to a new testnet (4?) instead and drop the testnet exception? Seems like it might be cleaner to have a new testnet that always has P2SH and segwit from genesis…. Does anyone think this would be worth proposing?

fanquake added the label Validation on Nov 20, 2017

in src/validation.cpp:1628 in 3a76e3c962 outdated

1624+    }
1625+
1626+    unsigned int flags = fStrictPayToScriptHash ? SCRIPT_VERIFY_P2SH : SCRIPT_VERIFY_NONE;
1627 
1628     // Start enforcing P2SH (BIP16)
1629     if (pindex->nHeight >= consensusparams.BIP16Height) {

MarcoFalke commented at 11:31 am on November 21, 2017:

fixup nit: in commit 3a76e3c962516cdf7d577aa3dcbe0251b94e5355: You probably meant to use if (fStrictPayToScriptHash) { flags |= SCRIPT_VERIFY_P2SH; }, as BIP16Height was removed and thus fails to compile.

TheBlueMatt commented at 0:10 am on November 22, 2017: member

Concept ACK. At a code level not so interesting, I suppose (except for maybe the P2SH backdating), but it’s generally a nice property for us to, over time, enforce soft forks like SegWit universally.

On November 20, 2017 12:32:05 PM PST, Suhas Daftuar notifications@github.com wrote:

As discussed at the IRC meeting back in October (https://botbot.me/freenode/bitcoin-core-dev/2017-10-12/?msg=92231929&page=2), I had looked into the feasibility of enforcing P2SH and SCRIPT_VERIFY_WITNESS back to the genesis block.

The P2SH change is pretty straightforward – there was only one historical block on mainnet that violated the rule, so I carved out an exception to it, similar to the way we have exceptions for the BIP30 violators.

The segwit change is not entirely as clear. The code changes themselves are relatively straightforward: we can just always turn on SCRIPT_VERIFY_WITNESS whenever P2SH is active. However conceptually, this amounts to splitting up BIP141 into two parts, the part that implements new script rules, and the part that handles witness commitments in blocks.

Arguably though the script rules are really defined in BIP 143 anyway, and so this really amounts to backdating BIP 143 – script rules for v0 segwit outputs – back to genesis. So maybe conceptually this isn’t so bad…

I don’t feel strongly about this change in either direction; I started working on it because I was searching for a way to simplify the way we understand and implement the consensus rules around segwit, but I’m not yet sure whether I think this achieves anything toward that goal.

ping @TheBlueMatt You can view, comment on, or merge this pull request online at:

#11739

– Commit Summary –

Use P2SH consensus rules for all blocks

Separate NULLDUMMY enforcement from SEGWIT enforcement

Always enforce SCRIPT_VERIFY_WITNESS with P2SH

[qa] Remove some pre-activation segwit tests

scripted-diff: rename IsWitnessEnabled -> IsWitnessCommitmentEnabled

– File Changes –

M src/chainparams.cpp (3) M src/consensus/params.h (2) M src/miner.cpp (2) M src/net_processing.cpp (8) M src/validation.cpp (49) M src/validation.h (5) M src/wallet/rpcwallet.cpp (2) M test/functional/p2p-segwit.py (2) M test/functional/segwit.py (8)

– Patch Links –

https://github.com/bitcoin/bitcoin/pull/11739.patch https://github.com/bitcoin/bitcoin/pull/11739.diff

– You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11739

sipa commented at 1:44 am on November 22, 2017: member

The code changes here don’t look as bad as I would have expected. I’d prefer to encapsulate the exception block hashes in chainparams, though.

gmaxwell commented at 1:36 am on November 23, 2017: contributor

Concept ACK! as far as testnet, restarting with a new testnet would be fine, but there are a bunch of very interesting test cases in testnet we should consider extracting. One possibility would be to instrument the codebase to call the gprof things to record branch coverage data at each block; an then make a list of testnet blocks that increased cover. Seems like a real pain though.

sdaftuar force-pushed on Dec 5, 2017

sdaftuar commented at 10:21 pm on December 5, 2017: member

Nits addressed.

in src/validation.cpp:1624 in ad3e97a109 outdated

1621-    // Start enforcing P2SH (BIP16)
1622-    if (pindex->nHeight >= consensusparams.BIP16Height) {
1623+    // BIP16 didn't become active until Apr 1 2012
1624+    // However, only one historical block violated the P2SH rules, so for simplicity, always leave P2SH
1625+    // on except for the one violating block.
1626+    if (!(pindex->phashBlock != nullptr && (*(pindex->phashBlock) == consensusparams.BIP16Exception))) {

TheBlueMatt commented at 6:24 pm on December 8, 2017:

Why not assert pindex->phashBlock? No properly-constructed CBlockIndex should have that unset, no?

sdaftuar commented at 5:35 pm on December 11, 2017:

It’s unset when invoked via TestBlockValidity() (eg for mining).

TheBlueMatt commented at 6:26 pm on December 12, 2017:

Ah, that sounds like a bug in TBV, but that’s for a separate PR, I suppose.

jtimon commented at 2:43 am on January 23, 2018:

Yes, please, I always found checks to pindex->phashBlock == nullptr ugly and expensive to maintain documented (perhaps we can repeat the “pindex->phashBlock can be null if called by CreateNewBlock/TestBlockValidity” comment from the beginning and unify it with the comment for fEnforceBIP30, to more easily delete them all later when they’re not needed). There’s the assert you want below, just needs to be moved to the top once the special cases are dealt with more explicitly. But, yeah, that’s for a separate PR.

TheBlueMatt commented at 6:27 pm on December 8, 2017: member

utACK ad3e97a109541bb644dc1cb5e264a2954926bcac

laanwj commented at 4:32 pm on December 20, 2017: member

Concept ACK

in src/validation.cpp:1619 in df7fad0a0b outdated

1612@@ -1613,8 +1613,10 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1613 
1614     unsigned int flags = SCRIPT_VERIFY_NONE;
1615 
1616-    // Start enforcing P2SH (BIP16)
1617-    if (pindex->nHeight >= consensusparams.BIP16Height) {
1618+    // BIP16 didn't become active until Apr 1 2012
1619+    // However, only one historical block violated the P2SH rules, so for simplicity, always leave P2SH
1620+    // on except for the one violating block.
1621+    if (!(pindex->phashBlock != nullptr && (*(pindex->phashBlock) == consensusparams.BIP16Exception))) {

ryanofsky commented at 10:17 pm on January 10, 2018:

In commit “Use P2SH consensus rules for all blocks”

Maybe check BIP16Exception.IsNull() to be a little more strict. Could also remove unnecessary nesting and negatives:

0if (consensusparams.BIP16Exception.IsNull() ||
1    pindex->phashBlock == nullptr ||
2    *pindex->phashBlock != consensusparams.BIP16Exception)

in test/functional/p2p-segwit.py:1908 in 1f4efbceec outdated

1904@@ -1905,7 +1905,7 @@ def run_test(self):
1905         self.test_unnecessary_witness_before_segwit_activation()
1906         self.test_witness_tx_relay_before_segwit_activation()
1907         self.test_block_relay(segwit_activated=False)
1908-        self.test_p2sh_witness(segwit_activated=False)
1909+        #self.test_p2sh_witness(segwit_activated=False)

ryanofsky commented at 10:20 pm on January 10, 2018:

In commit “[qa] Remove some pre-activation segwit tests”

Should actually remove

in test/functional/segwit.py:192 in 1f4efbceec outdated

188@@ -189,17 +189,13 @@ def run_test(self):
189 
190         # TODO: An old node would see these txs without witnesses and be able to mine them
191 
192-        self.log.info("Verify unsigned bare witness txs in versionbits-setting blocks are valid before the fork")
193-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V0][1], False) #block 428
194-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V1][1], False) #block 429
195+        # TODO: verify spending a segwit v0 output is invalid before the fork

ryanofsky commented at 10:20 pm on January 10, 2018:

In commit “[qa] Remove some pre-activation segwit tests”

Note: TODO

jnewbery commented at 10:18 pm on January 23, 2018:

I don’t understand this comment. What does ‘before the fork’ mean if segwit is active since genesis?

ryanofsky commented at 10:25 pm on January 10, 2018: member

utACK ad3e97a109541bb644dc1cb5e264a2954926bcac

gmaxwell commented at 8:49 pm on January 11, 2018: contributor

Tested ACK

sdaftuar commented at 8:04 pm on January 22, 2018: member

Addressed @ryanofsky’s comments. However I think I will need to rebase in order to for the new test I wrote to work…

jtimon commented at 2:45 am on January 23, 2018: contributor

This needs a BIP IMO. Concept ACK. if the segwit case is not clear yet, perhaps it should be left for later (in fact, I would prefer the 2 things in 2 different PRs either way, preferrably one on top of the other [perhaps open one with only p2sh and rebase this one of top of the new one], but all between the parenthesis is just PR bikeshedding).

utACK individual commits df7fad0a0b0f02ab2193673bf16ebffc5318778d d32bcd91c7748feb559848bf4db6da8fe4715494

sdaftuar force-pushed on Jan 23, 2018

sdaftuar commented at 5:44 pm on January 23, 2018: member

Rebased and fixed the last commit to work with latest master. Previous version of these commits is at https://github.com/sdaftuar/bitcoin/commits/11739.1

sdaftuar commented at 5:51 pm on January 23, 2018: member

@jtimon Agreed – I sent an email to the bitcoin-dev list recently with a link to the draft BIP text.

sdaftuar renamed this:
~~RFC: Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis~~
Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis
on Jan 23, 2018

in src/net_processing.cpp:860 in b782474f54 outdated

856@@ -857,7 +857,7 @@ void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std:
857         return;
858     nHighestFastAnnounce = pindex->nHeight;
859 
860-    bool fWitnessEnabled = IsWitnessEnabled(pindex->pprev, Params().GetConsensus());
861+    bool fWitnessEnabled = IsWitnessCommitmentEnabled(pindex->pprev, Params().GetConsensus());

jnewbery commented at 10:26 pm on January 23, 2018:

Could also change the name of the local variable here:

fWitnessEnabled -> witness_commitment_enabled

in src/chainparams.cpp:80 in b782474f54 outdated

74@@ -75,9 +75,9 @@ class CMainParams : public CChainParams {
75     CMainParams() {
76         strNetworkID = "main";
77         consensus.nSubsidyHalvingInterval = 210000;
78-        consensus.BIP16Height = 173805; // 00000000000000ce80a7e057163a4db1d5ad7b20fb6f598c9597b9665c8fb0d4 - April 1, 2012
79         consensus.BIP34Height = 227931;
80         consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
81+        consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");

jnewbery commented at 10:26 pm on January 23, 2018:

nit: prefer the BIP constants in numeric order (ie BIP 16 above BIP 34). Same for params.h below.

in test/functional/p2p-segwit.py:281 in b782474f54 outdated

276+    def test_v0_outputs_arent_spendable(self):
277+        self.log.info("Testing that v0 witness program outputs aren't spendable pre-activation")
278+
279+        assert(len(self.utxo))
280+
281+        # Create two outputs, a p2wsh and p2sh-p2wsh

jnewbery commented at 11:20 pm on January 23, 2018:

comment says p2sh-p2wsh, but the second output below is just a p2sh as far as I can tell.

jnewbery commented at 11:34 pm on January 23, 2018:

strange. The P2SH output can’t be spent either. If I change the test_witness_block() call to a test_transaction_acceptance() call below, I see the following log:

(mandatory-script-verify-flag-failed (Operation not valid with the current stack size) (code 16))

sdaftuar commented at 4:31 pm on January 25, 2018:

Fixed – I forgot to stick the p2sh redeem script in the scriptsig (and also inadvertently made this a p2sh, rather than p2sh-p2wsh).

jnewbery commented at 11:25 pm on January 23, 2018: member

utACK up to 4e2b90d21690059dc496a17d2aee9fe11ce3a8b2.

I don’t think the final commit adds any value and can be removed from this PR.

sdaftuar force-pushed on Jan 25, 2018

sdaftuar commented at 4:38 pm on January 25, 2018: member

Github thought this needed to be rebased, so I did, though there were no conflicts to be resolved. Original unsquashed version is https://github.com/sdaftuar/bitcoin/commits/11739.2.

in test/functional/feature_segwit.py:153 in 3bece54209 outdated

149@@ -150,17 +150,13 @@ def run_test(self):
150 
151         # TODO: An old node would see these txs without witnesses and be able to mine them
152 
153-        self.log.info("Verify unsigned bare witness txs in versionbits-setting blocks are valid before the fork")
154-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V0][1], False) #block 428
155-        self.success_mine(self.nodes[2], wit_ids[NODE_2][WIT_V1][1], False) #block 429
156+        # TODO: verify spending a segwit v0 output is invalid before the fork

jnewbery commented at 5:34 pm on January 25, 2018:

I wouldn’t bother adding this comment, which you’re just going to remove in a later commit. I think you can also remove the comment above.

sdaftuar force-pushed on Jan 25, 2018

sdaftuar commented at 5:45 pm on January 25, 2018: member

I decided to remove the scripted-diff commit, because in retrospect I think the existing function name, IsWitnessEnabled, is fine.

Addressed nits from @jnewbery and @ryanofsky and squashed. Old version is: https://github.com/sdaftuar/bitcoin/commits/11739.3

sdaftuar force-pushed on Jan 25, 2018

sdaftuar commented at 11:35 pm on January 25, 2018: member

Updated the last commit (which introduces a new test) with some fixes and cleanups suggested by @jnewbery. Prior version of this PR is 11739.4

sdaftuar force-pushed on Jan 26, 2018

sdaftuar commented at 2:47 pm on January 26, 2018: member

Pushed two more commits to fix a comment pointed out by @jnewbery, and fix a test bug that travis triggered. Travis is passing now. Squashed 11739.5 -> 9080e76f3e5a2afb6352aa79763e6d4d976d7cce

jnewbery commented at 4:16 pm on January 26, 2018: member

Tested ACK 9080e76f3e5a2afb6352aa79763e6d4d976d7cce.

Test changes look good. It’d be nice to do some general tidy-up to the segwit functional tests (p2p_segwit.py and feature_segwit.py), but we should leave that for another PR after #11398 is merged.

in src/chainparams.cpp:78 in 9080e76f3e outdated

74@@ -75,7 +75,7 @@ class CMainParams : public CChainParams {
75     CMainParams() {
76         strNetworkID = "main";
77         consensus.nSubsidyHalvingInterval = 210000;
78-        consensus.BIP16Height = 173805; // 00000000000000ce80a7e057163a4db1d5ad7b20fb6f598c9597b9665c8fb0d4 - April 1, 2012
79+        consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");

Sjors commented at 7:17 pm on February 1, 2018:

Nit: if we want to reuse this pattern: consensus.BIP16Exceptions = [...]?

in src/chainparams.cpp:193 in 9080e76f3e outdated

185@@ -186,7 +186,7 @@ class CTestNetParams : public CChainParams {
186     CTestNetParams() {
187         strNetworkID = "test";
188         consensus.nSubsidyHalvingInterval = 210000;
189-        consensus.BIP16Height = 514; // 00000000040b4e986385315e14bee30ad876d8b47f748025b26683116d21aa65
190+        consensus.BIP16Exception = uint256S("0x00000000dd30457c001f4095d208cc1296b0eed002427aa599874af7a432b105");

Sjors commented at 7:18 pm on February 1, 2018:

Put a TODO here to remove it when the next testnet comes?

instagibbs commented at 3:43 pm on March 7, 2018:

imo it should just have a completely new set of chainparams, in other words leave as is

Sjors commented at 7:36 pm on February 1, 2018: member

Concept ACK. This sort of thing is also useful for sidechain-like projects where you want to track the bitcoin core codebase with ideally zero patches.

Sjors commented at 11:12 am on February 2, 2018: member

Should we also remove the relevant soft fork entries from getblockchaininfo?

jnewbery commented at 2:38 pm on February 2, 2018: member

Should we also remove the relevant soft fork entries from getblockchaininfo?

BIP 16 already is not shown in getblockchaininfo (it was a flag day upgrade rather than IsSuperMajority deployment). This PR doesn’t move the segwit softfork height, it just enforces the SCRIPT_VERIFY_WITNESS rules to genesis.

laanwj referenced this in commit 3843780fd8 on Feb 8, 2018

laanwj referenced this in commit d59b8d6aa1 on Mar 5, 2018

in src/validation.cpp:1733 in 69ba5ecdec outdated

1729@@ -1730,8 +1730,13 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1730 
1731     unsigned int flags = SCRIPT_VERIFY_NONE;
1732 
1733-    // Start enforcing P2SH (BIP16)
1734-    if (pindex->nHeight >= consensusparams.BIP16Height) {
1735+    // BIP16 didn't become active until Apr 1 2012

instagibbs commented at 3:53 pm on March 7, 2018:

mu-nit: this comment is for mainnet only, though both mainnet and testnet has an instance. Perhaps discuss the date in chainparams?

sdaftuar commented at 1:20 pm on April 13, 2018:

Actually, this comment applies to testnet3 as well! See #1503.

instagibbs commented at 1:25 pm on April 13, 2018:

For the benefit of code readers, maybe say that as well :)

in src/validation.cpp:1748 in 8b975bc6c6 outdated

1744@@ -1740,6 +1745,11 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
1745         flags |= SCRIPT_VERIFY_P2SH;
1746     }
1747 
1748+    // Enforce WITNESS rules whenever P2SH is in effect.

instagibbs commented at 4:14 pm on March 7, 2018:

“Enforce WITNESS rules whenever P2SH is in effect and witness deployment is not defined.”

… or something

sdaftuar commented at 1:29 pm on April 13, 2018:

I’ve improved the comments in latest version.

instagibbs approved

instagibbs commented at 4:37 pm on March 7, 2018: member

I am unable to comment on the changes here as I don’t understand that section of code: https://github.com/bitcoin/bitcoin/pull/11739/commits/8b975bc6c6f3fd3889ba3f907d9e4a663525d240#diff-24efdb00bfbe56b140fb006b562cc70bR4034

I have also not tested the exact hashes given.

utACK otherwise

sdaftuar force-pushed on Apr 13, 2018

sdaftuar commented at 1:31 pm on April 13, 2018: member

Updated comments to address nit from @instagibbs, and squashed (previous version is https://github.com/sdaftuar/bitcoin/commits/11739.6 for comparison).

sdaftuar force-pushed on Apr 13, 2018

sdaftuar commented at 1:37 pm on April 13, 2018: member

Updated again to improve code comment around historical p2sh activation. (Compare: https://github.com/sdaftuar/bitcoin/commits/11739.7)

Use P2SH consensus rules for all blocks

This commit moves P2SH activation back to the genesis block, with
a hardcoded exception for the one historical block in the chain that
violated this rule.

ce650182f4

Separate NULLDUMMY enforcement from SEGWIT enforcement

This is in preparation for enforcing SCRIPT_VERIFY_WITNESS from
the genesis block.

95749a5836

[qa] Remove some pre-activation segwit tests

This is in preparation for always enforcing SCRIPT_VERIFY_WITNESS.

5c31b20a35

Always enforce SCRIPT_VERIFY_WITNESS with P2SH ccb8ca42a4

[qa] Test that v0 segwit outputs can't be spent pre-activation

Also updates the comments for an existing test, that now should be rewritten.
Includes changes suggested by John Newbery.

8b56fc0b91

sdaftuar force-pushed on Apr 13, 2018

sdaftuar commented at 2:36 pm on April 13, 2018: member

Needed rebase.

jnewbery commented at 3:47 pm on April 13, 2018: member

Tested ACK 8b56fc0b91eb4876004bdc92a7015b12b34a04ed

Travis failure was unrelated p2p_compactblocks.py flakiness. I’ve restared the job.

It’d be great to get this (and then #12360) in before v0.17.

sdaftuar commented at 3:28 pm on April 16, 2018: member

Tests are passing now.

laanwj added this to the "Blockers" column in a project

TheBlueMatt commented at 7:44 pm on April 17, 2018: member

non-test-changes-re-utACK 8b56fc0b91eb4876004bdc92a7015b12b34a04ed (though I miss the s/IsWitnessEnabled/IsWitnessCommitmentEnabled/ scripted-diff change).

MarcoFalke commented at 6:01 pm on April 19, 2018: member

utACK 8b56fc0b91eb4876004bdc92a7015b12b34a04ed

MarcoFalke merged this on Apr 19, 2018

MarcoFalke closed this on Apr 19, 2018

MarcoFalke referenced this in commit 0a8b7b4b33 on Apr 19, 2018

fanquake removed this from the "Blockers" column in a project

jasonbcox referenced this in commit 408db0d765 on May 3, 2019

jonspock referenced this in commit 73d9ef96d8 on Jun 5, 2019

PastaPastaPasta referenced this in commit 168e93162f on Jun 9, 2020

PastaPastaPasta referenced this in commit 7e04a9a3b1 on Jun 9, 2020

PastaPastaPasta referenced this in commit 8254d30d21 on Jun 10, 2020

PastaPastaPasta referenced this in commit 55cc7d8f89 on Jun 10, 2020

PastaPastaPasta referenced this in commit 35bd99881b on Jun 10, 2020

PastaPastaPasta referenced this in commit da0042972b on Jun 11, 2020

PastaPastaPasta referenced this in commit 1b7e6b45af on Jun 11, 2020

PastaPastaPasta referenced this in commit 8e66148ac6 on Jun 12, 2020

PastaPastaPasta referenced this in commit 5303822ef4 on Jun 13, 2020

PastaPastaPasta referenced this in commit b5b1a9261c on Jun 14, 2020

PastaPastaPasta referenced this in commit 4e26e4e72c on Jun 14, 2020

MarcoFalke locked this on Sep 8, 2021

Enforce SCRIPT_VERIFY_P2SH and SCRIPT_VERIFY_WITNESS from genesis #11739