-
Notifications
You must be signed in to change notification settings - Fork 725
[Test] Introducing tier two functional test suite + several fixes #1916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Test] Introducing tier two functional test suite + several fixes #1916
Conversation
Fuzzbawls
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, Concept ACK, but left comments where I think improvements and/or optimizations/clarifications could be made.
I also think that there may be no need to customize the regtest's initialize_datadir function as you can set multiple custom config params (per-node) already using the existing self.extra_args array-of-arrays.
b14f633 to
8228a9e
Compare
|
just updated it with more changes. Decreased even more the test time from 4:22 to 3:21 minutes. Updated the PR description time as well. This is still under WIP, will try to do the early feedback changes in the upcoming days 👍 . This is not priority to get merged anyway, Sapling PRs come first. |
JSKitty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo nits; everything else looks great!
… masternode manager dd67066 [Test] Add feature_blockhashcache.py (random-zebra) 4031fd2 [Refactor] Remove some more chainActive accesses and cs_main locks (random-zebra) fe1d84e [Validation] Verify MN pings with cached block hashes (random-zebra) 14d3c32 [Masternode] Use cached block hashes to create mn pings (random-zebra) f5349b9 [Cleanup] Remove global map mapCacheBlockHashes (random-zebra) 3cdb0a0 [Cleanup] Remove GetBlockHash in masternode.h/.cpp (random-zebra) 4d41910 [Masternode] Use cached block hashes to calculate the score (random-zebra) 616039e [Validation] Cycle-back when uncaching block hashes (random-zebra) 32617a7 [RPC] Add getcachedblockhashes function (random-zebra) 0de6544 [Init] Load block hashes cache at startup (random-zebra) 2237a69 [Refactor] Cache last 200 block hashes in Masternode Manager (random-zebra) ddf63fe [Refactor] Introduce CyclingVector class (random-zebra) 2c60025 [Cleanup] Remove not needed calls to masternode.cpp GetBlockHash (random-zebra) aa28000 [Refactor] Cache tip height in masternode manager (random-zebra) Pull request description: **Problem**: Validation of many objects on the second layer (masternode pings, winners/scores, etc...) requires access to chainActive to get the hashes of blocks at a certain height. This is done very often (especially for masternode pings) and means holding `cs_main` lock every time, making the process slow and possibly leading to locking order inconsistencies. **Solution**: Cache the last N block hashes in the masternode manager. In order to implement this efficiently, we introduce a new data structure called `CyclingVector`, which is a fixed-size vector, exposing only a setter `Set(i, value)` which modifies the index `i % N`. The cache is initialized at startup and updated when blocks get connected to / disconnected from the tip. The size (`CACHED_BLOCK_HASHES`) is currently set a default value of 200 making it possible to validate all masternode pings (but not all masternode winners) without accessing chainActive. In the future we could add a startup flag to let the user customize this (higher cache size would, as usual, mean faster execution but higher memory usage). An RPC `getcachedblockhashes` is introduced, in order to print the content of the cache. This can be used later in the functional test suite for better testing. This PR also removes the accesses to chainActive to get the current chain-height (using a value cached in the manager). ACKs for top commit: furszy: tested with #1916 and have run it for a while in mainnet, all looking good. Really nice, ACK dd67066 . Fuzzbawls: ACK dd67066 Tree-SHA512: 3ba56ddbf9fb37c96c1298773f1008f70ca8d0b9aac25754884805e96c7bd104f4e0c2403e49d5502117b6764c319ddd25c70bb53095b009855894d50531c056
9e119b6 to
a40048e
Compare
a40048e to
c89b00c
Compare
|
Updated per feedback plus added a basic budget finalization payment test coverage as well.
|
8814444 to
f5fa73c
Compare
random-zebra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good (modulo few nits).
Have some fixes and improvements for the python suite:
- add an RPC to send the ping on demand
- abstract the functions in a new specific
PivxTier2TestFrameworkbased on mocked time - refactor
masternode_activationandmasternode_sync_basictests with the new framework - add a separate test,
masternode_ping, to only check the masternode ping thread real-time (with sleep)
Can either push them here, or open a new PR on top of this one.
|
awesome! 😄 go ahead!, either way is fine for me. |
c28a06d to
637172a
Compare
furszy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the new commits are looking great 👌 , have done a quick pass through each of them.
Have left few minor comments for us to check for when travis gets happy with this PR.
7e95d00 to
f99557f
Compare
dc5c14a to
7cfcd55
Compare
…in mainnet/testnet. Plus, not updating asset when the received asset isn't what we are looking for.
…ock on every fail sync attempt.
…c process to the next step if the sporks are received before the tierTwoMan requested them (messageHandler requested them)
…block hash) to be used on CMasternodeSync::IsBlockchainSynced() and not lock cs_main there.
…nt tx broadcast wasn't included un the block
….erase return object.
…ppens when the controller needs to broadcast a new start message, the remote masternode was rejecting it because thinks that already has it enabled (when the MN is in un-capable state and it was kicked from the network list), loosing the reactivation opportunity and ending in a forever non-capable state.
64c92c7 to
dacfebe
Compare
|
Updated per feedback, plus rebased it on master. Note: added a commit dacfebe that is fixing a inv sync issue in master, introduced in the swiftx removal. I haven't pushed it in another PR because, as we have the new tier two test suite on this PR, it's fully verifiable here. Without it, the tier two sync simply doesn't work. |
dacfebe to
2b5fb9b
Compare
random-zebra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some great catches near the end.
Tiertwo tests passing also with -DDEBUG -DDEBUG_LOCKORDER.
ACK 2b5fb9b
Follow up to #1829, focused on decrease the testing time and continue improving the tier two functional tests.
Introduced the following changes:
Implemented a new RPC command
initmasternodeto initialize the masternode at runtime, with direct impact in the functional test time. The tests don't need to shut the nodes down and restart them anymore in order to setup the masternode/s.Adapted
tiertwo_governance_sync_basic.pyto the new runtime initialization feature.Further tier two cleanup.
Masternodeconfigcode refactored properly.It's still in WIP (because it need some more work) but the results are very favorable for now:
Test previous time:
Test time with this changes: