Skip to content

Commit a6782c3

Browse files
marcus290marcus290
authored
Interim fix for #391 cfund votingCycle increments (#396)
* only reset voting cycle when reorg causes state change * adds test to votingCycle change on reorg and stops votes counted for expired proposals * fix typos in test and logic * allow reset of votingCycle on reorg if state is pending * allow reset of votingCycle on reorg if state is pending * adds support for invalidateBlock where the fState was previously PENDING_VOTING_PREQ * limit max votingCycle output from toJson and modify GetState for nVotingCycle+1 * fix broken tests * comments and readibility fixes
1 parent c821bad commit a6782c3

File tree

6 files changed

+83
-47
lines changed

6 files changed

+83
-47
lines changed

‎qa/rpc-tests/cfund-paymentrequest-state-expired.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ def run_test(self):
5353
start_new_cycle(self.nodes[0])
5454
i = i + 1
5555

56-
# Validate that the payment request will expire at the end of the current voting cycle
56+
# Validate that the status of the payment request is expired
5757
assert (self.nodes[0].getpaymentrequest(paymentrequestid0)["state"] == 0)
58-
assert (self.nodes[0].getpaymentrequest(paymentrequestid0)["status"] == "expired waiting for end of voting period")
58+
assert (self.nodes[0].getpaymentrequest(paymentrequestid0)["status"] == "expired")
5959

6060
# Move to the last block of the voting cycle, where the payment request state changes
6161
end_cycle(self.nodes[0])

‎qa/rpc-tests/cfund-proposal-state-accept.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ def run_test(self):
2424
self.nodes[0].staking(False)
2525
activate_cfund(self.nodes[0])
2626

27-
proposal_duration = 3
27+
proposal_duration = 30
2828

2929
proposalid0 = self.nodes[0].createproposal(self.nodes[0].getnewaddress(), 1, proposal_duration, "test")["hash"]
3030
slow_gen(self.nodes[0], 1)
@@ -147,6 +147,13 @@ def run_test(self):
147147
assert (self.nodes[0].cfundstats()["funds"]["available"] == self.nodes[0].cfundstats()["consensus"]["proposalMinimalFee"])
148148
assert (self.nodes[0].cfundstats()["funds"]["locked"] == 1)
149149

150+
# Check the voting cycle does not increment in later cycle after reorg
151+
votingCycle_after_state_change = self.nodes[0].getproposal(proposalid0)["votingCycle"]
152+
start_new_cycle(self.nodes[0])
153+
blocks=slow_gen(self.nodes[0], 1)
154+
self.nodes[0].invalidateblock(blocks[-1])
155+
assert(self.nodes[0].getproposal(proposalid0)["votingCycle"] == votingCycle_after_state_change)
156+
150157
# Wait for the proposal to expire
151158
while int(time.time()) <= int(self.nodes[0].getproposal(proposalid0)["expiresOn"]):
152159
time.sleep(1)

‎qa/rpc-tests/cfund-proposal-state-expired.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ def run_test(self):
4141
start_new_cycle(self.nodes[0])
4242
i = i + 1
4343

44-
# Validate that the proposal will expire at the end of the current voting cycle
44+
# Validate that the status of the proposal is expired
4545
assert (self.nodes[0].getproposal(proposalid0)["state"] == 0)
46-
assert (self.nodes[0].getproposal(proposalid0)["status"] == "expired waiting for end of voting period")
46+
assert (self.nodes[0].getproposal(proposalid0)["status"] == "expired")
4747

4848
# Move to the last block of the voting cycle, where the proposal state changes
4949
end_cycle(self.nodes[0])

‎src/consensus/cfund.cpp‎

Lines changed: 49 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -275,13 +275,12 @@ bool CFund::CPaymentRequest::CanVote() const {
275275
CFund::CProposal proposal;
276276
if(!CFund::FindProposal(proposalhash, proposal))
277277
return false;
278-
return nAmount <= proposal.GetAvailable() && fState != ACCEPTED && fState != REJECTED && fState != EXPIRED;
278+
return nAmount <= proposal.GetAvailable() && fState != ACCEPTED && fState != REJECTED && fState != EXPIRED && !ExceededMaxVotingCycles();
279279
}
280280

281281
bool CFund::CPaymentRequest::IsExpired() const {
282282
if(nVersion >= 2)
283-
return ( nVotingCycle > Params().GetConsensus().nCyclesPaymentRequestVoting &&
284-
fState != ACCEPTED && fState != REJECTED);
283+
return (ExceededMaxVotingCycles() && fState != ACCEPTED && fState != REJECTED);
285284
return false;
286285
}
287286

@@ -364,6 +363,10 @@ bool CFund::CPaymentRequest::IsRejected() const {
364363
&& ((float)nVotesNo > ((float)(nTotalVotes) * Params().GetConsensus().nVotesRejectPaymentRequest));
365364
}
366365

366+
bool CFund::CPaymentRequest::ExceededMaxVotingCycles() const {
367+
return nVotingCycle > Params().GetConsensus().nCyclesPaymentRequestVoting;
368+
}
369+
367370
bool CFund::CProposal::IsAccepted() const {
368371
int nTotalVotes = nVotesYes + nVotesNo;
369372
float nMinimumQuorum = Params().GetConsensus().nMinimumQuorum;
@@ -384,18 +387,52 @@ bool CFund::CProposal::IsRejected() const {
384387
&& ((float)nVotesNo > ((float)(nTotalVotes) * Params().GetConsensus().nVotesRejectProposal));
385388
}
386389

390+
bool CFund::CProposal::CanVote() const {
391+
return (fState == NIL) && (!ExceededMaxVotingCycles());
392+
}
393+
387394
bool CFund::CProposal::IsExpired(uint32_t currentTime) const {
388395
if(nVersion >= 2) {
389396
if (fState == ACCEPTED && mapBlockIndex.count(blockhash) > 0) {
390-
CBlockIndex* pblockindex = mapBlockIndex[blockhash];
391-
return (pblockindex->GetBlockTime() + nDeadline < currentTime);
397+
CBlockIndex* pBlockIndex = mapBlockIndex[blockhash];
398+
return (pBlockIndex->GetBlockTime() + nDeadline < currentTime);
392399
}
393-
return (fState == EXPIRED) || (fState == PENDING_VOTING_PREQ) || (nVotingCycle > Params().GetConsensus().nCyclesProposalVoting && (CanVote() || fState == EXPIRED));
400+
return (fState == EXPIRED) || (fState == PENDING_VOTING_PREQ) || (ExceededMaxVotingCycles() && fState == NIL);
394401
} else {
395402
return (nDeadline < currentTime);
396403
}
397404
}
398405

406+
bool CFund::CProposal::ExceededMaxVotingCycles() const {
407+
return nVotingCycle > Params().GetConsensus().nCyclesProposalVoting;
408+
}
409+
410+
std::string CFund::CProposal::GetState(uint32_t currentTime) const {
411+
std::string sFlags = "pending";
412+
if(IsAccepted()) {
413+
sFlags = "accepted";
414+
if(fState == PENDING_FUNDS)
415+
sFlags += " waiting for enough coins in fund";
416+
else if(fState != ACCEPTED)
417+
sFlags += " waiting for end of voting period";
418+
}
419+
if(IsRejected()) {
420+
sFlags = "rejected";
421+
if(fState != REJECTED)
422+
sFlags += " waiting for end of voting period";
423+
}
424+
if(currentTime > 0 && IsExpired(currentTime)) {
425+
sFlags = "expired";
426+
if(fState != EXPIRED && !ExceededMaxVotingCycles())
427+
// This branch only occurs when a proposal expires due to exceeding its nDeadline during a voting cycle, not due to exceeding max voting cycles
428+
sFlags += " waiting for end of voting period";
429+
}
430+
if(fState == PENDING_VOTING_PREQ) {
431+
sFlags = "expired pending voting of payment requests";
432+
}
433+
return sFlags;
434+
}
435+
399436
void CFund::CProposal::ToJson(UniValue& ret) const {
400437
ret.push_back(Pair("version", nVersion));
401438
ret.push_back(Pair("hash", hash.ToString()));
@@ -408,15 +445,16 @@ void CFund::CProposal::ToJson(UniValue& ret) const {
408445
if(nVersion >= 2) {
409446
ret.push_back(Pair("proposalDuration", (uint64_t)nDeadline));
410447
if (fState == ACCEPTED && mapBlockIndex.count(blockhash) > 0) {
411-
CBlockIndex* pblockindex = mapBlockIndex[blockhash];
412-
ret.push_back(Pair("expiresOn", pblockindex->GetBlockTime() + (uint64_t)nDeadline));
448+
CBlockIndex* pBlockIndex = mapBlockIndex[blockhash];
449+
ret.push_back(Pair("expiresOn", pBlockIndex->GetBlockTime() + (uint64_t)nDeadline));
413450
}
414451
} else {
415452
ret.push_back(Pair("expiresOn", (uint64_t)nDeadline));
416453
}
417454
ret.push_back(Pair("votesYes", nVotesYes));
418455
ret.push_back(Pair("votesNo", nVotesNo));
419-
ret.push_back(Pair("votingCycle", (uint64_t)nVotingCycle));
456+
ret.push_back(Pair("votingCycle", (uint64_t)std::min(nVotingCycle, Params().GetConsensus().nCyclesProposalVoting)));
457+
// votingCycle does not return higher than nCyclesProposalVoting to avoid reader confusion, since votes are not counted anyway when votingCycle > nCyclesProposalVoting
420458
ret.push_back(Pair("status", GetState(chainActive.Tip()->GetBlockTime())));
421459
ret.push_back(Pair("state", (uint64_t)fState));
422460
if(fState == ACCEPTED)
@@ -443,7 +481,8 @@ void CFund::CPaymentRequest::ToJson(UniValue& ret) const {
443481
ret.push_back(Pair("requestedAmount", FormatMoney(nAmount)));
444482
ret.push_back(Pair("votesYes", nVotesYes));
445483
ret.push_back(Pair("votesNo", nVotesNo));
446-
ret.push_back(Pair("votingCycle", (uint64_t)nVotingCycle));
484+
ret.push_back(Pair("votingCycle", (uint64_t)std::min(nVotingCycle, Params().GetConsensus().nCyclesPaymentRequestVoting)));
485+
// votingCycle does not return higher than nCyclesPaymentRequestVoting to avoid reader confusion, since votes are not counted anyway when votingCycle > nCyclesPaymentRequestVoting
447486
ret.push_back(Pair("status", GetState()));
448487
ret.push_back(Pair("state", (uint64_t)fState));
449488
ret.push_back(Pair("stateChangedOnBlock", blockhash.ToString()));

‎src/consensus/cfund.h‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,8 @@ class CPaymentRequest
102102
if(fState != REJECTED)
103103
sFlags += " waiting for end of voting period";
104104
}
105-
if(IsExpired()) {
105+
if(IsExpired())
106106
sFlags = "expired";
107-
if(fState != EXPIRED)
108-
sFlags += " waiting for end of voting period";
109-
}
110107
return sFlags;
111108
}
112109

@@ -126,6 +123,8 @@ class CPaymentRequest
126123

127124
bool IsExpired() const;
128125

126+
bool ExceededMaxVotingCycles() const;
127+
129128
bool CanVote() const;
130129

131130
ADD_SERIALIZE_METHODS;
@@ -227,30 +226,7 @@ class CProposal
227226
return str + "\n";
228227
}
229228

230-
std::string GetState(uint32_t currentTime) const {
231-
std::string sFlags = "pending";
232-
if(IsAccepted()) {
233-
sFlags = "accepted";
234-
if(fState == PENDING_FUNDS)
235-
sFlags += " waiting for enough coins in fund";
236-
else if(fState != ACCEPTED)
237-
sFlags += " waiting for end of voting period";
238-
}
239-
if(IsRejected()) {
240-
sFlags = "rejected";
241-
if(fState != REJECTED)
242-
sFlags += " waiting for end of voting period";
243-
}
244-
if(currentTime > 0 && IsExpired(currentTime)) {
245-
sFlags = "expired";
246-
if(fState != EXPIRED)
247-
sFlags += " waiting for end of voting period";
248-
}
249-
if(fState == PENDING_VOTING_PREQ) {
250-
sFlags = "expired pending voting of payment requests";
251-
}
252-
return sFlags;
253-
}
229+
std::string GetState(uint32_t currentTime) const;
254230

255231
void ToJson(UniValue& ret) const;
256232

@@ -260,9 +236,9 @@ class CProposal
260236

261237
bool IsExpired(uint32_t currentTime) const;
262238

263-
bool CanVote() const {
264-
return fState == NIL;
265-
}
239+
bool ExceededMaxVotingCycles() const;
240+
241+
bool CanVote() const;
266242

267243
bool CanRequestPayments() const {
268244
return fState == ACCEPTED;

‎src/main.cpp‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3979,6 +3979,9 @@ void CountVotes(CValidationState& state, CBlockIndex *pindexNew, bool fUndo)
39793979
auto nElapsedCycles = nCurrentCycle - nCreatedOnCycle;
39803980
auto nVotingCycles = std::min(nElapsedCycles, Params().GetConsensus().nCyclesPaymentRequestVoting + 1);
39813981

3982+
auto oldState = prequest.fState;
3983+
auto oldCycle = prequest.nVotingCycle;
3984+
39823985
if((prequest.fState == CFund::NIL || fUndo) && nVotingCycles != prequest.nVotingCycle) {
39833986
prequest.nVotingCycle = nVotingCycles;
39843987
fUpdate = true;
@@ -4020,6 +4023,10 @@ void CountVotes(CValidationState& state, CBlockIndex *pindexNew, bool fUndo)
40204023
}
40214024
}
40224025
}
4026+
4027+
if (fUndo && fUpdate && prequest.fState == oldState && prequest.fState != CFund::NIL && prequest.nVotingCycle != oldCycle)
4028+
prequest.nVotingCycle = oldCycle;
4029+
40234030
if((pindexNew->nHeight) % Params().GetConsensus().nBlocksPerVotingCycle == 0)
40244031
{
40254032
if (!vSeen.count(prequest.hash) && prequest.fState == CFund::NIL
@@ -4063,13 +4070,16 @@ void CountVotes(CValidationState& state, CBlockIndex *pindexNew, bool fUndo)
40634070
auto nElapsedCycles = nCurrentCycle - nCreatedOnCycle;
40644071
auto nVotingCycles = std::min(nElapsedCycles, Params().GetConsensus().nCyclesProposalVoting + 1);
40654072

4073+
auto oldState = proposal.fState;
4074+
auto oldCycle = proposal.nVotingCycle;
4075+
40664076
if((proposal.fState == CFund::NIL || fUndo) && nVotingCycles != proposal.nVotingCycle) {
40674077
proposal.nVotingCycle = nVotingCycles;
40684078
fUpdate = true;
40694079
}
40704080

40714081
if((pindexNew->nHeight + 1) % Params().GetConsensus().nBlocksPerVotingCycle == 0) {
4072-
if((!proposal.IsExpired(pindexNew->GetBlockTime()) && proposal.fState == CFund::EXPIRED) ||
4082+
if((!proposal.IsExpired(pindexNew->GetBlockTime()) && (proposal.fState == CFund::EXPIRED || proposal.fState == CFund::PENDING_VOTING_PREQ)) ||
40734083
(!proposal.IsRejected() && proposal.fState == CFund::REJECTED)){
40744084
proposal.fState = CFund::NIL;
40754085
proposal.blockhash = uint256();
@@ -4118,6 +4128,10 @@ void CountVotes(CValidationState& state, CBlockIndex *pindexNew, bool fUndo)
41184128
}
41194129
}
41204130
}
4131+
4132+
if (fUndo && fUpdate && proposal.fState == oldState && proposal.fState != CFund::NIL && proposal.nVotingCycle != oldCycle)
4133+
proposal.nVotingCycle = oldCycle;
4134+
41214135
if((pindexNew->nHeight) % Params().GetConsensus().nBlocksPerVotingCycle == 0)
41224136
{
41234137
if (!vSeen.count(prequest.hash) && proposal.fState == CFund::NIL){

0 commit comments

Comments
 (0)