CFundDB extra log and ensure read before modify #622

aguycalled · 2019-11-08T09:33:10Z

This PR adds extra log for all the modifications of the CFundDB and ensures entries are read in the memory cache before being modified

mxaddict · 2019-11-08T10:00:40Z

utACK

chasingkirkjufell · 2019-11-08T15:14:24Z

if a node submits a payment request and that transaction is orphaned. the payment request will still be in the qt wallet in that node.
Edit: Same with proposals, if the transaction is in a orphan block, it'll still be in the "proposalvotelist".

aguycalled · 2019-11-08T15:32:18Z

@chasingkirkjufell fixed in 96e9d51

aguycalled · 2019-11-11T17:14:17Z

added test unit 5f43066

aguycalled · 2019-11-11T17:42:52Z

test unit cfunddb_tests.cpp

fails on master:

$ git name-rev --name-only HEAD
remotes/upstream/master
$ ./src/test/test_navcoin 
Running 100 test cases...
test/cfunddb_tests.cpp:41: error: in "coins_tests/cfunddb_state": check !view.GetProposal(emptyProposal.hash, proposal) has failed
test/cfunddb_tests.cpp:45: error: in "coins_tests/cfunddb_state": check mapProposals.size()==0 has failed
test/cfunddb_tests.cpp:55: error: in "coins_tests/cfunddb_state": check !view.GetPaymentRequest(emptyPaymentRequest.hash, prequest) has failed
test/cfunddb_tests.cpp:59: error: in "coins_tests/cfunddb_state": check mapProposals.size()==0 has failed
test/cfunddb_tests.cpp:61: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==0 has failed
test/cfunddb_tests.cpp:120: error: in "coins_tests/cfunddb_state": check mapProposals.size()==0 has failed
test/cfunddb_tests.cpp:122: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==0 has failed
test/cfunddb_tests.cpp:137: error: in "coins_tests/cfunddb_state": check mapProposals.size()==1 has failed
test/cfunddb_tests.cpp:139: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==1 has failed
test/cfunddb_tests.cpp:162: error: in "coins_tests/cfunddb_state": check mapProposals.size()==2 has failed
test/cfunddb_tests.cpp:164: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==1 has failed
test/cfunddb_tests.cpp:202: error: in "coins_tests/cfunddb_state": check !view.GetProposal(hash2, proposal2) has failed
test/cfunddb_tests.cpp:204: error: in "coins_tests/cfunddb_state": check !view.GetPaymentRequest(hash3, prequest) has failed
test/cfunddb_tests.cpp:207: error: in "coins_tests/cfunddb_state": check mapProposals.size()==1 has failed
test/cfunddb_tests.cpp:209: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==0 has failed
test/cfunddb_tests.cpp:229: error: in "coins_tests/cfunddb_state": check !base->GetProposal(hash2, proposal2) has failed
test/cfunddb_tests.cpp:231: error: in "coins_tests/cfunddb_state": check !base->GetPaymentRequest(hash3, prequest) has failed
test/cfunddb_tests.cpp:250: error: in "coins_tests/cfunddb_state": check !base->GetProposal(hash2, proposal2) has failed
test/cfunddb_tests.cpp:252: error: in "coins_tests/cfunddb_state": check !base->GetPaymentRequest(hash3, prequest) has failed
test/cfunddb_tests.cpp:255: error: in "coins_tests/cfunddb_state": check mapProposals.size()==2 has failed
test/cfunddb_tests.cpp:257: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==1 has failed
test/cfunddb_tests.cpp:278: error: in "coins_tests/cfunddb_state": check mapProposals.size()==2 has failed
test/cfunddb_tests.cpp:280: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==1 has failed
test/cfunddb_tests.cpp:384: error: in "coins_tests/cfunddb_state": check mapProposals.size()==2 has failed
test/cfunddb_tests.cpp:387: error: in "coins_tests/cfunddb_state": check mapPaymentRequests.size()==1 has failed

*** 25 failures are detected in the test module "NavCoin Test Suite"
$

passes on this branch:

$ git name-rev --name-only HEAD
cfund-log-modifiers
$ ./src/test/test_navcoin 
Running 100 test cases...

*** No errors detected
$

mxaddict · 2019-11-12T10:32:41Z

utACK

new native testcases make sense.

Ran the test_navcoin binary from this branch against master and master failed.

Ran the test_navcoin binary from this branch against this pr and it passed.

I'll approve as soon as I'm done with the manual test scenario that I used to replicate the 000000000 hash request.

mxaddict · 2019-11-12T10:40:15Z

Tested with this manual scenario:

Node 1 has block A with payment request A
Node 2 has block B with payment request A

Node 2 has more blocks so Node 1 tries to reorg against the blocks in Node 2
Node 1 does syncs blocks from Node 2
Node 1 no longer has block A which in it's copy of payment request A is referred to
Node 1 checks block A, it can't find it, so it thinks that Node 2 is lying
Node 1 bans Node 2

On master I was able to get node 1 to fork, still testing on this PR.

aguycalled · 2019-11-12T10:41:36Z

Tested with this manual scenario:

Node 1 has block A with payment request A
Node 2 has block B with payment request A

Node 2 has more blocks so Node 1 tries to reorg against the blocks in Node 2
Node 1 does syncs blocks from Node 2
Node 1 no longer has block A which in it's copy of payment request A is referred to
Node 1 checks block A, it can't find it, so it thinks that Node 2 is lying
Node 1 bans Node 2

On master I was able to get node 1 to fork, still testing on this PR.

To completely reproduce the issue we've seen we need to bring it further to the payment request's payout.

mxaddict · 2019-11-12T13:44:57Z

Tested with this manual scenario:
Node 1 has block A with payment request A
Node 2 has block B with payment request A

Node 2 has more blocks so Node 1 tries to reorg against the blocks in Node 2
Node 1 does syncs blocks from Node 2
Node 1 no longer has block A which in it's copy of payment request A is referred to
Node 1 checks block A, it can't find it, so it thinks that Node 2 is lying
Node 1 bans Node 2
On master I was able to get node 1 to fork, still testing on this PR.
To completely reproduce the issue we've seen we need to bring it further to the payment request's payout.

I think that might be covered in my test, cause I let Node 2 stake up to 300+ more blocks than node 1. Which should be more than enough for the payment request to be paid already

@aguycalled can you confirm this is the case with my test scenario?

aguycalled · 2019-11-12T14:56:12Z

@mxaddict that is one possible wrong case.
the one we saw on mainnet had the block of payment request A in Node 1 set as 0x000 after the reorg
we need to be sure the blockhash is set correctly to the right block after reorgs

aguycalled · 2019-11-12T18:39:08Z

test from c27635f passes on this branch and 4.7.1. But does not pass in 4.7.0 c8d9d72.

My theory:

Bootstrap downloaded from https://www.navexplorer.com/bootstrap.tar (node was running 4.7.0)
Client runs with -txindex=1 -spendindex=1 -addressindex=1
Best block on launch: 3259a848b73c2fd215382b74ac923536889c89bf45c87059e60ae689401ee18b - height 3582947
Let it sync with mainnet, it will reject the block 53c79cd433465b163dc760d3239f95edf0504d8b2a9ecc3cf76def8a77f7eddb - height 3628851
That block contains a payout for payment request bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c
The votes of this payment request have not been counted in previous blocks, so it never reaches the accepted state and that’s why the payout is rejected. We've seen this issue in 4.7.1 nodes too.
Querying the payment request status on launch with the navexplorer bootstrap we can see:

$ ./src/navcoin-cli -datadir=/Users/alex/bootstrap getpaymentrequest bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c
{
  "version": 2,
  "hash": "bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c",
  "blockHash": "9721c16edcacb11e83c69b873d9d67949b938a934ffad2f270fd0a80f0cade13",
  "description": "NavCoin Portuguese-CriptoBlock 2019PT-http://bit.ly/2o6pKRD",
  "requestedAmount": "2250.00",
  "votesYes": 0,
  "votesNo": 0,
  "votingCycle": 2,
  "status": "pending",
  "state": 0,
  "stateChangedOnBlock": "0000000000000000000000000000000000000000000000000000000000000000"
}

9721c16edcacb11e83c69b873d9d67949b938a934ffad2f270fd0a80f0cade13 is not part of the main chain, that’s the reason why the votes are not counted.
The payment request was created on 2019-10-08, so if it was affected by a reorg, we can be sure it happened with a 4.7.0 wallet and the entry is corrupted since then on the local cfunddb.
Payouts are the only consensus-critical action of payment requests. It makes sense wallets with that corrupted entry did not fork until the payout.
When 4.7.1 was released, nodes which were not aware of being suffering any issue just updated and inherited the corrupted entry from 4.7.0.
4.7.1 nodes would have needed to reindex to be completely sure their state was valid.

mxaddict · 2019-11-12T18:43:35Z

@aguycalled I agree with your idea to add a new hash to the merkle root, should we add it into this PR?

mxaddict · 2019-11-12T18:44:22Z

@aguycalled I agree with your idea to add a new hash to the merkle root, should we add it into this PR?

Nevermind this comment :D

aguycalled · 2019-11-12T22:11:46Z

Related to previous comments: #625

proletesseract · 2019-11-15T02:35:23Z

This PR compiles and runs on OSX 10.14.5.

I can verify that 4.7.0 fails the test cfund-fork-reorg.py on line 92 when it tries to reconnect the nodes with raise AssertionError("Block sync failed").

This branch (which is forked from master after 4.7.1) passes the test.

Now continuing with the code & test review.

proletesseract · 2019-11-16T03:22:28Z

Test makes sense, thanks for leaving comments in the file.

Regarding these changes, as i understand it we have a few areas with the potential to cause a fork if they were unable to be reorganised correctly;

As far as I can tell the test only reorgs a payment request, after the proposal is accepted on the same chain by each node. Is it also worth having the same test but reorg the proposal itself and ensure payment requests can still be submitted?

We have; cfund-paymentrequest-state-reorg.py i can see also but that also appears to be focused on payment request reorganisations and actually only checks the bestblockhashes after reorganisation occurs and does not confirm the cfund state.

These and probably more permutations are the types of things i want to map out in miniature and make sure are covered in a robust and micro focused python test framework. After we get the new testnet up and running.

I think this test is probably enough for now, and we can leave the full audit of the cfund tests for that stage of the network stability review.

proletesseract · 2019-11-16T08:25:11Z

src/coins.cpp

+            mapProposal.insert(make_pair(it->first, it->second));
+
+    for (auto it = mapProposal.begin(); it != mapProposal.end();)
+        it->second.IsNull() ? mapProposal.erase(it++) : ++it;


Is it necessary to erase entries with the null proposal pair since the loop above only inserts when it is not null?

yes, because it can have null entries from cacheProposals, which are entries marked to be removed in the base view when a flush is executed later

proletesseract · 2019-11-16T08:26:42Z

src/coins.cpp

+            mapPaymentRequests.insert(make_pair(it->first, it->second));
+
+    for (auto it = mapPaymentRequests.begin(); it != mapPaymentRequests.end();)
+        it->second.IsNull() ? mapPaymentRequests.erase(it++) : ++it;


same comment as above for here, if my above comment is valid.

same answer as before

proletesseract · 2019-11-16T08:45:31Z

src/coins.cpp

@@ -221,14 +229,22 @@ CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) {
 CProposalModifier CCoinsViewCache::ModifyProposal(const uint256 &pid) {
    assert(!hasModifier);
    std::pair<CProposalMap::iterator, bool> ret = cacheProposals.insert(std::make_pair(pid, CProposal()));
-    ret.first->second.fDirty = true;
+    if (ret.second) {


Can you explain what this function is doing sorry? I can see it's called before adding votes to proposals in the cache and.. again when we're updating the state at the end of the voting cycles.

::ModifyProposal returns a pointer to an entry in the view cache we want to modify.

std::pair<CProposalMap::iterator, bool> ret = cacheProposals.insert(std::make_pair(pid, CProposal()));

Here we try to insert in the view cache an empty proposal with the hash pid.

if (ret.second) {

This means the insert was successful (no previous entry with that hash in the cache).

if (!base->GetProposal(pid, ret.first->second)) { ret.first->second.SetNull(); }

We try to get the entry from the base view, and if it's not possible we set it to null (which is redundant but safer).

return CProposalModifier(*this, ret.first);

Finally we construct the CProposalModifier object and return it.

proletesseract · 2019-11-16T08:47:54Z

src/consensus/cfund.cpp

            CProposalModifier proposal = view.ModifyProposal(it->first);
            proposal->nVotesYes = it->second.first;
            proposal->nVotesNo = it->second.second;
+            if (*proposal != oldproposal)
+            {
+                proposal->fDirty = true;


what is the fDirty boolean used for?

Only entries flagged as dirty are written to the base view when flushed.

proletesseract · 2019-11-16T08:51:56Z

src/consensus/cfund.cpp

@@ -815,6 +847,13 @@ void CFund::CFundStep(const CValidationState& state, CBlockIndex *pindexNew, con
                    prequest->nVotesNo = 0;
                }
            }
+
+            if (*prequest != oldprequest)


does the == operator overload match != as well for these if statements?

There is an operator!= declaration which simply returns the inverse of ==, is that what you are referring to?

proletesseract · 2019-11-16T08:59:31Z

src/main.cpp

@@ -3425,13 +3475,15 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
            {
                uint256 prid = uint256S(metadata[nPaymentRequestsCount].get_str());

-                if(!view.HavePaymentRequest(prid))
+                CPaymentRequest prequest;


What's the benefit of doing the following operations on the cache prequest rather than the CPaymentRequestModifier? We now guarantee that we've done all the null checks on the payment request before running these block checks?

Is there somewhere in main where we should be doing a similar thing with the cache for proposals like we're doing for payment requests here?

There's no real benefit more than being more strictly correct with the purpose of each object.
CProposal and CPaymentRequest changes are not reflected in the cache view, while Modifiers can be changed and the changes are reflected in the cache view.
The declaration of mprequest could be moved forward, so it only happens if all the previous checks are satisfied. 36269a2
CProposalModifier (3 times) and CPaymentRequestModifier (4 times) are used very rarely in the code. A global search would show where one can put some more attention.

proletesseract · 2019-11-16T09:02:36Z

Diff reviewed in full while exploring some horizontal and vertical context around the changes. Comments above.

navbuilder · 2019-11-16T19:18:35Z

A new build of 36269a2 has completed succesfully!
Binaries available at https://build.nav.community/binaries/cfund-log-modifiers

proletesseract

changing approval status until discussed test is added

re-reviewing with test

proletesseract · 2019-11-16T23:48:26Z

I've added the test for checking a re-organised proposal can still make it through payment request submission, voting and payment. The test passes on this branch and fails on v4.7.0.

the 4.7.0 test failure is on line 96
the 4.7.0 error is JSONRPC error: Block not found

Which confirms this action would end with the nodes disagreeing on the blockHash of the proposal.

If someone can review the test that would be great; https://github.com/navcoin/navcoin-core/blob/09b636b3954b1d7374156339da10f6bb1ad91c61/qa/rpc-tests/cfund-fork-reorg-proposal.py

navbuilder · 2019-11-17T01:33:27Z

A new build of b0af91b has completed succesfully!
Binaries available at https://build.nav.community/binaries/cfund-log-modifiers

mxaddict · 2019-11-18T17:16:58Z

Build on Ubuntu 19.10 and ran the 2 new tests for cfund-fork-reorg*, tests passed

mxaddict · 2019-11-18T17:17:18Z

Read the new test that @proletesseract added, makes sense.

mxaddict · 2019-11-18T17:18:09Z

@aguycalled @proletesseract I'll let you do the honors incase you want to add more changes to the tests.

* add extra log * add __func__ * ensure read before modify * fix log * optimize log * do not access modifier * only set dirty when necessary * check for nullified * add extra log * do not insert nullified entries * HaveProposalInCache/HavePaymentRequestInCache * add cfunddb_tests.cpp * add 250 rounds and random remove * Added new test for cfund reorg scenario * Updates to the test as per aguycalled's suggestions * update qa/rpc-tests/cfund-fork-reorg.py * Added new test to the suite * move mprequest * adding (failing) test for proposal reorg * removed 5th cycle * fixed preq voting, removed logs added final payout check

add extra log

a6423a5

aguycalled requested review from chasingkirkjufell, mxaddict and proletesseract November 8, 2019 09:33

add __func__

c82b154

ensure read before modify

ea3f0a7

aguycalled changed the title ~~CFundDB extra log~~ CFundDB extra log and ensure read before modify Nov 8, 2019

alex v added 4 commits November 8, 2019 12:13

fix log

072fbd9

optimize log

d581945

do not access modifier

d537032

only set dirty when necessary

9551b52

check for nullified

96e9d51

alex v added 4 commits November 10, 2019 20:49

add extra log

eb2ab05

do not insert nullified entries

8a26d80

HaveProposalInCache/HavePaymentRequestInCache

406614e

add cfunddb_tests.cpp

5f43066

add 250 rounds and random remove

23687e5

Added new test for cfund reorg scenario

6636ba1

mxaddict and others added 2 commits November 13, 2019 01:21

Updates to the test as per aguycalled's suggestions

0c65aaa

update qa/rpc-tests/cfund-fork-reorg.py

a05f61d

merge qa/rpc-tests/cfund-fork-reorg.py

c27635f

Added new test to the suite

8d79712

aguycalled added the ready for review label Nov 13, 2019

proletesseract reviewed Nov 16, 2019

View reviewed changes

move mprequest

36269a2

proletesseract previously approved these changes Nov 16, 2019

View reviewed changes

proletesseract reviewed Nov 16, 2019

View reviewed changes

proletesseract self-requested a review November 16, 2019 22:32

proletesseract added 3 commits November 17, 2019 12:12

adding (failing) test for proposal reorg

09b636b

removed 5th cycle

4976387

fixed preq voting, removed logs added final payout check

b0af91b

proletesseract approved these changes Nov 16, 2019

View reviewed changes

mxaddict approved these changes Nov 18, 2019

View reviewed changes

aguycalled merged commit 37fa72e into navcoin:master Nov 19, 2019

proletesseract mentioned this pull request Dec 13, 2019

v4.7.2-rc #649

Merged

CFundDB extra log and ensure read before modify #622

CFundDB extra log and ensure read before modify #622

Conversation

aguycalled commented Nov 8, 2019 • edited Loading

mxaddict commented Nov 8, 2019

chasingkirkjufell commented Nov 8, 2019 • edited Loading

aguycalled commented Nov 8, 2019

aguycalled commented Nov 11, 2019 • edited Loading

aguycalled commented Nov 11, 2019

mxaddict commented Nov 12, 2019

mxaddict commented Nov 12, 2019

aguycalled commented Nov 12, 2019

mxaddict commented Nov 12, 2019

aguycalled commented Nov 12, 2019 • edited Loading

aguycalled commented Nov 12, 2019

mxaddict commented Nov 12, 2019

mxaddict commented Nov 12, 2019

aguycalled commented Nov 12, 2019

proletesseract commented Nov 15, 2019

proletesseract commented Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguycalled Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

proletesseract commented Nov 16, 2019 • edited Loading

navbuilder commented Nov 16, 2019

proletesseract left a comment

Choose a reason for hiding this comment

proletesseract commented Nov 16, 2019 • edited Loading

navbuilder commented Nov 17, 2019

mxaddict commented Nov 18, 2019

mxaddict commented Nov 18, 2019

mxaddict commented Nov 18, 2019

aguycalled commented Nov 8, 2019 •

edited

Loading

chasingkirkjufell commented Nov 8, 2019 •

edited

Loading

aguycalled commented Nov 11, 2019 •

edited

Loading

aguycalled commented Nov 12, 2019 •

edited

Loading

proletesseract commented Nov 16, 2019 •

edited

Loading

aguycalled Nov 16, 2019 •

edited

Loading

proletesseract commented Nov 16, 2019 •

edited

Loading

proletesseract commented Nov 16, 2019 •

edited

Loading