Merge #2360: [Tests] Resurrect feature_block and kill the p2p compari…

…son test framework

48e5d54 [Tests][Refactoring] Remove 'magic bytes' in p2p_invalid_* tests (random-zebra)
fcda6cc [QA][Cleanup] Retire the Comparison Test Framework (random-zebra)
3633e42 [Tests] Fix and resurrect feature_block.py (random-zebra)
77645bf [BUG][Tests] Add on_getblocks to P2PDataStore interface (random-zebra)
030517b [Refactoring] pass next block height to IncrementExtraNonce (random-zebra)
0f0c97c [BUG] Fix block with invalid PoW in zerocoin_rejection_tests (random-zebra)
b19bee9 [Refactoring][BUG] Never change chain-params after setup in unit tests (random-zebra)
810a880 [Tests] Update PIVX specific constants bits/block-size/block-version (random-zebra)
d0777a7 [Refactoring] Add fPowNoRetargeting consensus param + fix regtest diff (random-zebra)
93fcc2e [Params] Fix regtest coinbase - mine it with lower diff (random-zebra)
4cf7222 [Tests] Refactor wallet_sapling_transactions_validations_tests (random-zebra)
44a1c44 [Refactor] Option to mine mempool txes in TestChainSetup::CreateBlock (random-zebra)
0b670c8 [BUG] Properly recompute coinbase and sapling root hash in CreateBlock (random-zebra)
f5f72ca [Trivial] Update some state logs (tx oversize and inputs missing/spent) (random-zebra)
9d552f9 [Refactor][Tests] Do not change chain when not needed (random-zebra)
bd6e591 [Refactoring] remove SaplingActive check in GUI/RPC (random-zebra)
b2410ae [BUG] Fix assertion in error in assert_debug_log (random-zebra)

Pull request description:

  Follow-up to #2346.

  The original goal here was to remove the `ComparisonTestFramework`, following bitcoin#11818.

  But, before doing that, had to update the old `feature_block` test, which was still using the framework, even though the test was disabled.
  This is one of the most important functional tests, as it checks all block-related consensus rules, so, just keeping it disabled was not an option.

  Aside from being very long (1200+ lines), the test was taking ages to mine any single block.
  This is because it relies on `next_block` doing the actual proof of work (`CBlock::solve`), given the regtest nBits (difficulty is fixed on regtest).

  Now, since all networks (mainnet/testnet/regtest) share the same genesis block, the regtest difficulty was way higher than it should have been. We never had issues with this, because we are bailing out early in `CheckProofOfWork`, essentially not performing any proof of work check on regtest.

  So, in order to fix `feature_block.py`, there were two options: either modify `next_block` to keep a fixed nonce, or update the regtest chain parameters, creating a new genesis block with low-diff nBits, and actually checking the proof of work.
  I chose the latter.

  This triggered the need to refactor some unit-tests that were changing chain params (from `MAIN` to `REGTEST`) without reloading the genesis block in the block index (which wasn't needed before, because, as explained, all networks had the same genesis).

  After this PR, the chain is set only inside the test fixtures, and is never changed inside the single test cases (so there is no need to reload the chain index during execution).

ACKs for top commit:
  furszy:
    ACK 48e5d54 .
  Fuzzbawls:
    ACK 48e5d54

Tree-SHA512: 213cad1be7eade941ec5be59fc0b8753eeb11aef55ae74052f59b6c668c4b8ab87efe77221545bdbb2d7b5beeb34b6123005ed2964def6673080cc809c508145

src/blockassembler.cpp

@@ -483,7 +483,18 @@ uint256 CalculateSaplingTreeRoot(CBlock* pblock, int nHeight, const CChainParams
     return UINT256_ZERO;
 }
 
-void IncrementExtraNonce(std::shared_ptr<CBlock>& pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce)
+bool SolveBlock(std::shared_ptr<CBlock>& pblock, int nHeight)
+{
+    unsigned int extraNonce = 0;
+    IncrementExtraNonce(pblock, nHeight, extraNonce);
+    while (pblock->nNonce < std::numeric_limits<uint32_t>::max() &&
+           !CheckProofOfWork(pblock->GetHash(), pblock->nBits)) {
+        ++pblock->nNonce;
+    }
+    return pblock->nNonce != std::numeric_limits<uint32_t>::max();
+}
+
+void IncrementExtraNonce(std::shared_ptr<CBlock>& pblock, int nHeight, unsigned int& nExtraNonce)
 {
     // Update nExtraNonce
     static uint256 hashPrevBlock;
@@ -492,7 +503,6 @@ void IncrementExtraNonce(std::shared_ptr<CBlock>& pblock, const CBlockIndex* pin
         hashPrevBlock = pblock->hashPrevBlock;
     }
     ++nExtraNonce;
-    unsigned int nHeight = pindexPrev->nHeight + 1; // Height first in coinbase required for block.version=2
     CMutableTransaction txCoinbase(*pblock->vtx[0]);
     txCoinbase.vin[0].scriptSig = (CScript() << nHeight << CScriptNum(nExtraNonce)) + COINBASE_FLAGS;
     assert(txCoinbase.vin[0].scriptSig.size() <= 100);

src/blockassembler.h

@@ -92,8 +92,9 @@ class BlockAssembler
     bool isStillDependent(CTxMemPool::txiter iter);
 };
 
-/** Modify the extranonce in a block */
-void IncrementExtraNonce(std::shared_ptr<CBlock>& pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
+/** Modify the nonce/extranonce in a block */
+bool SolveBlock(std::shared_ptr<CBlock>& pblock, int nHeight);
+void IncrementExtraNonce(std::shared_ptr<CBlock>& pblock, int nHeight, unsigned int& nExtraNonce);
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 int32_t ComputeBlockVersion(const Consensus::Params& consensusParams, int nHeight);
 

src/chainparams.cpp

@@ -134,6 +134,7 @@ class CMainParams : public CChainParams
         assert(genesis.hashMerkleRoot == uint256S("0x1b2ef6e2f28be914103a277377ae7729dcd125dfeb8bf97bd5964ba72b6dc39b"));
 
         consensus.fPowAllowMinDifficultyBlocks = false;
+        consensus.fPowNoRetargeting = false;
         consensus.powLimit   = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV1 = uint256S("0x000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV2 = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
@@ -275,6 +276,7 @@ class CTestNetParams : public CChainParams
         assert(genesis.hashMerkleRoot == uint256S("0x1b2ef6e2f28be914103a277377ae7729dcd125dfeb8bf97bd5964ba72b6dc39b"));
 
         consensus.fPowAllowMinDifficultyBlocks = true;
+        consensus.fPowNoRetargeting = false;
         consensus.powLimit   = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV1 = uint256S("0x000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV2 = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
@@ -393,13 +395,14 @@ class CRegTestParams : public CChainParams
     {
         strNetworkID = "regtest";
 
-        genesis = CreateGenesisBlock(1454124731, 2402015, 0x1e0ffff0, 1, 250 * COIN);
+        genesis = CreateGenesisBlock(1454124731, 1, 0x207fffff, 1, 250 * COIN);
         consensus.hashGenesisBlock = genesis.GetHash();
-        assert(consensus.hashGenesisBlock == uint256S("0x0000041e482b9b9691d98eefb48473405c0b8ec31b76df3797c74a78680ef818"));
+        assert(consensus.hashGenesisBlock == uint256S("0x7445589c4c8e52b105247b13373e5ee325856aa05d53f429e59ea46b7149ae3f"));
         assert(genesis.hashMerkleRoot == uint256S("0x1b2ef6e2f28be914103a277377ae7729dcd125dfeb8bf97bd5964ba72b6dc39b"));
 
         consensus.fPowAllowMinDifficultyBlocks = true;
-        consensus.powLimit   = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
+        consensus.fPowNoRetargeting = true;
+        consensus.powLimit   = uint256S("0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV1 = uint256S("0x000000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.posLimitV2 = uint256S("0x00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
         consensus.nBudgetCycleBlocks = 144;         // approx 10 cycles per day

src/consensus/params.h

@@ -88,6 +88,7 @@ struct NetworkUpgrade {
 struct Params {
     uint256 hashGenesisBlock;
     bool fPowAllowMinDifficultyBlocks;
+    bool fPowNoRetargeting;
     uint256 powLimit;
     uint256 posLimitV1;
     uint256 posLimitV2;

src/consensus/tx_verify.cpp

@@ -74,7 +74,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fCol
     static_assert(MAX_TX_SIZE_AFTER_SAPLING > MAX_ZEROCOIN_TX_SIZE, "New max TX size must be bigger than old max TX size");  // sanity
     const unsigned int nMaxSize = tx.IsShieldedTx() ? MAX_TX_SIZE_AFTER_SAPLING : MAX_ZEROCOIN_TX_SIZE;
     if (tx.GetTotalSize() > nMaxSize) {
-        return state.DoS(10, false, REJECT_INVALID, "bad-txns-oversize");
+        return state.DoS(10, error("tx oversize: %d > %d", tx.GetTotalSize(), nMaxSize), REJECT_INVALID, "bad-txns-oversize");
     }
 
     // Dispatch to Sapling validator

src/miner.cpp

@@ -188,7 +188,7 @@ void BitcoinMiner(CWallet* pwallet, bool fProofOfStake)
         }
 
         // POW - miner main
-        IncrementExtraNonce(pblock, pindexPrev, nExtraNonce);
+        IncrementExtraNonce(pblock, pindexPrev->nHeight + 1, nExtraNonce);
 
         LogPrintf("Running PIVXMiner with %u transactions in block (%u bytes)\n", pblock->vtx.size(),
             ::GetSerializeSize(*pblock, SER_NETWORK, PROTOCOL_VERSION));

src/pow.cpp

@@ -18,7 +18,9 @@
 
 unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader* pblock)
 {
-    if (Params().IsRegTestNet())
+    const Consensus::Params& consensus = Params().GetConsensus();
+
+    if (consensus.fPowNoRetargeting)
         return pindexLast->nBits;
 
     /* current difficulty formula, pivx - DarkGravity v3, written by Evan Duffield - evan@dashpay.io */
@@ -31,7 +33,6 @@ unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHead
     int64_t CountBlocks = 0;
     arith_uint256 PastDifficultyAverage;
     arith_uint256 PastDifficultyAveragePrev;
-    const Consensus::Params& consensus = Params().GetConsensus();
     const arith_uint256& powLimit = UintToArith256(consensus.powLimit);
 
     if (BlockLastSolved == NULL || BlockLastSolved->nHeight == 0 || BlockLastSolved->nHeight < PastBlocksMin) {
@@ -124,8 +125,6 @@ bool CheckProofOfWork(uint256 hash, unsigned int nBits)
     bool fOverflow;
     arith_uint256 bnTarget;
 
-    if (Params().IsRegTestNet()) return true;
-
     bnTarget.SetCompact(nBits, &fNegative, &fOverflow);
 
     // Check range

src/qt/pivx/send.cpp

@@ -415,11 +415,6 @@ void SendWidget::ProcessSend(QList<SendCoinsRecipient>& recipients, bool hasShie
     // First check SPORK_20 (before unlock)
     bool isShieldedTx = hasShieldedOutput || !isTransparent;
     if (isShieldedTx) {
-        if (!walletModel->isSaplingEnforced()) {
-            inform(tr("Cannot perform shielded operations, v5 upgrade isn't being enforced yet!"));
-            return;
-        }
-
         if (walletModel->isSaplingInMaintenance()) {
             inform(tr("Sapling Protocol temporarily in maintenance. Shielded transactions disabled (SPORK 20)"));
             return;
@@ -702,8 +697,8 @@ void SendWidget::onCoinControlClicked()
 
 void SendWidget::onShieldCoinsClicked()
 {
-    if (!walletModel->isSaplingEnforced()) {
-        inform(tr("Cannot perform shielded operations, v5 upgrade isn't being enforced yet!"));
+    if (walletModel->isSaplingInMaintenance()) {
+        inform(tr("Sapling Protocol temporarily in maintenance. Shielded transactions disabled (SPORK 20)"));
         return;
     }
 
@@ -792,13 +787,6 @@ void SendWidget::onCheckBoxChanged()
 void SendWidget::onPIVSelected(bool _isTransparent)
 {
     isTransparent = _isTransparent;
-
-    if (!isTransparent && !walletModel->isSaplingEnforced()) {
-        ui->pushLeft->setChecked(true);
-        inform(tr("Cannot perform shielded operations, v5 upgrade isn't being enforced yet!"));
-        return;
-    }
-
     resetChangeAddress();
     resetCoinControl();
     tryRefreshAmounts();

src/qt/walletmodel.cpp

@@ -85,11 +85,6 @@ bool WalletModel::isSaplingInMaintenance() const
     return sporkManager.IsSporkActive(SPORK_20_SAPLING_MAINTENANCE);
 }
 
-bool WalletModel::isSaplingEnforced() const
-{
-    return Params().GetConsensus().NetworkUpgradeActive(cachedNumBlocks, Consensus::UPGRADE_V5_0);
-}
-
 bool WalletModel::isV6Enforced() const
 {
     return Params().GetConsensus().NetworkUpgradeActive(cachedNumBlocks, Consensus::UPGRADE_V6_0);
@@ -603,14 +598,6 @@ OperationResult WalletModel::PrepareShieldedTransaction(WalletModelTransaction*
                                                         bool fromTransparent,
                                                         const CCoinControl* coinControl)
 {
-    // Basic checks first
-
-    // Check network status
-    int nextBlockHeight = cachedNumBlocks + 1;
-    if (!Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_V5_0)) {
-        return errorOut("Error, cannot send transaction. Sapling is not activated");
-    }
-
     // Load shieldedAddrRecipients.
     std::vector<SendManyRecipient> recipients;
     for (const auto& recipient : modelTransaction->getRecipients()) {
@@ -630,6 +617,7 @@ OperationResult WalletModel::PrepareShieldedTransaction(WalletModelTransaction*
     if (!opResult) return opResult;
 
     // Create the operation
+    int nextBlockHeight = cachedNumBlocks + 1;
     SaplingOperation operation(Params().GetConsensus(), nextBlockHeight, wallet);
     auto operationResult = operation.setRecipients(recipients)
              ->setTransparentKeyChange(modelTransaction->getPossibleKeyChange())

src/qt/walletmodel.h

@@ -155,7 +155,6 @@ class WalletModel : public QObject
     /** Whether cold staking is enabled or disabled in the network **/
     bool isColdStakingNetworkelyEnabled() const;
     bool isSaplingInMaintenance() const;
-    bool isSaplingEnforced() const;
     bool isV6Enforced() const;
     CAmount getMinColdStakingAmount() const;
     /* current staking status from the miner thread **/

src/rpc/mining.cpp

@@ -35,7 +35,6 @@ UniValue generateBlocks(const Consensus::Params& consensus,
                         CScript* coinbaseScript)
 {
     UniValue blockHashes(UniValue::VARR);
-    unsigned int nExtraNonce = 0;
 
     while (nHeight < nHeightEnd && !ShutdownRequested()) {
 
@@ -52,16 +51,8 @@ UniValue generateBlocks(const Consensus::Params& consensus,
         std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>(pblocktemplate->block);
 
         if(!fPoS) {
-            {
-                LOCK(cs_main);
-                IncrementExtraNonce(pblock, chainActive.Tip(), nExtraNonce);
-            }
-            while (pblock->nNonce < std::numeric_limits<uint32_t>::max() &&
-                   !CheckProofOfWork(pblock->GetHash(), pblock->nBits)) {
-                ++pblock->nNonce;
-            }
             if (ShutdownRequested()) break;
-            if (pblock->nNonce == std::numeric_limits<uint32_t>::max()) continue;
+            if (!SolveBlock(pblock, nHeight + 1)) continue;
         }
 
         CValidationState state;

src/sapling/transaction_builder.cpp

@@ -129,15 +129,6 @@ std::string TransactionBuilderResult::GetError() {
     }
 }
 
-// Set default values of new CMutableTransaction based on consensus rules at given height.
-CMutableTransaction CreateNewContextualCMutableTransaction(const Consensus::Params& consensusParams, int nHeight)
-{
-    CMutableTransaction mtx;
-    bool isSapling = consensusParams.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_V5_0);
-    mtx.nVersion = isSapling ? CTransaction::TxVersion::SAPLING : CTransaction::TxVersion::LEGACY;
-    return mtx;
-}
-
 TransactionBuilder::TransactionBuilder(
     const Consensus::Params& _consensusParams,
     int _nHeight,
@@ -151,7 +142,8 @@ TransactionBuilder::TransactionBuilder(
 
 void TransactionBuilder::Clear()
 {
-    mtx = CreateNewContextualCMutableTransaction(consensusParams, nHeight);
+    mtx = CMutableTransaction();
+    mtx.nVersion = CTransaction::TxVersion::SAPLING;
     spends.clear();
     outputs.clear();
     tIns.clear();