Skip to content

test: optimize feature_llmq_data_recovery.py #6818

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

test: optimize feature_llmq_data_recovery.py #6818

wants to merge 3 commits into from
+25 −13

Conversation

knst
Copy link
Collaborator

@knst knst commented Aug 13, 2025 •
edited
Loading

Issue being fixed or feature implemented

The functional test feature_llmq_data_recover.py is currently the slowest one

What was done?

Refactors the LLMQ data recovery functional test.
Reduces node count and masternode count.
Faster fail-over in case if 2 quorums have same members - re-generate only one quorum, not both.
Reindex on restarted mns is done simultaneously on all nodes, achieved by splitting "restart" and "wait" operation for reindex.
Triggering of recovery threads is done for all restarted nodes at once, not one-by-one.

How Has This Been Tested?

Run 20 jobs in parallel. 1 slowest and 1 fastest job are discarded from results as outsiders.

This PR: test run takes 3360s accumulated; 90% jobs are finished in interval [159...179] seconds with median 167 seconds.
Develop: test run takes 4353 s (accumulated); 90% jobs are finished in interval [207..229] seconds with median 217 seconds.
It is 30% improvement for the worst case scenario; for median and for average.

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

knst added 3 commits August 13, 2025 16:53
@knst
test: decrease total amount of nodes from 9 to 7 in feature_llmq_data…
8bccb79 
…_recover.py

2 full nodes + 7 masternodes are replaced to 1 full node + 6 masternodes

Second full node is no necessary. 6 masternodes provides still high probability of
at least one quorum member to be different; futher commit reduces overhead in case
of unhappy choice for 50%
@knst
test: wait for reindex on restarted nodes asynchronously in feature_l…
655c5fa 
…lmq_data_recovery.py

It is achieved by splitting helper restart_mn to 2 helper: one to restart mn, other to wait it
to get in sync
@knst
test: generate less quorums in case of unlucky members distributions
7e94747
@knst knst added this to the 23 milestone Aug 13, 2025
@github-actions GitHub Actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai coderabbitai
Copy link

coderabbitai bot commented Aug 13, 2025

Walkthrough

Refactors the LLMQ data recovery functional test. Reduces node count and extra args in set_test_params/set_dash_test_params. Introduces wait_restarted_mn to centralize post-restart synchronization with optional reindex and block_count waiting, mnsync finalization, and reconnection. restart_mns now precomputes block_count, restarts targeted masternodes, and then invokes wait_restarted_mn per node. When qdata_recovery_enabled, restart_mns triggers recovery by generating a block, bumping mocktime, and syncing blocks. Updates test_llmq_qvvec_sync for llmq_sync_mode == 1 to maintain consistent, non-empty, incremental quorum/member sets using reused hashes and member lists.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 489c5f0 and 7e94747.

📒 Files selected for processing (1)
  • test/functional/feature_llmq_data_recovery.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/functional/**/*.py

📄 CodeRabbit Inference Engine (CLAUDE.md)

Functional tests should be written in Python and placed in test/functional/

Files:

  • test/functional/feature_llmq_data_recovery.py
🧬 Code Graph Analysis (1)
test/functional/feature_llmq_data_recovery.py (3)
test/functional/test_framework/test_framework.py (11)
  • set_dash_test_params (1508-1530)
  • MasternodeInfo (1142-1472)
  • wait_until (904-905)
  • get_node (1216-1221)
  • connect_nodes (699-739)
  • connect_nodes (1499-1506)
  • generate (803-806)
  • no_op (800-801)
  • bump_mocktime (877-894)
  • sync_blocks (823-843)
  • mine_quorum (2109-2196)
test/functional/test_framework/util.py (1)
  • force_finish_mnsync (531-537)
test/functional/p2p_quorum_data.py (1)
  • restart_mn (126-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: linux64_multiprocess-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (4)
test/functional/feature_llmq_data_recovery.py (4)

31-32: Node count reduction appears valid for test optimization.

The reduction from 9 to 7 nodes and from 7 to 6 masternodes is consistent with the PR's optimization goal. This change reduces resource usage while still maintaining enough nodes for testing LLMQ data recovery scenarios.

48-54: Well-structured helper method for post-restart synchronization.

The new wait_restarted_mn method effectively consolidates the post-restart wait logic, properly handling the reindex case by waiting for the expected block count before proceeding with mnsync finalization and reconnection.

61-68: Effective separation of restart and wait phases.

The two-phase approach (restart all nodes first, then wait for all) is a good optimization that allows nodes to restart in parallel before synchronization, potentially improving test performance.

70-77: Recovery trigger logic properly centralized.

Moving the recovery trigger logic outside the individual node restart loop is a good refactor. The block generation and mocktime bump properly initiate the DKG data recovery threads when enabled.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

UdjinM6
UdjinM6 approved these changes Aug 14, 2025
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7e94747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta Awaiting requested review from PastaPastaPasta

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
23
Development

Successfully merging this pull request may close these issues.
2 participants
@knst @UdjinM6