Merge bitcoin/bitcoin#21090: Default to NODE_WITNESS in nLocalServices

a806647d26 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db7 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b82 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd64 [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)

Pull request description:

  Builds on #21009 and makes progress on remaining items in #17862

  Removing `RewindBlockIndex()` in #21009 allows the following:

  - removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
  - move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
  - in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
  - that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
  - that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`

ACKs for top commit:
  mzumsande:
    Code-Review ACK a806647d26
  laanwj:
    Code review ACK a806647d26, nice cleanup
  jnewbery:
    utACK a806647d26
  theStack:
    ACK a806647d26

Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e

src/chainparams.cpp

@@ -490,11 +490,8 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
 {
     if (args.IsArgSet("-segwitheight")) {
         int64_t height = args.GetArg("-segwitheight", consensus.SegwitHeight);
-        if (height < -1 || height >= std::numeric_limits<int>::max()) {
+        if (height < 0 || height >= std::numeric_limits<int>::max()) {
-            throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range. Use -1 to disable segwit.", height));
+            throw std::runtime_error(strprintf("Activation height %ld for segwit is out of valid range.", height));
-        } else if (height == -1) {
-            LogPrintf("Segwit disabled for testing\n");
-            height = std::numeric_limits<int>::max();
         }
         consensus.SegwitHeight = static_cast<int>(height);
     }

src/chainparamsbase.cpp

@@ -20,7 +20,7 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
     argsman.AddArg("-chain=<chain>", "Use the chain <chain> (default: main). Allowed values: main, test, signet, regtest", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
                  "This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
-    argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
     argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);

src/init.cpp

@@ -715,7 +715,7 @@ namespace { // Variables internal to initialization process only
 int nMaxConnections;
 int nUserMaxConnections;
 int nFD;
-ServiceFlags nLocalServices = ServiceFlags(NODE_NETWORK | NODE_NETWORK_LIMITED);
+ServiceFlags nLocalServices = ServiceFlags(NODE_NETWORK | NODE_NETWORK_LIMITED | NODE_WITNESS);
 int64_t peer_connect_timeout;
 std::set<BlockFilterType> g_enabled_filter_types;
 
@@ -1588,12 +1588,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         }
     }
 
-    if (DeploymentEnabled(chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) {
-        // Advertise witness capabilities.
-        // The option to not set NODE_WITNESS is only used in the tests and should be removed.
-        nLocalServices = ServiceFlags(nLocalServices | NODE_WITNESS);
-    }
-
     // ********************************************************* Step 11: import blocks
 
     if (!CheckDiskSpace(gArgs.GetDataDirNet())) {

src/net_processing.cpp

@@ -877,7 +877,7 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid)
         }
         m_connman.ForNode(nodeid, [this](CNode* pfrom) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
             AssertLockHeld(::cs_main);
-            uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
+            uint64_t nCMPCTBLOCKVersion = 2;
             if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
                 // As per BIP152, we only get 3 of our peers to announce
                 // blocks using compact encodings.
@@ -1973,7 +1973,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
 
 static uint32_t GetFetchFlags(const CNode& pfrom) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
     uint32_t nFetchFlags = 0;
-    if ((pfrom.GetLocalServices() & NODE_WITNESS) && State(pfrom.GetId())->fHaveWitness) {
+    if (State(pfrom.GetId())->fHaveWitness) {
         nFetchFlags |= MSG_WITNESS_FLAG;
     }
     return nFetchFlags;
@@ -2688,8 +2688,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             // they may wish to request compact blocks from us
             bool fAnnounceUsingCMPCTBLOCK = false;
             uint64_t nCMPCTBLOCKVersion = 2;
-            if (pfrom.GetLocalServices() & NODE_WITNESS)
+            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
-                m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
             nCMPCTBLOCKVersion = 1;
             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
         }
@@ -2707,7 +2706,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         bool fAnnounceUsingCMPCTBLOCK = false;
         uint64_t nCMPCTBLOCKVersion = 0;
         vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
-        if (nCMPCTBLOCKVersion == 1 || ((pfrom.GetLocalServices() & NODE_WITNESS) && nCMPCTBLOCKVersion == 2)) {
+        if (nCMPCTBLOCKVersion == 1 || nCMPCTBLOCKVersion == 2) {
             LOCK(cs_main);
             // fProvidesHeaderAndIDs is used to "lock in" version of compact blocks we send (fWantsCmpctWitness)
             if (!State(pfrom.GetId())->fProvidesHeaderAndIDs) {
@@ -2721,10 +2720,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                 pfrom.m_bip152_highbandwidth_from = fAnnounceUsingCMPCTBLOCK;
             }
             if (!State(pfrom.GetId())->fSupportsDesiredCmpctVersion) {
-                if (pfrom.GetLocalServices() & NODE_WITNESS)
+                State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
-                    State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 2);
-                else
-                    State(pfrom.GetId())->fSupportsDesiredCmpctVersion = (nCMPCTBLOCKVersion == 1);
             }
         }
         return;

src/validation.cpp

@@ -1645,13 +1645,8 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consens
         pindex->phashBlock == nullptr || // this is a new candidate block, eg from TestBlockValidity()
         *pindex->phashBlock != consensusparams.BIP16Exception) // this block isn't the historical exception
     {
-        flags |= SCRIPT_VERIFY_P2SH;
+        // Enforce WITNESS rules whenever P2SH is in effect
-    }
+        flags |= SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS;
-
-    // Enforce WITNESS rules whenever P2SH is in effect (and the segwit
-    // deployment is defined).
-    if (flags & SCRIPT_VERIFY_P2SH && DeploymentEnabled(consensusparams, Consensus::DEPLOYMENT_SEGWIT)) {
-        flags |= SCRIPT_VERIFY_WITNESS;
     }
 
     // Enforce the DERSIG (BIP66) rule
@@ -3101,25 +3096,23 @@ std::vector<unsigned char> GenerateCoinbaseCommitment(CBlock& block, const CBloc
     std::vector<unsigned char> commitment;
     int commitpos = GetWitnessCommitmentIndex(block);
     std::vector<unsigned char> ret(32, 0x00);
-    if (DeploymentEnabled(consensusParams, Consensus::DEPLOYMENT_SEGWIT)) {
+    if (commitpos == NO_WITNESS_COMMITMENT) {
-        if (commitpos == NO_WITNESS_COMMITMENT) {
+        uint256 witnessroot = BlockWitnessMerkleRoot(block, nullptr);
-            uint256 witnessroot = BlockWitnessMerkleRoot(block, nullptr);
+        CHash256().Write(witnessroot).Write(ret).Finalize(witnessroot);
-            CHash256().Write(witnessroot).Write(ret).Finalize(witnessroot);
+        CTxOut out;
-            CTxOut out;
+        out.nValue = 0;
-            out.nValue = 0;
+        out.scriptPubKey.resize(MINIMUM_WITNESS_COMMITMENT);
-            out.scriptPubKey.resize(MINIMUM_WITNESS_COMMITMENT);
+        out.scriptPubKey[0] = OP_RETURN;
-            out.scriptPubKey[0] = OP_RETURN;
+        out.scriptPubKey[1] = 0x24;
-            out.scriptPubKey[1] = 0x24;
+        out.scriptPubKey[2] = 0xaa;
-            out.scriptPubKey[2] = 0xaa;
+        out.scriptPubKey[3] = 0x21;
-            out.scriptPubKey[3] = 0x21;
+        out.scriptPubKey[4] = 0xa9;
-            out.scriptPubKey[4] = 0xa9;
+        out.scriptPubKey[5] = 0xed;
-            out.scriptPubKey[5] = 0xed;
+        memcpy(&out.scriptPubKey[6], witnessroot.begin(), 32);
-            memcpy(&out.scriptPubKey[6], witnessroot.begin(), 32);
+        commitment = std::vector<unsigned char>(out.scriptPubKey.begin(), out.scriptPubKey.end());
-            commitment = std::vector<unsigned char>(out.scriptPubKey.begin(), out.scriptPubKey.end());
+        CMutableTransaction tx(*block.vtx[0]);
-            CMutableTransaction tx(*block.vtx[0]);
+        tx.vout.push_back(out);
-            tx.vout.push_back(out);
+        block.vtx[0] = MakeTransactionRef(std::move(tx));
-            block.vtx[0] = MakeTransactionRef(std::move(tx));
-        }
     }
     UpdateUncommittedBlockStructures(block, pindexPrev, consensusParams);
     return commitment;

test/functional/feature_presegwit_node_upgrade.py

@@ -0,0 +1,52 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test a pre-segwit node upgrading to segwit consensus"""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    softfork_active,
+)
+
+class SegwitUpgradeTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [["-segwitheight=10"]]
+
+    def run_test(self):
+        """A pre-segwit node with insufficiently validated blocks needs to redownload blocks"""
+
+        self.log.info("Testing upgrade behaviour for pre-segwit node to segwit rules")
+        node = self.nodes[0]
+
+        # Node hasn't been used or connected yet
+        assert_equal(node.getblockcount(), 0)
+
+        assert not softfork_active(node, "segwit")
+
+        # Generate 8 blocks without witness data
+        node.generate(8)
+        assert_equal(node.getblockcount(), 8)
+
+        self.stop_node(0)
+        # Restarting the node (with segwit activation height set to 5) should result in a shutdown
+        # because the blockchain consists of 3 insufficiently validated blocks per segwit consensus rules.
+        node.assert_start_raises_init_error(
+                extra_args=["-segwitheight=5"],
+                expected_msg=": Witness data for blocks after height 5 requires validation. Please restart with -reindex..\nPlease restart with -reindex or -reindex-chainstate to recover.")
+
+        # As directed, the user restarts the node with -reindex
+        self.start_node(0, extra_args=["-reindex", "-segwitheight=5"])
+
+        # With the segwit consensus rules, the node is able to validate only up to block 4
+        assert_equal(node.getblockcount(), 4)
+
+        # The upgraded node should now have segwit activated
+        assert softfork_active(node, "segwit")
+
+
+if __name__ == '__main__':
+    SegwitUpgradeTest().main()

test/functional/mining_basic.py

@@ -13,6 +13,7 @@ from decimal import Decimal
 
 from test_framework.blocktools import (
     create_coinbase,
+    get_witness_script,
     NORMAL_GBT_REQUEST_PARAMS,
     TIME_GENESIS_BLOCK,
 )
@@ -20,6 +21,7 @@ from test_framework.messages import (
     CBlock,
     CBlockHeader,
     BLOCK_HEADER_SIZE,
+    ser_uint256,
 )
 from test_framework.p2p import P2PDataStore
 from test_framework.test_framework import BitcoinTestFramework
@@ -49,6 +51,9 @@ class MiningTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         self.supports_cli = False
 
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
     def mine_chain(self):
         self.log.info('Create some old blocks')
         for t in range(TIME_GENESIS_BLOCK, TIME_GENESIS_BLOCK + 200 * 600, 600):
@@ -89,7 +94,21 @@ class MiningTest(BitcoinTestFramework):
         assert_equal(mining_info['networkhashps'], Decimal('0.003333333333333334'))
         assert_equal(mining_info['pooledtx'], 0)
 
-        # Mine a block to leave initial block download
+        self.log.info("getblocktemplate: Test default witness commitment")
+        txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16)
+        tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
+
+        # Check that default_witness_commitment is present.
+        assert 'default_witness_commitment' in tmpl
+        witness_commitment = tmpl['default_witness_commitment']
+
+        # Check that default_witness_commitment is correct.
+        witness_root = CBlock.get_merkle_root([ser_uint256(0),
+                                               ser_uint256(txid)])
+        script = get_witness_script(witness_root, 0)
+        assert_equal(witness_commitment, script.hex())
+
+        # Mine a block to leave initial block download and clear the mempool
         node.generatetoaddress(1, node.get_deterministic_priv_key().address)
         tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
         self.log.info("getblocktemplate: Test capability advertised")

test/functional/p2p_segwit.py

@@ -9,11 +9,10 @@ import random
 import struct
 import time
 
-from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment, get_witness_script, WITNESS_COMMITMENT_HEADER
+from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment, WITNESS_COMMITMENT_HEADER
 from test_framework.key import ECKey
 from test_framework.messages import (
     BIP125_SEQUENCE_NUMBER,
-    CBlock,
     CBlockHeader,
     CInv,
     COutPoint,
@@ -206,24 +205,17 @@ class TestP2PConn(P2PInterface):
 class SegWitTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        self.num_nodes = 3
+        self.num_nodes = 2
         # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
         self.extra_args = [
             ["-acceptnonstdtxn=1", "-segwitheight={}".format(SEGWIT_HEIGHT), "-whitelist=noban@127.0.0.1"],
             ["-acceptnonstdtxn=0", "-segwitheight={}".format(SEGWIT_HEIGHT)],
-            ["-acceptnonstdtxn=1", "-segwitheight=-1"],
         ]
         self.supports_cli = False
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
-    def setup_network(self):
-        self.setup_nodes()
-        self.connect_nodes(0, 1)
-        self.connect_nodes(0, 2)
-        self.sync_all()
-
     # Helper functions
 
     def build_next_block(self, version=4):
@@ -264,7 +256,6 @@ class SegWitTest(BitcoinTestFramework):
         self.test_non_witness_transaction()
         self.test_v0_outputs_arent_spendable()
         self.test_block_relay()
-        self.test_getblocktemplate_before_lockin()
         self.test_unnecessary_witness_before_segwit_activation()
         self.test_witness_tx_relay_before_segwit_activation()
         self.test_standardness_v0()
@@ -292,7 +283,6 @@ class SegWitTest(BitcoinTestFramework):
         self.test_signature_version_1()
         self.test_non_standard_witness_blinding()
         self.test_non_standard_witness()
-        self.test_upgrade_after_activation()
         self.test_witness_sigops()
         self.test_superfluous_witness()
         self.test_wtxid_relay()
@@ -482,11 +472,6 @@ class SegWitTest(BitcoinTestFramework):
         witness, and so can't be spent before segwit activation (the point at which
         blocks are permitted to contain witnesses)."""
 
-        # node2 doesn't need to be connected for this test.
-        # (If it's connected, node0 may propagate an invalid block to it over
-        # compact blocks and the nodes would have inconsistent tips.)
-        self.disconnect_nodes(0, 2)
-
         # Create two outputs, a p2wsh and p2sh-p2wsh
         witness_program = CScript([OP_TRUE])
         script_pubkey = script_to_p2wsh_script(witness_program)
@@ -544,37 +529,9 @@ class SegWitTest(BitcoinTestFramework):
             # TODO: support multiple acceptable reject reasons.
             test_witness_block(self.nodes[0], self.test_node, block, accepted=False, with_witness=False)
 
-        self.connect_nodes(0, 2)
-
         self.utxo.pop(0)
         self.utxo.append(UTXO(txid, 2, value))
 
-    @subtest  # type: ignore
-    def test_getblocktemplate_before_lockin(self):
-        txid = int(self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1), 16)
-
-        for node in [self.nodes[0], self.nodes[2]]:
-            gbt_results = node.getblocktemplate({"rules": ["segwit"]})
-            if node == self.nodes[2]:
-                # If this is a non-segwit node, we should not get a witness
-                # commitment.
-                assert 'default_witness_commitment' not in gbt_results
-            else:
-                # For segwit-aware nodes, check the witness
-                # commitment is correct.
-                assert 'default_witness_commitment' in gbt_results
-                witness_commitment = gbt_results['default_witness_commitment']
-
-                # Check that default_witness_commitment is present.
-                witness_root = CBlock.get_merkle_root([ser_uint256(0),
-                                                       ser_uint256(txid)])
-                script = get_witness_script(witness_root, 0)
-                assert_equal(witness_commitment, script.hex())
-
-        # Clear out the mempool
-        self.nodes[0].generate(1)
-        self.sync_blocks()
-
     @subtest  # type: ignore
     def test_witness_tx_relay_before_segwit_activation(self):
 
@@ -1927,39 +1884,6 @@ class SegWitTest(BitcoinTestFramework):
 
         self.utxo.pop(0)
 
-    @subtest  # type: ignore
-    def test_upgrade_after_activation(self):
-        """Test the behavior of starting up a segwit-aware node after the softfork has activated."""
-
-        # All nodes are caught up and node 2 is a pre-segwit node that will soon upgrade.
-        for n in range(2):
-            assert_equal(self.nodes[n].getblockcount(), self.nodes[2].getblockcount())
-            assert softfork_active(self.nodes[n], "segwit")
-        assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
-        assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']
-
-        # Restarting node 2 should result in a shutdown because the blockchain consists of
-        # insufficiently validated blocks per segwit consensus rules.
-        self.stop_node(2)
-        self.nodes[2].assert_start_raises_init_error(
-            extra_args=[f"-segwitheight={SEGWIT_HEIGHT}"],
-            expected_msg=f": Witness data for blocks after height {SEGWIT_HEIGHT} requires validation. Please restart with -reindex..\nPlease restart with -reindex or -reindex-chainstate to recover.",
-        )
-
-        # As directed, the user restarts the node with -reindex
-        self.start_node(2, extra_args=["-reindex", f"-segwitheight={SEGWIT_HEIGHT}"])
-
-        # With the segwit consensus rules, the node is able to validate only up to SEGWIT_HEIGHT - 1
-        assert_equal(self.nodes[2].getblockcount(), SEGWIT_HEIGHT - 1)
-        self.connect_nodes(0, 2)
-
-        # We reconnect more than 100 blocks, give it plenty of time
-        # sync_blocks() also verifies the best block hash is the same for all nodes
-        self.sync_blocks(timeout=240)
-
-        # The upgraded node should now have segwit activated
-        assert softfork_active(self.nodes[2], "segwit")
-
     @subtest  # type: ignore
     def test_witness_sigops(self):
         """Test sigop counting is correct inside witnesses."""

test/functional/test_runner.py

@@ -297,6 +297,7 @@ BASE_SCRIPTS = [
     'wallet_startup.py',
     'p2p_i2p_ports.py',
     'feature_config_args.py',
+    'feature_presegwit_node_upgrade.py',
     'feature_settings.py',
     'rpc_getdescriptorinfo.py',
     'rpc_addresses_deprecation.py',