Skip to content

Commit e688c6e

Browse files
proletesseractproletesseract
authored
Merge pull request #608 from aguycalled/cfund-proposal-reorg
Fix CFund DB read after nullified entry
2 parents 5562509 + a5f9b9e commit e688c6e

5 files changed

Lines changed: 119 additions & 12 deletions

File tree

‎qa/pull-tester/rpc-tests.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@
162162
'cfund-paymentrequest-state-expired.py',
163163
'cfund-proposal-state-accept.py',
164164
'cfund-proposal-state-expired.py',
165+
'cfund-reorg.py',
165166
'cfund-rawtx-create-proposal.py',
166167
'cfund-rawtx-paymentrequest-create.py',
167168
'cfund-rawtx-paymentrequest-vote.py',

‎qa/rpc-tests/cfund-reorg.py‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
#!/usr/bin/env python3
2+
# Copyright (c) 2019 The Navcoin Core developers
3+
# Distributed under the MIT software license, see the accompanying
4+
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+
from test_framework.test_framework import NavCoinTestFramework
7+
from test_framework.cfund_util import *
8+
9+
import urllib.parse
10+
11+
class CommunityFundProposalReorg(NavCoinTestFramework):
12+
def __init__(self):
13+
super().__init__()
14+
self.setup_clean_chain = True
15+
self.num_nodes = 2
16+
17+
def setup_network(self, split=False):
18+
self.nodes = []
19+
20+
self.nodes.append(start_node(0, self.options.tmpdir, ["-debug=cfund"]))
21+
self.nodes.append(start_node(1, self.options.tmpdir, ["-debug=cfund"]))
22+
23+
connect_nodes_bi(self.nodes, 0, 1)
24+
25+
self.is_network_split = split
26+
27+
def run_test(self):
28+
self.nodes[0].staking(False)
29+
self.nodes[0].staking(False)
30+
31+
activate_cfund(self.nodes[0])
32+
sync_blocks(self.nodes)
33+
34+
self.nodes[0].donatefund(10)
35+
slow_gen(self.nodes[0], 1)
36+
37+
rawproposal = self.nodes[0].createproposal(self.nodes[0].getnewaddress(), 10, 36000, "test", 50, True)
38+
39+
# disconnect the nodes and generate the proposal on each node
40+
url = urllib.parse.urlparse(self.nodes[1].url)
41+
self.nodes[0].disconnectnode(url.hostname+":"+str(p2p_port(1)))
42+
43+
time.sleep(2) # wait for disconnect
44+
45+
hash = self.nodes[0].sendrawtransaction(rawproposal)
46+
self.nodes[1].sendrawtransaction(rawproposal)
47+
48+
self.nodes[0].generate(1)
49+
self.nodes[1].generate(2)
50+
51+
connect_nodes_bi(self.nodes, 0, 1)
52+
sync_blocks(self.nodes)
53+
54+
assert_equal(self.nodes[0].getproposal(hash), self.nodes[1].getproposal(hash))
55+
56+
self.nodes[0].proposalvote(hash, "yes")
57+
slow_gen(self.nodes[0], 1)
58+
end_cycle(self.nodes[0])
59+
60+
sync_blocks(self.nodes)
61+
62+
assert_equal(self.nodes[0].getproposal(hash)['status'], 'accepted')
63+
64+
rawpaymentrequest = self.nodes[0].createpaymentrequest(hash, 10, "paymentReq1", True)
65+
66+
# disconnect the nodes and generate the proposal on each node
67+
url = urllib.parse.urlparse(self.nodes[1].url)
68+
self.nodes[0].disconnectnode(url.hostname+":"+str(p2p_port(1)))
69+
70+
time.sleep(2) # wait for disconnect
71+
72+
hash = self.nodes[0].sendrawtransaction(rawpaymentrequest)
73+
assert_equal(self.nodes[1].sendrawtransaction(rawpaymentrequest), hash)
74+
75+
self.nodes[0].generate(1)
76+
self.nodes[1].generate(2)
77+
78+
blockhash0 = self.nodes[0].getpaymentrequest(hash)["blockHash"]
79+
blockhash1 = self.nodes[1].getpaymentrequest(hash)["blockHash"]
80+
81+
longestChain = self.nodes[1].getbestblockhash()
82+
83+
preq1 = self.nodes[0].getpaymentrequest(hash)
84+
85+
# I would have assumed reorg to node 1 should reorg the payment request and probably include it in the next block?
86+
connect_nodes_bi(self.nodes, 0, 1)
87+
sync_blocks(self.nodes)
88+
89+
# check the nodes reorged to the longest chain (node 1)
90+
assert_equal(self.nodes[0].getbestblockhash(), longestChain)
91+
assert_equal(self.nodes[0].getbestblockhash(), self.nodes[1].getbestblockhash())
92+
93+
assert_equal(self.nodes[0].getpaymentrequest(hash), self.nodes[1].getpaymentrequest(hash))
94+
assert_equal(self.nodes[0].getpaymentrequest(hash)['hash'], hash)
95+
96+
assert_equal(self.nodes[0].getpaymentrequest(hash)["blockHash"], blockhash1)
97+
assert_equal(self.nodes[1].getpaymentrequest(hash)["blockHash"], blockhash1)
98+
99+
100+
101+
if __name__ == '__main__':
102+
CommunityFundProposalReorg().main()

‎src/coins.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ CProposalMap::const_iterator CCoinsViewCache::FetchProposal(const uint256 &pid)
113113

114114
CProposal tmp;
115115

116-
if (!base->GetProposal(pid, tmp))
116+
if (!base->GetProposal(pid, tmp) || tmp.IsNull())
117117
return cacheProposals.end();
118118

119119
CProposalMap::iterator ret = cacheProposals.insert(std::make_pair(pid, CProposal())).first;
@@ -130,7 +130,7 @@ CPaymentRequestMap::const_iterator CCoinsViewCache::FetchPaymentRequest(const ui
130130

131131
CPaymentRequest tmp;
132132

133-
if (!base->GetPaymentRequest(prid, tmp))
133+
if (!base->GetPaymentRequest(prid, tmp) || tmp.IsNull())
134134
return cachePaymentRequests.end();
135135

136136
CPaymentRequestMap::iterator ret = cachePaymentRequests.insert(std::make_pair(prid, CPaymentRequest())).first;

‎src/consensus/cfund.cpp‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -737,10 +737,10 @@ void CFund::CFundStep(const CValidationState& state, CBlockIndex *pindexNew, con
737737
if (!view.GetProposal(prequest->proposalhash, proposal))
738738
continue;
739739

740-
auto nCreatedOnCycle = (unsigned )(pblockindex->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
741-
auto nCurrentCycle = (unsigned )(pindexNew->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
742-
auto nElapsedCycles = nCurrentCycle - nCreatedOnCycle;
743-
auto nVotingCycles = std::min(nElapsedCycles, Params().GetConsensus().nCyclesPaymentRequestVoting + 1);
740+
int nCreatedOnCycle = (pblockindex->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
741+
int nCurrentCycle = (pindexNew->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
742+
int nElapsedCycles = std::max(nCurrentCycle - nCreatedOnCycle, 0);
743+
int nVotingCycles = std::min(nElapsedCycles, (int)Params().GetConsensus().nCyclesPaymentRequestVoting + 1);
744744

745745
auto oldState = prequest->fState;
746746
auto oldCycle = prequest->nVotingCycle;
@@ -803,7 +803,9 @@ void CFund::CFundStep(const CValidationState& state, CBlockIndex *pindexNew, con
803803

804804
if (fUndo && fUpdate && prequest->fState == oldState && prequest->fState != CFund::NIL
805805
&& prequest->nVotingCycle != oldCycle)
806+
{
806807
prequest->nVotingCycle = oldCycle;
808+
}
807809

808810
if((pindexNew->nHeight) % Params().GetConsensus().nBlocksPerVotingCycle == 0)
809811
{
@@ -854,10 +856,10 @@ void CFund::CFundStep(const CValidationState& state, CBlockIndex *pindexNew, con
854856

855857
CBlockIndex* pblockindex = mapBlockIndex[proposal->txblockhash];
856858

857-
auto nCreatedOnCycle = (unsigned int)(pblockindex->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
858-
auto nCurrentCycle = (unsigned int)(pindexNew->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
859-
auto nElapsedCycles = nCurrentCycle - nCreatedOnCycle;
860-
auto nVotingCycles = std::min(nElapsedCycles, Params().GetConsensus().nCyclesProposalVoting + 1);
859+
int nCreatedOnCycle = (pblockindex->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
860+
int nCurrentCycle = (pindexNew->nHeight / Params().GetConsensus().nBlocksPerVotingCycle);
861+
int nElapsedCycles = std::max(nCurrentCycle - nCreatedOnCycle, 0);
862+
int nVotingCycles = std::min(nElapsedCycles, (int)Params().GetConsensus().nCyclesProposalVoting + 1);
861863

862864
auto oldState = proposal->fState;
863865
auto oldCycle = proposal->nVotingCycle;
@@ -938,7 +940,9 @@ void CFund::CFundStep(const CValidationState& state, CBlockIndex *pindexNew, con
938940
}
939941

940942
if (fUndo && fUpdate && proposal->fState == oldState && proposal->fState != CFund::NIL && proposal->nVotingCycle != oldCycle)
943+
{
941944
proposal->nVotingCycle = oldCycle;
945+
}
942946

943947
if((pindexNew->nHeight) % Params().GetConsensus().nBlocksPerVotingCycle == 0)
944948
{

‎src/main.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2453,11 +2453,11 @@ bool DisconnectBlock(const CBlock& block, CValidationState& state, const CBlockI
24532453
int nMaxVersionProposal = fReducedQuorum ? Params().GetConsensus().nProposalMaxVersion : 2;
24542454
int nMaxVersionPaymentRequest = fReducedQuorum ? Params().GetConsensus().nPaymentRequestMaxVersion : 2;
24552455

2456-
if(tx.nVersion == CTransaction::PROPOSAL_VERSION && CFund::IsValidProposal(tx, nMaxVersionProposal)) {
2456+
if(tx.nVersion == CTransaction::PROPOSAL_VERSION) {
24572457
view.RemoveProposal(hash);
24582458
}
24592459

2460-
if(tx.nVersion == CTransaction::PAYMENT_REQUEST_VERSION && CFund::IsValidPaymentRequest(tx, view, nMaxVersionPaymentRequest)) {
2460+
if(tx.nVersion == CTransaction::PAYMENT_REQUEST_VERSION) {
24612461
view.RemovePaymentRequest(hash);
24622462
}
24632463
}

0 commit comments

Comments
 (0)