[TierTwo] Masternode collateral auto-locking + more tests #2345
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
337dd3b to
7640ee5
Compare
1 task
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
2 times, most recently
from
ec8c075 to
14bef63
Compare
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
3 times, most recently
from
0234ece to
dbb8352
Compare
Author
|
Rebased on master. Next in line. |
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
dbb8352 to
d88265b
Compare
- check dmn list, protx creation and new payment logic
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
d88265b to
62bb2d6
Compare
furszy
reviewed
May 28, 2021
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Code review ACK 62bb2d6f581ac52e6d05e77f7f1f85839473a415, awesome testing work, we definitely need more ☕ ☕
Fuzzbawls
reviewed
May 29, 2021
Collaborator
Fuzzbawls
left a comment
Fuzzbawls
left a comment
There was a problem hiding this comment.
ACK 62bb2d6f581ac52e6d05e77f7f1f85839473a415
However, I noticed a race condition in the tiertwo_reorg_mempool.py test that required me to add some arbitrary sleeps to allow a newly generated block to finish being processed prior to checking the MN list:
Index: test/functional/tiertwo_reorg_mempool.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/functional/tiertwo_reorg_mempool.py b/test/functional/tiertwo_reorg_mempool.py
--- a/test/functional/tiertwo_reorg_mempool.py (revision 62bb2d6f581ac52e6d05e77f7f1f85839473a415)
+++ b/test/functional/tiertwo_reorg_mempool.py (date 1622194250450)
@@ -170,6 +170,7 @@
self.register_masternode(nodeB, dmn, collateral_addr)
mnsB.append(dmn)
nodeB.generate(1)
+ time.sleep(1)
self.check_mn_list_on_node(1, mnsB)
# Pick the proReg for the first MN registered on chain A, and replay it on chain B
@@ -179,6 +180,7 @@
mnsB.append(replay_mn) # same proTx hash
nodeB.sendrawtransaction(nodeA.getrawtransaction(replay_mn.proTx, False))
nodeB.generate(1)
+ time.sleep(1)
self.check_mn_list_on_node(1, mnsB)
# Now pick a proReg for another MN registered on chain A, and re-register it on chain B
@@ -188,6 +190,7 @@
self.register_masternode(nodeB, rereg_mn, collateral_addr)
mnsB.append(rereg_mn) # changed proTx hash
nodeB.generate(1)
+ time.sleep(1)
self.check_mn_list_on_node(1, mnsB)
# Register 5 more masternodes. One per block.
without that, I couldn't get the test to pass at all in a few dozen attempts
|
That is because the signal isn't being processed before calling the MN check on your machine and the retrieved MN list is from the previous chain tip. |
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
62bb2d6 to
faa87a4
Compare
As the tiertwo_reorg_mempool shows, the reorganization currently fails. This is because, during a reorg, CheckBlock is called on blocks where the prev is not connected to the chain yet. Since the budget/mn payment is checked there, we call GeListForBlock (requesting a list which has not been built yet) and so we erroneously cache an empty list. Fix this by moving IsBlockPayeeValid from CheckBlock to ConnectBlock (as, to check budget/mn payments, the previous block deterministic list must be fully built). Also, to prevent sneaky bugs like this, in the future, only cache the initial snapshot in GetListForBlock for the very last block before the enforcement of DIP3. After the enforcement block, at least the diff is always written to disk in BuildListFromBlock.
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
faa87a4 to
f6f87a4
Compare
Author
|
Yes, I agree |
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
f6f87a4 to
6594af7
Compare
Collaborator
|
The previously failing (locally) functional test is now passing, however now the |
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
6594af7 to
3ff41d2
Compare
furszy
suggested changes
May 29, 2021
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
1a43512 to
2d83495
Compare
furszy
reviewed
May 30, 2021
furszy
previously approved these changes
May 30, 2021
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
code ACK 2d83495fc75024b6aa3ca7b69d24f606a6b2262c, just found one extra line that can be removed.
Thus avoid direct calls to `UpdatedBlockTip` in the tests.
random-zebra
force-pushed
the
202103-dmn-test-fixes
branch
from
2d83495 to
4fd564c
Compare
furszy
added a commit
that referenced
this pull request
Jun 17, 2021
aacada8 [BUG] Fix mempool conflicts due to disconnected blocks (random-zebra) 91ad6eb [Tests] Functional testing for ProUpServ txes (random-zebra) e2c83dd [RPC] Implement "protx_update_service" call (random-zebra) fb2557f [Mempool] Conflict handling for ProUpServ txes (random-zebra) 281d460 [Tests] Add provider-update-service to dip3_protx unit-test (random-zebra) e83e454 [TierTwo] Connect ProUpServ payload management in dmn manager (random-zebra) a197fce [Tests] Add Get/Set payload unit-test for ProUpServ txes (random-zebra) 1f5f509 [Refactoring] Prepare mempool code for multiple payload types (random-zebra) a83d87e [Core] Introduce ProUpServ payload and transaction type (random-zebra) Pull request description: Exctracted from #2267 This PR: - introduces the `PROUPSERV` payload and transaction type. This special transaction is submitted by the operator (the payload must be signed with the operator key) to update the IP/port fields. If the original ProReg transaction set a non-zero operatorReward, this payload can also be used to update the operator's payout address. - adds the new RPC `protx_update_service` to create ProUpServ transactions - adds unit and functional testing Builds on top of: - [x] #2345 ACKs for top commit: furszy: utACK aacada8 after rebase. Fuzzbawls: Code ACK aacada8 Tree-SHA512: 6a642dfa5c7d239de23adaacd2cba56621f4392b2d34a436e023df6452cf9d39c55fb58c62ade2705db4f55f47674fc936644d731e9f8eb526d6679815faa864
random-zebra
added a commit
to random-zebra/PIVX
that referenced
this pull request
Jun 19, 2021
Partial backport of f6aa6ad (PIVX-Project#2345): only moving IsBlockPayeeValid
random-zebra
added a commit
that referenced
this pull request
Jun 21, 2021
4368613 [BUG] Check masternode/budget payments during block connection (random-zebra) 0374dbe [QA] Add tiertwo_governance_reorg functional test (random-zebra) Pull request description: First commit backports the functional test of #2436. Second commit extracts from f6aa6ad (#2345) the fix to make it pass. ACKs for top commit: Fuzzbawls: Code ACK 4368613 Tree-SHA512: d12cbcc59719c5a5328c04c567cfa387d98ef00565e21a2bd37a3de3d85cead7120267b04b8c121d0489be6d5c7cf726337228e4123048d1f285a2c35b826651
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another one coming from #2267
This PR introduces a "auto-locking" feature for masternode collaterals:
at startup, the wallet is scanned, and any masternode collateral coin found gets locked. Can be disabled setting
-mnconflock=0(although, there is no need for amasternode.confanymore, so we might want to consider defining and using a new flag here, and removingmnconflockafter 6.0).during runtime, when a ProRegTx is processed by
CWallet::AddToWalletIfInvolvingMe, the wallet checks for ownership of the collateral, and automatically locks it in case.Here, we also add two more functional tests (
tiertwo_deterministicmns.py,tiertwo_reorg_mempool.py), update the other tests, checking the auto-locking feature, and fix a couple bugs:Missing special tx processing in RollforwardBlock
Validity of tiertwo payments can (and must) be verified only during block connection, as we keep the deterministic state only for the active chain, not for all possible chaintips.
Builds on top of: