Skip to content

test: restore complete spork21 signing coverage - #7552

Merged
PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/restore-llmq-signing-spork21-coverage
Aug 7, 2026
Merged

test: restore complete spork21 signing coverage#7552
PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/restore-llmq-signing-spork21-coverage

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Aug 6, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

PR #7497 merged the default and --spork21 variants of feature_llmq_signing.py based on the assumption that the shared signing-session flow does not depend on spork 21.

That assumption does not hold for the signature-share transport. Mainnet hardens SPORK_21_QUORUM_ALL_CONNECTED to value 1, which keeps the all-connected concentrated QSIGSHARE path enabled for small quorums while LLMQ-100/400 quorums retain the legacy announce/request and QBSIGSHARES path. Both implementations are therefore live and need coverage for the complete signing-session lifecycle.

The shortened mid-test spork section retained basic threshold recovery and recovery-member retry coverage, but no longer exercised conflict handling, quorum aging, recovered-signature cleanup, and session reuse through the spork-21 transport.

What was done?

  • Restored the --spork21 option and activation before the first quorum is mined.
  • Restored the original spork-specific QSIGSHARE submission, full-connectivity checks, and recovery-member isolation flow.
  • Removed the mid-test spork activation and shortened duplicate signing section.
  • Re-added feature_llmq_signing.py --spork21 to the functional test runner.

This is limited to the signing-test deduplication from #7497. Its wallet RPC coverage split and spork-23 test optimization remain unchanged.

How Has This Been Tested?

The following checks passed against a current local Dash Core build:

  • test/functional/feature_llmq_signing.py
  • test/functional/feature_llmq_signing.py --spork21
  • python3 -m py_compile test/functional/feature_llmq_signing.py test/functional/test_runner.py
  • test/lint/lint-whitespace.py
  • git diff --check

test/lint/lint-python.py was also invoked, but skipped because flake8 is not installed in the local environment.

Breaking Changes

None. Test-only change.

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 (not applicable)
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

This pull request was created by Codex.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The functional test adds a --spork21 mode that enables SPORK 21 before the first quorum and checks quorum connectivity and symmetric recovered-signature messaging. In this mode, valid submit=false signature shares are converted to CSigShare and sent through P2P. Recovery-member isolation uses a later request ID. The test runner adds a base invocation with --spork21.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: knst, udjinm6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly states that the change restores complete SPORK 21 signing test coverage.
Description check ✅ Passed The description directly explains the restored SPORK 21 coverage, test-runner update, validation steps, and test-only scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@thepastaclaw

thepastaclaw commented Aug 6, 2026

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit 2f0bd0e)

1 similar comment
@thepastaclaw

thepastaclaw commented Aug 6, 2026

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit 2f0bd0e)

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

utACK 2f0bd0e

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Final validation — Codex + Opus

The PR-specific commit cleanly restores the dedicated --spork21 signing-test variant and registers it with the functional test runner. The restored test is byte-for-byte identical to the established pre-deduplication version, and mine_quorum() waits for the all-connected topology before the subsequent connection and handshake assertions; no in-scope correctness or coverage issues were found.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
  • Verifier: gpt-5.6-sol — final-verifier (Opus verifier fallback)
  • Opus reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

@PastaPastaPasta
PastaPastaPasta merged commit 0c4973f into dashpay:develop Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants