[05:58:23] @aguycalled I see that the test is run in the native test suite [05:58:39] I don't have much experience with that kind of test [05:58:53] But I'll do my best to understand/review [07:20:48] neither, but i do think a combination of functional tests and end to end (rpc) tests are going to give us quite high quality coverage. I'm keen to check it out and try my hand at writing some 🙂 [11:24:24] great, if u can confirm test passes on this branch and fails on master on ur systems thats already good [11:25:25] Yes, I can confirm that is passed on my build [11:26:03] And fails on any branch without the patches from 622 (I tried on master + my GUI PR) [11:26:31] 👌 [11:29:42] from my point of view we should release a new client with this patch and the restricted creation of proposals/requests [11:30:28] then focus on stress testing [11:38:50] I agree [15:48:53] @aguycalled I'm just about done with the RPC test that runs my reorg scenario [15:50:02] let me know and i ll have a look [15:50:38] Yeah, I'm not entirely sure if it actually works, I'm still testing it agains the master branch binary [15:50:43] And against the binary from the PR [15:50:53] In theory it should fail everytime on master based bin [15:51:05] And pass everytime on the navcoind bin from PR [15:53:09] in your test scenario there's a part that is not completely accurate: there are a few things here that can be a bit confusing [15:55:08] @aguycalled I've pushed the test [15:55:14] Can you take a look? [15:55:27] I can't seem to get it to fail on 4.7.0 [15:55:31] It still passes [15:55:38] I'm not sure what I'm doing wrong in the test [15:56:47] yes ill have a look now [15:58:36] im going to test it myself but i would say if you do assert(self.nodes[0].getpaymentrequest(hash) == self.nodes[1].getpaymentrequest(hash)) it would fail [15:59:11] Hmmm, I'm getting off my laptop for now [15:59:20] Heading home, I'll try that when I get back online [15:59:31] im bulding master to try [15:59:33] In the meantime, can you maybe test that? [15:59:36] Alright, nice [15:59:37] yes [15:59:38] TTYL [16:17:57] it's passing on master [16:34:25] Yeah, same happened with my test against 4.7.0 [16:34:49] So I'm not sure why though. [16:34:58] well, the reorg was already fixed in 4.7.1 [16:35:27] the pr ive opened fixed something different i was only able to see with the bootstrap from navexplorer [16:35:28] As the block for the payment request should be orphaned on node 0. [16:36:11] were you testing against master and / or 4.7.0? [16:36:29] or by 4.7.0 you mean master/4.7.1? [16:36:54] I tested with 4.7.0 and with your new PR [16:37:20] Hmmm, wait, maybe that had the reorg fix in that 4.7.0 bin [16:37:37] I'll retest with a fresh build of 4.7.0 [16:37:57] With 4.7.0 this teat should fail right? And with 4.7.1 it should pass? [16:38:28] yes iirc [16:38:30] theres also a related test [16:38:36] What kind of scenario do you think would emulate the block hash 0x000? [16:39:01] CFundPaymentRequestStateReorg [16:39:36] I was under the impression that the scenario i tested would make the block orphaned which would cause the block hash to be 0x000 [16:39:52] i was also under that impression [16:39:59] Hmmm [16:40:14] I'm more confused than when I started 🤣 [16:40:38] I might have a better go at this tomorrow after some sleep. [16:40:45] the only way i know to replicate the issue is using the bootstrap from navexplorer [16:41:58] Hmmm, then why did some users fork? If it was caused by that bootstrap, then in theory only navexplorer + binance node should have forked right? [16:42:24] im thinking to try loading a chain with an already existing payment request [16:43:17] Hmmm, i wonder if it's possible that it only happens if the node is shutdown and restarted... [16:43:44] I've seen one of my nodes fork like this before. With the block 0x000 [16:43:57] that's what i'm thinking [16:44:15] I'll see if I can integrate it into the test. [16:44:17] that should be incorporated to our stress testings [18:01:39] so the description of what i see happenning when using the navexplorer bootstrap is [18:01:41] Bootstrap downloaded from https://www.navexplorer.com/bootstrap.tar Client runs with -txindex=1 -spendindex=1 -addressindex=1 Best block on launch: 3259a848b73c2fd215382b74ac923536889c89bf45c87059e60ae689401ee18b - 3582947 Rejects block 53c79cd433465b163dc760d3239f95edf0504d8b2a9ecc3cf76def8a77f7eddb - 3628851 Block contains a payout for payment request bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c The [18:01:42] 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. [18:03:43] @aguycalled does this same scenario happen without the indexes? [18:03:48] Or only with those ticked on? [18:04:01] im seeing this extending a bit your test [18:04:07] so no indexes are needed [18:04:42] Not sure what you mean by m seeing this extending a bit your test [18:04:48] https://pastebin.com/VXPJRNq5 [18:04:57] the payment request is never accepted [18:05:03] the votes are not counted [18:05:08] if it was part of a reorg [18:05:25] this is using master branch [18:05:55] Assertion failed: pending != accepted File "/Users/alex/navcoin-core/qa/rpc-tests/test_framework/test_framework.py", line 149, in main self.run_test() File "qa/rpc-tests/cfund-fork-reorg.py", line 102, in run_test assert_equal(self.nodes[0].getpaymentrequest(paymentHash0)["status"], "accepted") File "/Users/alex/navcoin-core/qa/rpc-tests/test_framework/util.py", line 507, in assert_equal raise AssertionError("%s [18:05:55] != %s"%(str(thing1),str(thing2))) Stopping nodes Not cleaning up dir /var/folders/qh/7_l8lmqx2sb5ncxgtzzyb51r0000gn/T/testkjnjejds/96149 Failed [18:06:07] {'version': 2, 'hash': 'f38c660d2eab3806c27c670147d6b75f8c6dd354d9b238ac5c4c4b9abcdaa124', 'blockHash': '69666186c2f67c6f4089a56df7c011aecc57ba9d1dcdd806c57015dd402a9f04', 'description': 'test', 'requestedAmount': '10000.00', 'votesYes': 0, 'votesNo': 0, 'votingCycle': 1, 'status': 'pending', 'state': 0, 'stateChangedOnBlock': '0000000000000000000000000000000000000000000000000000000000000000'} [18:06:12] i will try now with my pr [18:06:22] Nice [18:06:27] I'll test as well [18:06:50] Should I commit the changes if it fails on master and passes on PR [18:07:21] id say yes [18:07:50] Alright [18:08:12] I just ran on 4.7.1 bin: Initializing test directory /tmp/testdmsl24mk/10776 Assertion failed: pending != accepted File "/home/mxaddict/Projects/mxaddict/navcoin-temp/qa/rpc-tests/test_framework/test_framework.py", line 149, in main self.run_test() File "qa/rpc-tests/cfund-fork-reorg.py", line 99, in run_test assert_equal(self.nodes[0].getpaymentrequest(paymentHash0)["status"], "accepted") File [18:08:13] "/home/mxaddict/Projects/mxaddict/navcoin-temp/qa/rpc-tests/test_framework/util.py", line 507, in assert_equal raise AssertionError("%s != %s"%(str(thing1),str(thing2))) Stopping nodes [18:08:46] im building the pr again now [18:09:47] I just ran against the PR bin [18:09:49] Same failure [18:11:08] Initializing test directory /tmp/test0ng7vka3/11390 Assertion failed: pending != accepted File "/home/mxaddict/Projects/mxaddict/navcoin-temp/qa/rpc-tests/test_framework/test_framework.py", line 149, in main self.run_test() File "qa/rpc-tests/cfund-fork-reorg.py", line 99, in run_test assert_equal(self.nodes[0].getpaymentrequest(paymentHash0)["status"], "accepted") File [18:11:09] "/home/mxaddict/Projects/mxaddict/navcoin-temp/qa/rpc-tests/test_framework/util.py", line 507, in assert_equal raise AssertionError("%s != %s"%(str(thing1),str(thing2))) Stopping nodes [18:12:23] theres a typo on line 69 on the paste ive done [18:12:34] it should be node1 who mines the blocks [18:12:42] Alright [18:12:43] $ qa/rpc-tests/cfund-fork-reorg.py Initializing test directory /var/folders/qh/7_l8lmqx2sb5ncxgtzzyb51r0000gn/T/testaa61upbb/2442 Stopping nodes Cleaning up Tests successful [18:12:47] it passes for me with the pr build [18:12:49] I'll test again [18:18:57] It passes for me on the PR [18:18:57] But it also passed on the old bin [18:19:09] I made the change on like 69 [18:22:41] I've pushed the new test anyway, I think it makes more sense and asserts more details about the blocks [18:31:17] i keep investigating [18:31:39] Alright, I'm gonna hit the sack [18:32:11] I need some sleep Caffeine is no longer working 😄 [19:07:40] sure talk later [19:07:46] this is what else i've found [19:07:46] Querying the payment request status on launch with the navexplorer bootstrap: $ ./src/navcoin-cli -datadir=/Users/alex/bootstrap getpaymentrequest bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c { "version": 2, "hash": "bc6f31a269a9733be2dc8e2d1cfd1102bd569c736346139933755e7bab5f8e9c", "blockHash": "9721c16edcacb11e83c69b873d9d67949b938a934ffad2f270fd0a80f0cade13", "description": "NavCoin [19:07:47] 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 right chain, that’s the reason why the votes are not counted The payment request was created [19:07:47] on 2019-10-08, so if it was affected by a reorg, 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. [19:08:28] when 4.7.1 was released, they just updated and inherited the corrupted entry from 4.7.0 [19:16:58] 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. [19:17:01] that's my theory [19:32:56] Makes sense [19:33:01] I actually found that my home PC [19:33:10] Was affected by this [19:33:11] I updated the wallet [19:33:15] Then started the client [19:33:26] But it forked (Unlike all my other nodes) [19:33:53] Well, not really forked, but it stopped accepting new blocks from valid nodes [19:33:53] i think we should introduce a commit to the merkle root hash of all the cfund db entries as a coinbase output [19:34:29] if a node detects that more than 5 of the last 10 blocks commit to a different merkle root, then it should show a warning to the user asking to reindex [19:34:56] i've confirmed the test fails on 4.7.0 [19:36:46] I see, so the issue was solved already in 4.7.1 with the patch that we merged [19:36:56] It's just that the nodes were already affected before upgrading [19:37:01] Hence the fork. [19:37:10] that's my theory [19:37:41] Ok, I just now understand your idea about adding the hash to the merkle root [19:37:45] And I agree with it. [19:38:28] So should I add this test to the suite? [19:38:36] yup [19:38:39] I only added the test, but did not add it to the test suite. [19:38:40] Alright [19:39:35] ill work now on the merkle root hash idea and open a new pr [19:44:10] Alright [23:04:24] https://github.com/navcoin/navcoin-core/pull/625 [23:04:28] @prole @mxaddict [23:49:35] utACK