[miner] always assume we can create witness blocks

Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.

Author: glozow

src/node/miner.cpp

@@ -97,7 +97,6 @@ void BlockAssembler::resetBlock()
     // Reserve space for coinbase tx
     nBlockWeight = 4000;
     nBlockSigOpsCost = 400;
-    fIncludeWitness = false;
 
     // These counters do not include coinbase tx
     nBlockTx = 0;
@@ -137,17 +136,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     pblock->nTime = GetAdjustedTime();
     m_lock_time_cutoff = pindexPrev->GetMedianTimePast();
 
-    // Decide whether to include witness transactions
-    // This is only needed in case the witness softfork activation is reverted
-    // (which would require a very deep reorganization).
-    // Note that the mempool would accept transactions with witness data before
-    // the deployment is active, but we would only ever mine blocks after activation
-    // unless there is a massive block reorganization with the witness softfork
-    // not activated.
-    // TODO: replace this with a call to main to assess validity of a mempool
-    // transaction (which in most cases can be a no-op).
-    fIncludeWitness = DeploymentActiveAfter(pindexPrev, chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT);
-
     int nPackagesSelected = 0;
     int nDescendantsUpdated = 0;
     addPackageTxs(nPackagesSelected, nDescendantsUpdated);
@@ -215,17 +203,12 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost
 
 // Perform transaction-level checks before adding to block:
 // - transaction finality (locktime)
-// - premature witness (in case segwit transactions are added to mempool before
-//   segwit activation)
 bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package) const
 {
     for (CTxMemPool::txiter it : package) {
         if (!IsFinalTx(it->GetTx(), nHeight, m_lock_time_cutoff)) {
             return false;
         }
-        if (!fIncludeWitness && it->GetTx().HasWitness()) {
-            return false;
-        }
     }
     return true;
 }

src/node/miner.h

@@ -132,7 +132,6 @@ class BlockAssembler
     std::unique_ptr<CBlockTemplate> pblocktemplate;
 
     // Configuration parameters for the block size
-    bool fIncludeWitness;
     unsigned int nBlockMaxWeight;
     CFeeRate blockMinFeeRate;
 

test/functional/feature_segwit.py

@@ -117,11 +117,9 @@ def success_mine(self, node, txid, sign, redeem_script=""):
         assert_equal(len(node.getblock(block[0])["tx"]), 2)
         self.sync_blocks()
 
-    def skip_mine(self, node, txid, sign, redeem_script=""):
+    def fail_mine(self, node, txid, sign, redeem_script=""):
         send_to_witness(1, node, getutxo(txid), self.pubkey[0], False, Decimal("49.998"), sign, redeem_script)
-        block = self.generate(node, 1)
-        assert_equal(len(node.getblock(block[0])["tx"]), 1)
-        self.sync_blocks()
+        assert_raises_rpc_error(-1, "unexpected witness data found", self.generate, node, 1)
 
     def fail_accept(self, node, error_msg, txid, sign, redeem_script=""):
         assert_raises_rpc_error(-26, error_msg, send_to_witness, use_p2wsh=1, node=node, utxo=getutxo(txid), pubkey=self.pubkey[0], encode_p2sh=False, amount=Decimal("49.998"), sign=sign, insert_redeem_script=redeem_script)
@@ -197,21 +195,21 @@ def run_test(self):
         assert_equal(self.nodes[1].getbalance(), 20 * Decimal("49.999"))
         assert_equal(self.nodes[2].getbalance(), 20 * Decimal("49.999"))
 
-        self.generate(self.nodes[0], 260)  # block 423
+        self.generate(self.nodes[0], 264)  # block 427
 
-        self.log.info("Verify witness txs are skipped for mining before the fork")
-        self.skip_mine(self.nodes[2], wit_ids[NODE_2][P2WPKH][0], True)  # block 424
-        self.skip_mine(self.nodes[2], wit_ids[NODE_2][P2WSH][0], True)  # block 425
-        self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WPKH][0], True)  # block 426
-        self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][P2WSH][0], True)  # block 427
+        self.log.info("Verify witness txs cannot be mined before the fork")
+        self.fail_mine(self.nodes[2], wit_ids[NODE_2][P2WPKH][0], True)
+        self.fail_mine(self.nodes[2], wit_ids[NODE_2][P2WSH][0], True)
+        self.fail_mine(self.nodes[2], p2sh_ids[NODE_2][P2WPKH][0], True)
+        self.fail_mine(self.nodes[2], p2sh_ids[NODE_2][P2WSH][0], True)
 
         self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid")
         self.fail_accept(self.nodes[2], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False)
         self.fail_accept(self.nodes[2], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False)
 
-        self.generate(self.nodes[2], 4)  # blocks 428-431
+        self.generate(self.nodes[0], 4)  # blocks 428-431
 
-        self.log.info("Verify previous witness txs skipped for mining can now be mined")
+        self.log.info("Verify previous witness txs can now be mined")
         assert_equal(len(self.nodes[2].getrawmempool()), 4)
         blockhash = self.generate(self.nodes[2], 1)[0]  # block 432 (first block with new rules; 432 = 144 * 3)
         assert_equal(len(self.nodes[2].getrawmempool()), 0)