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,136 @@
+#!/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.expect_mempool_restricted ("txid")
+
+        # 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 ("txid")
+        set_node_times (self.nodes, ACTIVATION_TIME + NO_RESTRICTIONS)
+        self.expect_unrestricted ()
+
+if __name__ == '__main__':
+    AroundSegwitLightTest ().main ()

divi/qa/rpc-tests/test_runner.py

@@ -80,6 +80,7 @@
     'StakingVaultStaking.py',
     'StakingVaultDeactivation.py',
     'StakingVaultSpam.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 <Settings.h>
@@ -54,3 +56,10 @@ bool ActivationState::IsActive(const Fork f) const
   assert(mit != ACTIVATION_TIMES.end());
   return nTime >= 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

@@ -63,6 +63,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

@@ -113,6 +113,14 @@ bool fSpentIndex = false;
 bool fCheckBlockIndex = false;
 bool fVerifyingBlocks = false;
 unsigned int nCoinCacheSize = 5000;
+
+/** 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);
@@ -594,7 +602,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;
@@ -607,6 +615,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 == 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

@@ -14,6 +14,7 @@
 #include <chain.h>
 #include "coincontrol.h"
 #include <chainparams.h>
+#include "ForkActivation.h"
 #include "masternode-payments.h"
 #include "net.h"
 #include "script/script.h"
@@ -58,6 +59,14 @@ using namespace std;
 CAmount nTransactionValueMultiplier = 10000; // 1 / 0.0001 = 10000;
 unsigned int nTransactionSizeMultiplier = 300;
 static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000;
+
+/** 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;
+
 int64_t nStartupTime = GetAdjustedTime();
 
 /** @defgroup mapWallet
@@ -2023,6 +2032,12 @@ void CWallet::UpdateNextTransactionIndexAvailable(int64_t transactionIndex)
     orderedTransactionIndex = transactionIndex;
 }
 
+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)
@@ -2834,7 +2849,7 @@ bool CWallet::IsTrusted(const CWalletTx& walletTransaction) const
         return true;
     if (nDepth < 0)
         return false;
-    if (!allowSpendingZeroConfirmationOutputs || !DebitsFunds(walletTransaction, ISMINE_ALL)) // using wtx's cached debit
+    if (!AllowSpendingZeroConfirmationChange() || !DebitsFunds(walletTransaction, ISMINE_ALL)) // using wtx's cached debit
         return false;
 
     // Trusted if all inputs are from us and are in the mempool:

divi/src/wallet.h

@@ -166,6 +166,11 @@ class CWallet : public CCryptoKeyStore, public NotificationInterface, public I_K
 private:
     void DeriveNewChildKey(const CKeyMetadata& metadata, CKey& secretRet, uint32_t nAccountIndex, bool fInternal /*= false*/);
 
+    /** 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;
+
 public:
     bool MoveFundsBetweenAccounts(std::string from, std::string to, CAmount amount, std::string comment);