Restrict unconfirmed change around fork.

Around the segwit-light fork (some hours before and after), we should
avoid spending unconfirmed outputs:  If we do that, it is unclear
whether or not the fork will be active in the block that confirms
those spends, and thus we can't reliably construct a valid chain.

With this change, we introduce a (temporary) measure to disallow
those spends in the wallet and mempool policy in a window of time around
the planned fork activation.

Author: domob1812

divi/qa/rpc-tests/around_segwit_light.py

@@ -0,0 +1,137 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The DIVI developers
+# Distributed under the MIT/X11 software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+# Tests the policy changes in wallet and mempool around the
+# segwit-light activation time that prevent spending of
+# unconfirmed outputs.
+
+from test_framework import BitcoinTestFramework
+from authproxy import JSONRPCException
+from util import *
+
+ACTIVATION_TIME = 2_100_000_000
+
+# Offset around the activation time where no restrictions are in place.
+NO_RESTRICTIONS = 24 * 3_600
+
+# Offset around the activation time where the wallet disallows
+# spending unconfirmed change, but the mempool still accepts it.
+WALLET_RESTRICTED = 10 * 3_600
+
+# Offset around the activation time where wallet and mempool
+# disallow spending unconfirmed outputs.
+MEMPOOL_RESTRICTED = 3_600
+
+
+class AroundSegwitLightTest (BitcoinTestFramework):
+
+    def setup_network (self, split=False):
+        args = ["-debug", "-spendzeroconfchange"]
+        self.nodes = start_nodes (1, self.options.tmpdir, extra_args=[args])
+        self.node = self.nodes[0]
+        self.is_network_split = False
+
+    def build_spending_chain (self, key, addr, initialValue):
+        """
+        Spends the initialValue to addr, and then builds a follow-up
+        transaction that spends that output again back to addr with
+        a smaller value (for fees).  The second transaction is built
+        using the raw-transactions API and returned as hex, not
+        submitted to the node already.
+        """
+
+        txid = self.node.sendtoaddress (addr, initialValue)
+        data = self.node.getrawtransaction (txid, 1)
+        outputIndex = None
+        for i in range (len (data["vout"])):
+          if data["vout"][i]["value"] == initialValue:
+            outputIndex = i
+            break
+        assert outputIndex is not None
+
+        inputs = [{"txid": data[key], "vout": outputIndex}]
+        outputs = {addr: initialValue - 10}
+        tx = self.node.createrawtransaction (inputs, outputs)
+        signed = self.node.signrawtransaction (tx)
+        assert signed["complete"]
+
+        return signed["hex"]
+
+    def expect_unrestricted (self):
+        """
+        Checks that spending of unconfirmed change is possible without
+        restrictions of wallet or mempool.
+        """
+
+        balance = self.node.getbalance ()
+        addr = self.node.getnewaddress ()
+
+        self.node.sendtoaddress (addr, balance - 10)
+        self.node.sendtoaddress (addr, balance - 20)
+        self.node.setgenerate (True, 1)
+
+    def expect_wallet_restricted (self, key):
+        """
+        Checks that the wallet forbids spending unconfirmed change,
+        while the mempool still allows it.
+        """
+
+        balance = self.node.getbalance ()
+        addr = self.node.getnewaddress ()
+
+        self.node.sendtoaddress (addr, balance - 10)
+        assert_raises (JSONRPCException, self.node.sendtoaddress, addr, balance - 20)
+        self.node.setgenerate (True, 1)
+
+        balance = self.node.getbalance ()
+        tx = self.build_spending_chain (key, addr, balance - 10)
+        self.node.sendrawtransaction (tx)
+        self.node.setgenerate (True, 1)
+
+    def expect_mempool_restricted (self, key):
+        """
+        Checks that the mempool does not allow spending unconfirmed
+        outputs (even if the transaction is built and submitted directly),
+        while blocks should still allow it.
+        """
+
+        balance = self.node.getbalance ()
+        addr = self.node.getnewaddress ()
+
+        tx = self.build_spending_chain (key, addr, balance - 10)
+        assert_raises (JSONRPCException, self.node.sendrawtransaction, tx)
+        self.node.generateblock ({"extratx": [tx]})
+
+    def run_test (self):
+        self.node.setgenerate (True, 30)
+
+        # Before restrictions come into force, doing a normal
+        # spend of unconfirmed change through the wallet is fine.
+        set_node_times (self.nodes, ACTIVATION_TIME - NO_RESTRICTIONS)
+        self.expect_unrestricted ()
+
+        # Next the wallet doesn't allow those spends, but the mempool
+        # will (if done directly).
+        set_node_times (self.nodes, ACTIVATION_TIME - WALLET_RESTRICTED)
+        self.expect_wallet_restricted ("txid")
+
+        # Very close to the fork (on both sides), even the mempool won't
+        # allow spending unconfirmed change.  If we include it directly in
+        # a block, it works.
+        set_node_times (self.nodes, ACTIVATION_TIME - MEMPOOL_RESTRICTED)
+        self.expect_mempool_restricted ("txid")
+        set_node_times (self.nodes, ACTIVATION_TIME + MEMPOOL_RESTRICTED)
+        self.node.setgenerate (True, 1)
+        self.expect_mempool_restricted ("baretxid")
+
+        # Finally, we should run into mempool-only or no restrictions
+        # at all if we go further into the future, away from the fork.
+        set_node_times (self.nodes, ACTIVATION_TIME + WALLET_RESTRICTED)
+        self.expect_wallet_restricted ("baretxid")
+        set_node_times (self.nodes, ACTIVATION_TIME + NO_RESTRICTIONS)
+        self.expect_unrestricted ()
+
+if __name__ == '__main__':
+    AroundSegwitLightTest ().main ()

divi/qa/rpc-tests/test_runner.py

@@ -83,6 +83,7 @@
     'StakingVaultDeactivation.py',
     'StakingVaultSpam.py',
     'StakingVaultRedundancy.py',
+    'around_segwit_light.py',
     'forknotify.py',
     'getchaintips.py',
     'httpbasics.py',

divi/src/ForkActivation.cpp

@@ -6,7 +6,9 @@
 
 #include "chain.h"
 #include "primitives/block.h"
+#include "timedata.h"
 
+#include <cmath>
 #include <unordered_map>
 #include <unordered_set>
 
@@ -59,3 +61,10 @@ bool ActivationState::IsActive(const Fork f) const
   assert(mit != ACTIVATION_TIMES.end());
   return currentTime >= mit->second;
 }
+
+bool ActivationState::CloseToSegwitLight(const int maxSeconds)
+{
+  const int64_t now = GetAdjustedTime();
+  const int64_t activation = ACTIVATION_TIMES.at(Fork::SegwitLight);
+  return std::abs(now - activation) <= maxSeconds;
+}

divi/src/ForkActivation.h

@@ -62,6 +62,15 @@ class ActivationState
    */
   bool IsActive(Fork f) const;
 
+  /**
+   * Returns true if the current time is "close" to the activation of
+   * segwit-light (before or after), with close being within the given
+   * number of seconds.  This is used for a temporary measure to disallow
+   * (by mempool and wallet policy) spending of unconfirmed change
+   * around the fork.
+   */
+  static bool CloseToSegwitLight(int maxSeconds);
+
 };
 
 #endif // FORK_ACTIVATION_H

divi/src/main.cpp

@@ -66,6 +66,7 @@
 #include <TransactionOpCounting.h>
 #include <OrphanTransactions.h>
 #include <MasternodeModule.h>
+#include <MemPoolEntry.h>
 #include <IndexDatabaseUpdates.h>
 #include <BlockTransactionChecker.h>
 #include <NodeState.h>
@@ -114,6 +115,13 @@ extern bool fAddressIndex;
 extern bool fSpentIndex;
 extern CTxMemPool mempool;
 
+/** Number of seconds around the "segwit light" fork for which we disallow
+ *  spending unconfirmed outputs (to avoid messing up with the change
+ *  itself).  This should be smaller than the matching constant for the
+ *  wallet (so the wallet does not construct things the mempool won't
+ *  accept in the end).  */
+constexpr int SEGWIT_LIGHT_FORBID_SPENDING_ZERO_CONF_SECONDS = 14400;
+
 bool IsFinalTx(const CTransaction& tx, const CChain& activeChain, int nBlockHeight = 0 , int64_t nBlockTime = 0);
 
 CCheckpointServices checkpointsVerifier(GetCurrentChainCheckpoints);
@@ -620,7 +628,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransa
             // do all inputs exist?
             // Note that this does not check for the presence of actual outputs (see the next check for that),
             // only helps filling in pfMissingInputs (to determine missing vs spent).
-            for (const CTxIn txin : tx.vin) {
+            for (const auto& txin : tx.vin) {
                 if (!view.HaveCoins(txin.prevout.hash)) {
                     if (pfMissingInputs)
                         *pfMissingInputs = true;
@@ -634,6 +642,21 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransa
                 return state.Invalid(error("%s : inputs already spent",__func__),
                                      REJECT_DUPLICATE, "bad-txns-inputs-spent");
 
+            // If we are close to the segwit-light activation (before or after),
+            // do not allow spending of zero-confirmation outputs.  This would
+            // mess up with the fork, as it is not clear whether the fork will be
+            // active or not when they get confirmed (and thus it is not possible
+            // to reliably construct a chain of transactions).
+            if (ActivationState::CloseToSegwitLight(SEGWIT_LIGHT_FORBID_SPENDING_ZERO_CONF_SECONDS)) {
+                for (const auto& txin : tx.vin) {
+                    const auto* coins = view.AccessCoins(txin.prevout.hash);
+                    assert(coins != nullptr && coins->IsAvailable(txin.prevout.n));
+                    if (coins->nHeight == CTxMemPoolEntry::MEMPOOL_HEIGHT)
+                        return state.DoS(0, error("%s : spending zero-confirmation output around segwit light", __func__),
+                                         REJECT_NONSTANDARD, "zero-conf-segwit-light");
+                }
+            }
+
             // Bring the best block into scope
             view.GetBestBlock();
 

divi/src/wallet.cpp

@@ -45,6 +45,13 @@ extern Settings& settings;
 
 const FeeAndPriorityCalculator& priorityFeeCalculator = FeeAndPriorityCalculator::instance();
 
+/** Number of seconds around the "segwit light" fork for which we force
+ *  not spending unconfirmed change (to avoid messing up with the change
+ *  itself).  This should be larger than the matching constant for the
+ *  mempool (so the wallet does not construct things the mempool won't
+ *  accept in the end).  */
+constexpr int SEGWIT_LIGHT_DISABLE_SPENDING_ZERO_CONF_SECONDS = 43200;
+
 extern CCriticalSection cs_main;
 extern CTxMemPool mempool;
 
@@ -2045,6 +2052,12 @@ bool CWallet::SelectCoinsMinConf(
     return true;
 }
 
+bool CWallet::AllowSpendingZeroConfirmationChange() const
+{
+    return allowSpendingZeroConfirmationOutputs
+        && !ActivationState::CloseToSegwitLight(SEGWIT_LIGHT_DISABLE_SPENDING_ZERO_CONF_SECONDS);
+}
+
 void AppendOutputs(
     const std::vector<std::pair<CScript, CAmount> >& intendedDestinations,
     CMutableTransaction& txNew)
@@ -2813,7 +2826,7 @@ bool CWallet::IsTrusted(const CWalletTx& walletTransaction) const
         return true;
     if (nDepth < 0)
         return false;
-    if (!allowSpendingZeroConfirmationOutputs || !DebitsFunds(walletTransaction, isminetype::ISMINE_SPENDABLE)) // using wtx's cached debit
+    if (!AllowSpendingZeroConfirmationChange() || !DebitsFunds(walletTransaction, isminetype::ISMINE_SPENDABLE)) // using wtx's cached debit
         return false;
 
     // Trusted if all inputs are from us and are in the mempool:

divi/src/wallet.h

@@ -200,6 +200,11 @@ class CWallet :
     isminetype IsMine(const CTxIn& txin) const;
     bool IsMine(const CTransaction& tx) const;
 
+    /** Returns true if the wallet should allow spending of unconfirmed change.
+     *  This is mostly determined by allowSpendingZeroConfirmationOutputs,
+     *  but is forced to off around the segwit-light fork.  */
+    bool AllowSpendingZeroConfirmationChange() const;
+
 protected:
     // CWalletDB: load from disk methods
     void LoadWalletTransaction(const CWalletTx& wtxIn) override;