Extend block merkle root by hash of account changes (#337)

Signed-off-by: Anthony Fieroni <[email protected]> Co-authored-by: monstrobishi <[email protected]>
DeFiCh · Apr 29, 2021 · dd89fc9 · dd89fc9
1 parent 0eac646
commit dd89fc9
Show file tree

Hide file tree

Showing 12 changed files with 152 additions and 34 deletions.
diff --git a/src/hash.h b/src/hash.h
@@ -89,6 +89,20 @@ inline uint256 Hash(const T1 p1begin, const T1 p1end,
     return result;
 }
 
+/** Compute the 256-bit hash of a container. */
+template<typename T>
+inline uint256 Hash(const T& c)
+{
+    return Hash(c.begin(), c.end());
+}
+
+/** Compute the 256-bit hash of two containers. */
+template<typename T>
+inline uint256 Hash2(const T& c1, const T& c2)
+{
+    return Hash(c1.begin(), c1.end(), c2.begin(), c2.end());
+}
+
 /** Compute the 160-bit hash an object. */
 template<typename T1>
 inline uint160 Hash160(const T1 pbegin, const T1 pend)

diff --git a/src/masternodes/accountshistory.cpp b/src/masternodes/accountshistory.cpp
@@ -33,7 +33,7 @@ CAccountHistoryStorage::CAccountHistoryStorage(const fs::path& dbName, std::size
 }
 
 CAccountsHistoryWriter::CAccountsHistoryWriter(CCustomCSView & storage, uint32_t height, uint32_t txn, const uint256& txid, uint8_t type, CAccountsHistoryView* historyView)
-    : CStorageView(new CFlushableStorageKV(storage.GetRaw())), height(height), txn(txn), txid(txid), type(type), historyView(historyView)
+    : CStorageView(new CFlushableStorageKV(static_cast<CStorageKV&>(storage.GetStorage()))), height(height), txn(txn), txid(txid), type(type), historyView(historyView)
 {
 }
 
@@ -66,7 +66,7 @@ bool CAccountsHistoryWriter::Flush()
 }
 
 CAccountsHistoryEraser::CAccountsHistoryEraser(CCustomCSView & storage, uint32_t height, uint32_t txn, CAccountsHistoryView* historyView)
-    : CStorageView(new CFlushableStorageKV(storage.GetRaw())), height(height), txn(txn), historyView(historyView)
+    : CStorageView(new CFlushableStorageKV(static_cast<CStorageKV&>(storage.GetStorage()))), height(height), txn(txn), historyView(historyView)
 {
 }
 

diff --git a/src/masternodes/masternodes.cpp b/src/masternodes/masternodes.cpp
@@ -8,6 +8,7 @@
 #include <masternodes/mn_checks.h>
 
 #include <chainparams.h>
+#include <consensus/merkle.h>
 #include <net_processing.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
@@ -717,12 +718,12 @@ void CCustomCSView::CreateAndRelayConfirmMessageIfNeed(const CAnchorIndex::Ancho
 
 void CCustomCSView::OnUndoTx(uint256 const & txid, uint32_t height)
 {
-    const auto undo = this->GetUndo(UndoKey{height, txid});
+    const auto undo = GetUndo(UndoKey{height, txid});
     if (!undo) {
         return; // not custom tx, or no changes done
     }
-    CUndo::Revert(this->GetRaw(), *undo); // revert the changes of this tx
-    this->DelUndo(UndoKey{height, txid}); // erase undo data, it served its purpose
+    CUndo::Revert(GetStorage(), *undo); // revert the changes of this tx
+    DelUndo(UndoKey{height, txid}); // erase undo data, it served its purpose
 }
 
 bool CCustomCSView::CanSpend(const uint256 & txId, int height) const
@@ -768,6 +769,19 @@ bool CCustomCSView::CalculateOwnerRewards(CScript const & owner, uint32_t target
     return UpdateBalancesHeight(owner, targetHeight);
 }
 
+uint256 CCustomCSView::MerkleRoot() {
+    auto& rawMap = GetStorage().GetRaw();
+    if (rawMap.empty()) {
+        return {};
+    }
+    std::vector<uint256> hashes;
+    for (const auto& it : rawMap) {
+        auto value = it.second ? *it.second : TBytes{};
+        hashes.push_back(Hash2(it.first, value));
+    }
+    return ComputeMerkleRoot(std::move(hashes));
+}
+
 std::map<CKeyID, CKey> AmISignerNow(CAnchorData::CTeam const & team)
 {
     AssertLockHeld(cs_main);

diff --git a/src/masternodes/masternodes.h b/src/masternodes/masternodes.h
@@ -281,8 +281,11 @@ class CCustomCSView
 
     int GetDbVersion() const;
 
-    CStorageKV& GetRaw() {
-        return DB();
+    uint256 MerkleRoot();
+
+    // we construct it as it
+    CFlushableStorageKV& GetStorage() {
+        return static_cast<CFlushableStorageKV&>(DB());
     }
 };
 

diff --git a/src/masternodes/mn_checks.cpp b/src/masternodes/mn_checks.cpp
@@ -1244,8 +1244,8 @@ Res ApplyCustomTx(CCustomCSView& mnview, const CCoinsViewCache& coins, const CTr
     }
 
     // construct undo
-    auto& flushable = dynamic_cast<CFlushableStorageKV&>(view.GetRaw());
-    auto undo = CUndo::Construct(mnview.GetRaw(), flushable.GetRaw());
+    auto& flushable = view.GetStorage();
+    auto undo = CUndo::Construct(mnview.GetStorage(), flushable.GetRaw());
     // flush changes
     view.Flush();
     // write undo

diff --git a/src/miner.cpp b/src/miner.cpp
@@ -240,7 +240,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
 
     int nPackagesSelected = 0;
     int nDescendantsUpdated = 0;
-    addPackageTxs(nPackagesSelected, nDescendantsUpdated, nHeight);
+    CCustomCSView mnview(*pcustomcsview);
+    addPackageTxs(nPackagesSelected, nDescendantsUpdated, nHeight, mnview);
 
     int64_t nTime1 = GetTimeMicros();
 
@@ -324,6 +325,13 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     }
     int64_t nTime2 = GetTimeMicros();
 
+    pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
+    if (nHeight >= chainparams.GetConsensus().EunosHeight) {
+        // includes coinbase account changes
+        ApplyGeneralCoinbaseTx(mnview, *(pblock->vtx[0]), nHeight, nFees, chainparams.GetConsensus());
+        pblock->hashMerkleRoot = Hash2(pblock->hashMerkleRoot, mnview.MerkleRoot());
+    }
+
     LogPrint(BCLog::BENCH, "CreateNewBlock() packages: %.2fms (%d packages, %d updated descendants), validity: %.2fms (total %.2fms)\n", 0.001 * (nTime1 - nTimeStart), nPackagesSelected, nDescendantsUpdated, 0.001 * (nTime2 - nTime1), 0.001 * (nTime2 - nTimeStart));
 
     return std::move(pblocktemplate);
@@ -449,7 +457,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve
 // Each time through the loop, we compare the best transaction in
 // mapModifiedTxs with the next transaction in the mempool to decide what
 // transaction package to work on next.
-void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight)
+void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight, CCustomCSView &view)
 {
     // mapModifiedTx will store sorted packages after they are modified
     // because some of their txs are already in the block
@@ -474,7 +482,6 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
     std::set<uint256> checkedTX;
 
     // Copy of the view
-    CCustomCSView view(*pcustomcsview);
     CCoinsViewCache coins(&::ChainstateActive().CoinsTip());
 
     while (mi != mempool.mapTx.get<ancestor_score>().end() || !mapModifiedTx.empty())

diff --git a/src/miner.h b/src/miner.h
@@ -180,7 +180,7 @@ class BlockAssembler
     /** Add transactions based on feerate including unconfirmed ancestors
       * Increments nPackagesSelected / nDescendantsUpdated with corresponding
       * statistics from the package selection (for logging statistics). */
-    void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
+    void addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, int nHeight, CCustomCSView &view) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
 
     // helper functions for addPackageTxs()
     /** Remove confirmed (inBlock) entries from given set */

diff --git a/src/pos.cpp b/src/pos.cpp
@@ -158,14 +158,6 @@ boost::optional<std::string> SignPosBlock(std::shared_ptr<CBlock> pblock, const
         throw std::logic_error{"Only non-complete PoS block templates are accepted"};
     }
 
-    // coinstakeTx
-    CMutableTransaction coinstakeTx{*pblock->vtx[0]};
-
-    // Update coinstakeTx after signing
-    pblock->vtx[0] = MakeTransactionRef(coinstakeTx);
-
-    pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
-
     bool signingRes = key.SignCompact(pblock->GetHashToSign(), pblock->sig);
     if (!signingRes) {
         return {std::string{} + "Block signing error"};

diff --git a/src/test/storage_tests.cpp b/src/test/storage_tests.cpp
@@ -77,9 +77,15 @@ static std::vector<unsigned char> ToBytes(const char * in)
     return std::vector<unsigned char>(in, in + strlen(in) + 1);
 }
 
+BOOST_AUTO_TEST_CASE(flushableType)
+{
+    CStorageKV & storage = pcustomcsview->GetStorage();
+    BOOST_REQUIRE(dynamic_cast<CFlushableStorageKV*>(&storage));
+}
+
 BOOST_AUTO_TEST_CASE(undo)
 {
-    CStorageKV & base_raw = pcustomcsview->GetRaw();
+    CStorageKV & base_raw = pcustomcsview->GetStorage();
     // place some "old" record
     pcustomcsview->Write("testkey1", "value0");
 
@@ -90,7 +96,7 @@ BOOST_AUTO_TEST_CASE(undo)
     BOOST_CHECK(mnview.Write("testkey2", "value2")); // insert
 
     // construct undo
-    auto& flushable = dynamic_cast<CFlushableStorageKV&>(mnview.GetRaw());
+    auto& flushable = mnview.GetStorage();
     auto undo = CUndo::Construct(base_raw, flushable.GetRaw());
     BOOST_CHECK(undo.before.size() == 2);
     BOOST_CHECK(undo.before.at(ToBytes("testkey1")) == ToBytes("value0"));

diff --git a/src/validation.cpp b/src/validation.cpp
@@ -2280,6 +2280,9 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
     CAmount nFees = 0;
     int nInputs = 0;
     int64_t nSigOpsCost = 0;
+    // it's used for account changes by the block
+    // to calculate their merkle root in isolation
+    CCustomCSView accountsView(mnview);
     blockundo.vtxundo.reserve(block.vtx.size() - 1);
     std::vector<PrecomputedTransactionData> txdata;
     txdata.reserve(block.vtx.size()); // Required so that pointers to individual PrecomputedTransactionData don't get invalidated
@@ -2292,7 +2295,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
         if (!tx.IsCoinBase())
         {
             CAmount txfee = 0;
-            if (!Consensus::CheckTxInputs(tx, state, view, &mnview, pindex->nHeight, txfee, chainparams)) {
+            if (!Consensus::CheckTxInputs(tx, state, view, &accountsView, pindex->nHeight, txfee, chainparams)) {
                 if (!IsBlockReason(state.GetReason())) {
                     // CheckTxInputs may return MISSING_INPUTS or
                     // PREMATURE_SPEND but we can't return that, as it's not
@@ -2352,7 +2355,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
                     tx.GetHash().ToString(), FormatStateMessage(state));
             }
 
-            const auto res = ApplyCustomTx(mnview, view, tx, chainparams.GetConsensus(), pindex->nHeight, pindex->GetBlockTime(), i, paccountHistoryDB.get());
+            const auto res = ApplyCustomTx(accountsView, view, tx, chainparams.GetConsensus(), pindex->nHeight, pindex->GetBlockTime(), i, paccountHistoryDB.get());
             if (!res.ok && (res.code & CustomTxErrCodes::Fatal)) {
                 // we will never fail, but skip, unless transaction mints UTXOs
                 return error("ConnectBlock(): ApplyCustomTx on %s failed with %s",
@@ -2428,7 +2431,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
     LogPrint(BCLog::BENCH, "      - Connect %u transactions: %.2fms (%.3fms/tx, %.3fms/txin) [%.2fs (%.2fms/blk)]\n", (unsigned)block.vtx.size(), MILLI * (nTime3 - nTime2), MILLI * (nTime3 - nTime2) / block.vtx.size(), nInputs <= 1 ? 0 : MILLI * (nTime3 - nTime2) / (nInputs-1), nTimeConnect * MICRO, nTimeConnect * MILLI / nBlocksTotal);
 
     // chek main coinbase
-    Res res = ApplyGeneralCoinbaseTx(mnview, *block.vtx[0], pindex->nHeight, nFees, chainparams.GetConsensus());
+    Res res = ApplyGeneralCoinbaseTx(accountsView, *block.vtx[0], pindex->nHeight, nFees, chainparams.GetConsensus());
     if (!res.ok) {
         return state.Invalid(ValidationInvalidReason::CONSENSUS,
                              error("ConnectBlock(): %s", res.msg),
@@ -2437,11 +2440,29 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
 
     if (!control.Wait())
         return state.Invalid(ValidationInvalidReason::CONSENSUS, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed");
+
     int64_t nTime4 = GetTimeMicros(); nTimeVerify += nTime4 - nTime2;
     LogPrint(BCLog::BENCH, "    - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs (%.2fms/blk)]\n", nInputs - 1, MILLI * (nTime4 - nTime2), nInputs <= 1 ? 0 : MILLI * (nTime4 - nTime2) / (nInputs-1), nTimeVerify * MICRO, nTimeVerify * MILLI / nBlocksTotal);
 
     if (fJustCheck)
-        return true;
+        return accountsView.Flush(); // keeps compatibility
+
+    // validates account changes as well
+    if (pindex->nHeight >= chainparams.GetConsensus().EunosHeight) {
+        bool mutated;
+        uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated);
+        if (block.hashMerkleRoot != Hash2(hashMerkleRoot2, accountsView.MerkleRoot()))
+            return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txnmrklroot", "hashMerkleRoot mismatch");
+
+        // Check for merkle tree malleability (CVE-2012-2459): repeating sequences
+        // of transactions in a block without affecting the merkle root of a block,
+        // while still invalidating it.
+        if (mutated)
+            return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txns-duplicate", "duplicate transaction");
+    }
+
+    // account changes are validated
+    accountsView.Flush();
 
     if (!WriteUndoDataForBlock(blockundo, state, pindex, chainparams))
         return false;
@@ -2497,8 +2518,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
         }
 
         // construct undo
-        auto& flushable = dynamic_cast<CFlushableStorageKV&>(cache.GetRaw());
-        auto undo = CUndo::Construct(mnview.GetRaw(), flushable.GetRaw());
+        auto& flushable = cache.GetStorage();
+        auto undo = CUndo::Construct(mnview.GetStorage(), flushable.GetRaw());
         // flush changes to underlying view
         cache.Flush();
         // write undo
@@ -3143,11 +3164,17 @@ bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainPar
                         // at this stage only high hash error can be in header
                         // so just skip that block
                         continue;
+                    } else if (reason == ValidationInvalidReason::BLOCK_MUTATED) {
+                        // prior EunosHeight we shoutdown node on mutated block
+                        if (ShutdownRequested()) {
+                            return false;
+                        }
+                        // now block cannot be part of blockchain either
+                        // but it can be produced by outdated/malicious masternode
+                        // so we should not shoutdown entire network, let's skip it
+                        continue;
                     }
-                    if (reason != ValidationInvalidReason::BLOCK_MUTATED) {
-                        InvalidChainFound(vpindexToConnect.front());
-                    }
-                    state = CValidationState();
+                    InvalidChainFound(vpindexToConnect.front());
                     if (fCheckpointsEnabled && pindexConnect == pindexMostWork) {
                         // NOTE: Invalidate blocks back to last checkpoint
                         auto &checkpoints = chainparams.Checkpoints().mapCheckpoints;
@@ -3721,7 +3748,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
         return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "high-hash", "proof of stake failed");
 
     // Check the merkle root.
-    if (fCheckMerkleRoot) {
+    // block merkle root is delayed to ConnectBlock to ensure account changes
+    if (fCheckMerkleRoot && block.height < consensusParams.EunosHeight) {
         bool mutated;
         uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated);
         if (block.hashMerkleRoot != hashMerkleRoot2)

diff --git a/test/functional/feature_accounts_validation.py b/test/functional/feature_accounts_validation.py
@@ -0,0 +1,53 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2019 The Bitcoin Core developers
+# Copyright (c) DeFi Blockchain Developers
+# Distributed under the MIT software license, see the accompanying
+# file LICENSE or http://www.opensource.org/licenses/mit-license.php.
+"""Test account mining behaviour"""
+
+from test_framework.test_framework import DefiTestFramework
+from test_framework.util import assert_equal
+
+class AccountsValidatingTest(DefiTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.setup_clean_chain = True
+        self.extra_args = [
+            ['-txnotokens=0', '-amkheight=50', '-eunosheight=101'],
+            ['-txnotokens=0', '-amkheight=50', '-eunosheight=101'],
+        ]
+
+    def run_test(self):
+        node = self.nodes[0]
+        node1 = self.nodes[1]
+        node.generate(101)
+        self.sync_all()
+
+        assert_equal(node.getblockcount(), 101) # eunos
+
+        # Get addresses and set up account
+        account = node.getnewaddress()
+        destination = node.getnewaddress()
+        node.utxostoaccount({account: "10@0"})
+        node.generate(1)
+        self.sync_all()
+
+        # Check we have expected balance
+        assert_equal(node1.getaccount(account)[0], "10.00000000@DFI")
+
+        node.accounttoaccount(account, {destination: "1@DFI"})
+        node.accounttoutxos(account, {destination: "1@DFI"})
+        node.accounttoutxos(account, {destination: "2@DFI"})
+
+        # Store block height
+        blockcount = node.getblockcount()
+
+        # Generate a block
+        node.generate(1)
+        self.sync_all()
+
+        # Check the blockchain has extended as expected
+        assert_equal(node1.getblockcount(), blockcount + 1)
+
+if __name__ == '__main__':
+    AccountsValidatingTest().main ()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
@@ -244,6 +244,7 @@
     'feature_blocksdir.py',
     'feature_config_args.py',
     'feature_account_mining.py',
+    'feature_accounts_validation.py',
     'rpc_help.py',
     'feature_help.py',
     'feature_shutdown.py',