Skip to content

Conversation

@furszy
Copy link

@furszy furszy commented Oct 14, 2020

Follow up to #1829, focused on decrease the testing time and continue improving the tier two functional tests.

Introduced the following changes:

  1. Implemented a new RPC command initmasternode to 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.

  2. Adapted tiertwo_governance_sync_basic.py to the new runtime initialization feature.

  3. Further tier two cleanup. Masternodeconfig code refactored properly.

It's still in WIP (because it need some more work) but the results are very favorable for now:

Test previous time:

  • 9:43 min

Test time with this changes:

  • 3:21 min

@furszy furszy self-assigned this Oct 14, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a 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.

@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch from b14f633 to 8228a9e Compare October 15, 2020 15:25
@furszy
Copy link
Author

furszy commented Oct 15, 2020

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.

@furszy furszy changed the title [WIP][Test][RPC] Decreasing tier two test time + introducing RPC call to initialize the masternode at runtime. [WIP][Test][RPC] Decreasing tier two test time + introducing RPC call to initialize the masternode. Oct 15, 2020
JSKitty
JSKitty previously approved these changes Oct 18, 2020
Copy link
Member

@JSKitty JSKitty left a 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!

furszy added a commit that referenced this pull request Oct 20, 2020
… 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
@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch 2 times, most recently from 9e119b6 to a40048e Compare October 20, 2020 06:01
@random-zebra random-zebra added this to the 5.0.0 milestone Oct 20, 2020
@furszy furszy changed the title [WIP][Test][RPC] Decreasing tier two test time + introducing RPC call to initialize the masternode. [Test][RPC] Decreasing tier two test time + introducing RPC call to initialize the masternode. Oct 21, 2020
@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch from a40048e to c89b00c Compare October 21, 2020 17:51
@furszy
Copy link
Author

furszy commented Oct 21, 2020

Updated per feedback plus added a basic budget finalization payment test coverage as well.

  • Update: It seems that the finalization is failing on current master.. will dig on it soon.

@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch 3 times, most recently from 8814444 to f5fa73c Compare October 22, 2020 19:53
Copy link

@random-zebra random-zebra left a 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 PivxTier2TestFramework based on mocked time
  • refactor masternode_activation and masternode_sync_basic tests 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.

@furszy
Copy link
Author

furszy commented Oct 25, 2020

awesome! 😄 go ahead!, either way is fine for me.

@random-zebra random-zebra force-pushed the 2020_decrease_tier_two_test_time branch from c28a06d to 637172a Compare October 25, 2020 20:47
Copy link
Author

@furszy furszy left a 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.

@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch 3 times, most recently from 7e95d00 to f99557f Compare October 29, 2020 02:54
@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch from dc5c14a to 7cfcd55 Compare October 29, 2020 05:22
random-zebra and others added 21 commits November 5, 2020 10:55
…in mainnet/testnet. Plus, not updating asset when the received asset isn't what we are looking for.
…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
…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.
@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch from 64c92c7 to dacfebe Compare November 5, 2020 14:54
@furszy
Copy link
Author

furszy commented Nov 5, 2020

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.

@furszy furszy requested a review from Fuzzbawls November 5, 2020 15:16
@furszy furszy force-pushed the 2020_decrease_tier_two_test_time branch from dacfebe to 2b5fb9b Compare November 5, 2020 16:38
Copy link

@random-zebra random-zebra left a 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

@random-zebra random-zebra merged commit 6d85251 into PIVX-Project:master Nov 6, 2020
@furszy furszy deleted the 2020_decrease_tier_two_test_time branch November 29, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants