Merge pull request #456 from aguycalled/ignore-preq-invalid-hash-patch

Fix for Payment Request reorganizations

Author: proletesseract

qa/pull-tester/rpc-tests.py

@@ -165,6 +165,7 @@
     'cfund-vote.py',
     'cfund-proposalvotelist.py',
     'cfund-paymentrequestvotelist.py',
+    'cfund-paymentrequest-state-reorg.py',
     'reject-version-bit.py',
     'getcoldstakingaddress.py',
     'getstakereport.py',

qa/rpc-tests/cfund-paymentrequest-extract-funds.py

@@ -47,15 +47,21 @@ def run_test(self):
 
         # Create 6 payment requests
         paymentRequests = []
-        for x in range(6):
+        for x in range(5):
             paymentRequests.append(self.nodes[0].createpaymentrequest(proposalid0, 20, "test0")["hash"])
 
+	# The last 1 should be rejected as it excedes proposal balance
+        try:
+            paymentRequests.append(self.nodes[0].createpaymentrequest(proposalid0, 20, "test0")["hash"])
+            raise ValueError("Error should be thrown for invalid prequest")
+        except JSONRPCException as e:
+            assert("The transaction was rejected" in e.error['message'])
+
         slow_gen(self.nodes[0], 1)
 
         # One of them should have been rejected at creation
         valid = 0
         invalid = 0
-        invalid_pos = -1
 
         for paymentReq in paymentRequests:
             try:
@@ -64,13 +70,10 @@ def run_test(self):
                 valid = valid + 1
             except JSONRPCException:
                 invalid = invalid + 1
-                invalid_pos = valid 
                 continue
 
         assert(valid == 5)
-        assert(invalid == 1)
-
-        paymentRequests.pop(invalid_pos)
+        assert(invalid == 0)
 
         assert(self.nodes[0].cfundstats()["funds"]["locked"] == locked_accepted)
 

qa/rpc-tests/cfund-paymentrequest-state-reorg.py

@@ -0,0 +1,108 @@
+#!/usr/bin/env python3
+# Copyright (c) 2019 The Navcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import NavCoinTestFramework
+from test_framework.cfund_util import *
+
+import time
+import urllib.parse
+
+
+class CFundPaymentRequestStateReorg(NavCoinTestFramework):
+    """Tests consistency of Community Fund Payment Requests state through reorgs."""
+
+    def __init__(self):
+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 2
+
+    def setup_network(self, split=False):
+        self.nodes = []
+        self.nodes.append(start_node(0, self.options.tmpdir, ["-debug","-headerspamfiltermaxsize=1000"]))
+        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug","-headerspamfiltermaxsize=1000"]))
+        connect_nodes(self.nodes[0], 1)
+        self.is_network_split = False
+
+    def run_test(self):
+        self.nodes[0].staking(False)
+        self.nodes[1].staking(False)
+        activate_cfund(self.nodes[0])
+        self.sync_all()
+
+        self.nodes[0].donatefund(100)
+
+        # Generate our addresses
+        node_0_address = self.nodes[0].getnewaddress()
+        node_1_address = self.nodes[1].getnewaddress()
+
+        # Split funds
+        self.nodes[0].sendtoaddress(node_1_address, 5000000)
+        proposal=self.nodes[0].createproposal(node_0_address, 100, 36000, "test")
+        proposal_id=proposal["hash"]
+
+        slow_gen(self.nodes[0], 1)
+        end_cycle(self.nodes[0])
+
+        self.sync_all()
+
+        self.nodes[0].proposalvote(proposal_id, "yes")
+
+        slow_gen(self.nodes[0], 1)
+        end_cycle(self.nodes[0])
+
+        self.sync_all()
+
+        assert(self.nodes[0].getproposal(proposal_id)["state"] == 1)
+        assert(self.nodes[0].getproposal(proposal_id)["status"] == "accepted")
+
+        assert(self.nodes[1].getproposal(proposal_id)["state"] == 1)
+        assert(self.nodes[1].getproposal(proposal_id)["status"] == "accepted")
+
+        raw_preq = self.nodes[0].createpaymentrequest(proposal_id, 100, "preq", True)
+        self.sync_all()
+
+        # Disconnect Nodes 0 and 1
+        url = urllib.parse.urlparse(self.nodes[1].url)
+        self.nodes[0].disconnectnode(url.hostname+":"+str(p2p_port(1)))
+
+        self.nodes[0].forcetransactions([raw_preq])
+        self.nodes[1].forcetransactions([raw_preq])
+
+        blockcount_0 = self.nodes[0].getblockcount()
+        blockcount_1 = self.nodes[1].getblockcount()
+
+        self.nodes[0].staking(True)
+        self.nodes[1].staking(True)
+
+        # Let's wait for at least 20 blocks from Node 0
+        while self.nodes[0].getblockcount() - blockcount_0 < 20:
+            time.sleep(1)
+
+        # Node 1 only has 1 output so it will only stake 1 block
+        while self.nodes[1].getblockcount() == blockcount_1:
+            time.sleep(1)
+
+        print("nodes staked!")
+
+        self.nodes[0].staking(False)
+        self.nodes[1].staking(False)
+
+        node_0_best_hash = self.nodes[0].getblockhash(self.nodes[0].getblockcount())
+        node_1_best_hash = self.nodes[1].getblockhash(self.nodes[1].getblockcount())
+
+        # Node 0 and Node 1 have forked.
+        assert(node_0_best_hash != node_1_best_hash)
+
+        connect_nodes(self.nodes[0], 1)
+
+        self.sync_all()
+
+        node_1_best_hash_ = self.nodes[1].getblockhash(self.nodes[1].getblockcount())
+
+        # Node 1 must have reorg'd to Node 0 chain
+        assert(node_0_best_hash == node_1_best_hash_)
+
+if __name__ == '__main__':
+    CFundPaymentRequestStateReorg().main()

qa/rpc-tests/cfund-rawtx-paymentrequest-create.py

@@ -111,8 +111,16 @@ def run_test(self):
 
         # Create multiple payment requests
         self.send_raw_paymentrequest(6, self.goodAddress, self.goodProposalHash, "sum_to_too_large_amount0")
-        self.send_raw_paymentrequest(6, self.goodAddress, self.goodProposalHash, "sum_to_too_large_amount1")
-        self.send_raw_paymentrequest(6, self.goodAddress, self.goodProposalHash, "sum_to_too_large_amount2")
+        try:
+            self.send_raw_paymentrequest(6, self.goodAddress, self.goodProposalHash, "sum_to_too_large_amount1")
+            raise ValueError("Error should be thrown for invalid rawtx (prequest)")
+        except JSONRPCException as e:
+            assert("bad-cfund-payment-request" in e.error['message'])
+        try:
+            self.send_raw_paymentrequest(6, self.goodAddress, self.goodProposalHash, "sum_to_too_large_amount2")
+            raise ValueError("Error should be thrown for invalid rawtx (prequest)")
+        except JSONRPCException as e:
+            assert("bad-cfund-payment-request" in e.error['message'])
 
         slow_gen(self.nodes[0], 1)
 

src/consensus/cfund.cpp

@@ -79,11 +79,12 @@ bool CFund::FindPaymentRequest(string preqstr, CFund::CPaymentRequest &prequest)
 
 bool CFund::VoteProposal(string strProp, bool vote, bool &duplicate)
 {
+    AssertLockHeld(cs_main);
 
     CFund::CProposal proposal;
     bool found = CFund::FindProposal(uint256S("0x"+strProp), proposal);
 
-    if(!found || proposal.IsNull())
+    if(!found || proposal.IsNull() || !proposal.CanVote())
         return false;
 
     vector<std::pair<std::string, bool>>::iterator it = vAddedProposalVotes.begin();
@@ -133,11 +134,12 @@ bool CFund::RemoveVoteProposal(uint256 proposalHash)
 
 bool CFund::VotePaymentRequest(string strProp, bool vote, bool &duplicate)
 {
+    AssertLockHeld(cs_main);
 
     CFund::CPaymentRequest prequest;
     bool found = CFund::FindPaymentRequest(uint256S("0x"+strProp), prequest);
 
-    if(!found || prequest.IsNull())
+    if(!found || prequest.IsNull() || !prequest.CanVote(*pcoinsTip))
         return false;
 
     vector<std::pair<std::string, bool>>::iterator it = vAddedPaymentRequestVotes.begin();
@@ -188,7 +190,7 @@ bool CFund::RemoveVotePaymentRequest(uint256 proposalHash)
     return RemoveVotePaymentRequest(proposalHash.ToString());
 }
 
-bool CFund::IsValidPaymentRequest(CTransaction tx, int nMaxVersion)
+bool CFund::IsValidPaymentRequest(CTransaction tx, CCoinsViewCache& coins, int nMaxVersion)
 {    
     if(tx.strDZeel.length() > 1024)
         return error("%s: Too long strdzeel for payment request %s", __func__, tx.GetHash().ToString());
@@ -258,9 +260,9 @@ bool CFund::IsValidPaymentRequest(CTransaction tx, int nMaxVersion)
     if (!pubkey.RecoverCompact(ss.GetHash(), vchSig) || pubkey.GetID() != keyID)
         return error("%s: Invalid signature for payment request %s", __func__, tx.GetHash().ToString());
 
-    if(nAmount > proposal.GetAvailable(true))
+    if(nAmount > proposal.GetAvailable(coins, true))
         return error("%s: Invalid requested amount for payment request %s (%d vs %d available)",
-                     __func__, tx.GetHash().ToString(), nAmount, proposal.GetAvailable());
+                     __func__, tx.GetHash().ToString(), nAmount, proposal.GetAvailable(coins, true));
 
     bool ret = (nVersion <= nMaxVersion);
 
@@ -271,11 +273,23 @@ bool CFund::IsValidPaymentRequest(CTransaction tx, int nMaxVersion)
 
 }
 
-bool CFund::CPaymentRequest::CanVote() const {
+bool CFund::CPaymentRequest::CanVote(CCoinsViewCache& coins) const
+{
+    AssertLockHeld(cs_main);
+
+    CBlockIndex* pindex;
+    if(txblockhash == uint256() || !mapBlockIndex.count(txblockhash))
+        return false;
+
+    pindex = mapBlockIndex[txblockhash];
+    if(!chainActive.Contains(pindex))
+        return false;
+
     CFund::CProposal proposal;
     if(!CFund::FindProposal(proposalhash, proposal))
         return false;
-    return nAmount <= proposal.GetAvailable() && fState != ACCEPTED && fState != REJECTED && fState != EXPIRED && !ExceededMaxVotingCycles();
+
+    return nAmount <= proposal.GetAvailable(coins) && fState != ACCEPTED && fState != REJECTED && fState != EXPIRED && !ExceededMaxVotingCycles();
 }
 
 bool CFund::CPaymentRequest::IsExpired() const {
@@ -388,6 +402,16 @@ bool CFund::CProposal::IsRejected() const {
 }
 
 bool CFund::CProposal::CanVote() const {
+    AssertLockHeld(cs_main);
+
+    CBlockIndex* pindex;
+    if(txblockhash == uint256() || !mapBlockIndex.count(txblockhash))
+        return false;
+
+    pindex = mapBlockIndex[txblockhash];
+    if(!chainActive.Contains(pindex))
+        return false;
+
     return (fState == NIL) && (!ExceededMaxVotingCycles());
 }
 
@@ -418,6 +442,59 @@ bool CFund::CProposal::ExceededMaxVotingCycles() const {
     return nVotingCycle > Params().GetConsensus().nCyclesProposalVoting;
 }
 
+CAmount CFund::CProposal::GetAvailable(CCoinsViewCache& coins, bool fIncludeRequests) const
+{
+    AssertLockHeld(cs_main);
+
+    CAmount initial = nAmount;
+    for (unsigned int i = 0; i < vPayments.size(); i++)
+    {
+        CFund::CPaymentRequest prequest;
+        if(FindPaymentRequest(vPayments[i], prequest))
+        {
+            if (!coins.HaveCoins(prequest.hash))
+            {
+                CBlockIndex* pindex;
+                if(prequest.txblockhash == uint256() || !mapBlockIndex.count(prequest.txblockhash))
+                    continue;
+                pindex = mapBlockIndex[prequest.txblockhash];
+                if(!chainActive.Contains(pindex))
+                    continue;
+            }
+            if((fIncludeRequests && prequest.fState != REJECTED && prequest.fState != EXPIRED) || (!fIncludeRequests && prequest.fState == ACCEPTED))
+                initial -= prequest.nAmount;
+        }
+    }
+    return initial;
+}
+
+std::string CFund::CProposal::ToString(CCoinsViewCache& coins, uint32_t currentTime) const {
+    std::string str;
+    str += strprintf("CProposal(hash=%s, nVersion=%i, nAmount=%f, available=%f, nFee=%f, address=%s, nDeadline=%u, nVotesYes=%u, "
+                     "nVotesNo=%u, nVotingCycle=%u, fState=%s, strDZeel=%s, blockhash=%s)",
+                     hash.ToString(), nVersion, (float)nAmount/COIN, (float)GetAvailable(coins)/COIN, (float)nFee/COIN, Address, nDeadline,
+                     nVotesYes, nVotesNo, nVotingCycle, GetState(currentTime), strDZeel, blockhash.ToString().substr(0,10));
+    for (unsigned int i = 0; i < vPayments.size(); i++) {
+        CFund::CPaymentRequest prequest;
+        if(FindPaymentRequest(vPayments[i], prequest))
+            str += "\n    " + prequest.ToString();
+    }
+    return str + "\n";
+}
+
+bool CFund::CProposal::HasPendingPaymentRequests(CCoinsViewCache& coins) const {
+    AssertLockHeld(cs_main);
+
+    for (unsigned int i = 0; i < vPayments.size(); i++)
+    {
+        CFund::CPaymentRequest prequest;
+        if(FindPaymentRequest(vPayments[i], prequest))
+            if(prequest.CanVote(coins))
+                return true;
+    }
+    return false;
+}
+
 std::string CFund::CProposal::GetState(uint32_t currentTime) const {
     std::string sFlags = "pending";
     if(IsAccepted()) {
@@ -444,13 +521,15 @@ std::string CFund::CProposal::GetState(uint32_t currentTime) const {
     return sFlags;
 }
 
-void CFund::CProposal::ToJson(UniValue& ret) const {
+void CFund::CProposal::ToJson(UniValue& ret, CCoinsViewCache& coins) const {
+    AssertLockHeld(cs_main);
+
     ret.push_back(Pair("version", nVersion));
     ret.push_back(Pair("hash", hash.ToString()));
     ret.push_back(Pair("blockHash", txblockhash.ToString()));
     ret.push_back(Pair("description", strDZeel));
     ret.push_back(Pair("requestedAmount", FormatMoney(nAmount)));
-    ret.push_back(Pair("notPaidYet", FormatMoney(GetAvailable())));
+    ret.push_back(Pair("notPaidYet", FormatMoney(GetAvailable(coins))));
     ret.push_back(Pair("userPaidFee", FormatMoney(nFee)));
     ret.push_back(Pair("paymentAddress", Address));
     if(nVersion >= 2) {