Merge bitcoin/bitcoin#24421: miner: always assume we can build witness blocks

40e871d [miner] always assume we can create witness blocks (glozow)

Pull request description:

  Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there.

ACKs for top commit:
  gruve-p:
    ACK bitcoin/bitcoin@40e871d
  jnewbery:
    utACK 40e871d, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: bitcoin/bitcoin#24421 (comment).
  achow101:
    ACK 40e871d
  theStack:
    Code-review ACK 40e871d

Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019

Author: fanquake

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)