feat: process ChainLocks embedded in connected blocks

[PastaPastaPasta]

Issue being fixed or feature implemented

A synced node that misses a ChainLock message currently stores the signature embedded in later coinbases without using it to update its best ChainLock. This change lets it recover the ChainLock when those blocks connect.

What was done?

• Process coinbase signatures from the block already supplied to the block-connected notification, after v20 activation and while ChainLocks are enabled and the node is synced.
• Derive the signed ancestor as containing height - bestCLHeightDiff - 1, checking the unsigned offset before conversion. Reuse normal quorum-signature verification, duplicate suppression, asynchronous enforcement, and inventory relay.
• Adapt the original implementation to the current chainlock::ChainlockHandler. The peer manager supplies the quorum manager and handles the returned relay inventory from its block-connected callback.
• Replace arithmetic-only tests with coverage that calls the production handler. Replace the previous functional test with a two-node, single-member-quorum test that disconnects the regular node and delivers blocks exclusively through submitblock. Cover zero/nonzero offsets, restart recovery, repeated signatures, and normal relay after reconnection.
• Add doc/release-notes-6236.md.

The old nine-commit series was consolidated and rebased onto 552577d32a (develop). Existing upstream ChainLock tests and quorum helpers are retained. The unrelated CCoinbaseChainlock pair-to-class refactor, unused helpers, and duplicate tests are omitted; extraction still uses the in-memory block, without a disk read. No mining, serialization, or coinbase consensus-validation rules are changed.

How Has This Been Tested?

Locally on Apple Silicon macOS, using the prebuilt depends prefix and a wallet-enabled build without GUI:

• Full make -j8 build.
• test_dash --run_test=llmq_chainlock_tests.
• feature_llmq_chainlocks_automatic.py, feature_llmq_chainlocks.py, and rpc_verifychainlock.py.
• Mutation check: restoring the original missing -1 makes the new unit test fail at all three tested offsets and makes the functional test fail when the isolated receiver must recover its first coinbase-delivered ChainLock. Both pass with the fix restored.
• Python lint with the repository-pinned mypy 0.981, including its 312-file type-check pass; circular dependencies, includes, assertions, whitespace, and formatting of changed C++ lines.

CI follow-up

The first full run passed lint, the standard Linux/SQLite/no-wallet/ASan test jobs, and the other platform builds. Two failures in unchanged code are being retried:

• Multiprocess build: net_processing.cpp compilation started at 16:58:31 before the recursive BLS build generated relic_conf.h at 16:58:36. The missing generated-header dependency is present on the base revision.
• TSan test: feature_protx_version.py detected concurrent first-use initialization of ep2_curve_get_s3 in unchanged src/dashbls/src/legacy.cpp, reached by two DKG signing threads. This is a real pre-existing race, not a timeout or a ChainLock-test failure; a retry does not fix the underlying BLS issue.

Only failed jobs were rerun in attempt 2. No test suppression or unrelated source change was added.

Breaking Changes

None. Nodes can now learn an existing, valid ChainLock from a connected block as well as from a standalone message.

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

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• perf: use cache for SML #6738 - perf: use cache for SML
• refactor: move ChainLocks impl. to src/chainlock/ and split masternode logic out of manager source #6761 - refactor: move ChainLocks impl. to src/chainlocks/ and split masternode logic out of manager source
• test: add comprehensive unit tests for LLMQ subsystem #6691 - test: add comprehensive unit tests for LLMQ subsystem

Please coordinate with the authors of these PRs to avoid merge conflicts.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 0d0f167e-c7eb-4bba-acdc-447d0aa45ee0

📥 Commits

Reviewing files that changed from the base of the PR and between 552577d and 47b0bfb.

📒 Files selected for processing (7)

• doc/release-notes-6236.md
• src/chainlock/handler.cpp
• src/chainlock/handler.h
• src/net_processing.cpp
• src/test/llmq_chainlock_tests.cpp
• test/functional/feature_llmq_chainlocks_automatic.py
• test/functional/test_runner.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

The change processes ChainLock signatures embedded in coinbase transactions after v20 activation. The handler validates synchronization, deployment status, payload format, referenced height, and ancestor availability. Connected blocks route the result through standard message post-processing. Unit and functional tests cover valid, invalid, isolated, repeated, restarted, and reconnected scenarios.

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

Sequence Diagram(s)

sequenceDiagram
  participant BlockConnected
  participant ChainlockHandler
  participant ProcessNewChainLock
  participant PostProcessMessage
  BlockConnected->>ChainlockHandler: ProcessCoinbaseChainLock(block, pindex, qman)
  ChainlockHandler->>ProcessNewChainLock: ProcessNewChainLock(ChainLockSig)
  ChainlockHandler-->>BlockConnected: MessageProcessingResult
  BlockConnected->>PostProcessMessage: PostProcessMessage(result, -1)

Loading

Merge Risk: ⚪ Minimal · up to 47b0b

The recovery path validates embedded ChainLocks and routes successful results through normal processing, with coverage for recovery, restart, duplicate, isolation, and relay scenarios. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1…	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.
Description check	✅ Passed	The description clearly explains the ChainLock recovery feature, implementation, tests, and release-note change. It is directly related to the changeset.
Title check	✅ Passed	The title clearly and concisely describes the main change: processing ChainLocks embedded in connected blocks.

Full details: Docstring Coverage

Explanation

Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

✅ Final review complete — no blockers (commit 47b0bfb) · triage: normal

[thepastaclaw]

Code Review

The PR refactors std::pair<CBLSSignature, uint32_t> into CCoinbaseChainlock cleanly, but the new automatic-detection path in CChainLocksHandler::BlockConnected has an off-by-one in the height derivation that diverges from every other use site (specialtxman, miner, rawtransaction, functional test). The result is that the constructed CChainLockSig points to the wrong block and the signature cannot verify, making the new feature non-functional. The new tests do not catch the bug — the unit tests assert tautological arithmetic and the functional test never forces the coinbase-only delivery path.

🔴 1 blocking | 🟡 2 suggestion(s) | 💬 3 nitpick(s)

Reviewed commit: 50eb7db

1. [BLOCKING] src/llmq/chainlocks.cpp:395 — Off-by-one in clsig_height derivation breaks automatic chainlock detection

bestCLHeightDiff is encoded such that the signed block lives at containing_block_height - bestCLHeightDiff - 1. This convention is used at every other call site:

• src/evo/specialtxman.cpp:71: int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
• src/node/miner.cpp:151: int prevCLHeight = pindexPrev->nHeight - static_cast<int>(prevBlockCoinbaseChainlock->heightDiff) - 1;
• src/rpc/rawtransaction.cpp:643: pTipBlockIndex->GetAncestor(pTipBlockIndex->nHeight - cbtx_best_cl->heightDiff - 1)
• Existing functional helper in test_coinbase_best_cl: best_cl_height = cb_height - best_cl_height_diff - 1

The new code drops the -1, so the constructed CChainLockSig carries a height/hash pair that the BLS signature does not actually cover. ProcessNewChainLock will fail signature verification for every legitimate coinbase-embedded chainlock, rendering the new automatic-detection feature non-functional. Note that the new unit tests at lines 180, 233, 295, 325 mirror the same incorrect formula (e.g. block_height - height_diff without -1), and the new functional test does not exercise the BlockConnected path, so CI does not catch the bug.

        int32_t clsig_height = pindex->nHeight - static_cast<int32_t>(coinbase_cl.heightDiff) - 1;

2. [SUGGESTION] src/llmq/chainlocks.cpp:398 — Tighten clsig_height bounds check to strictly less than pindex->nHeight

With the correct formula clsig_height = pindex->nHeight - heightDiff - 1, the signed block is strictly an ancestor of pindex, so the upper bound should be >= pindex->nHeight, not > pindex->nHeight. The current clsig_height > pindex->nHeight permits clsig_height == pindex->nHeight, which (after the off-by-one fix) cannot legitimately occur and indicates this path was never exercised. The fact that the existing check is > rather than >= is itself a tell that the off-by-one was masked during testing.

        if (clsig_height < 0 || clsig_height >= pindex->nHeight) {

3. [SUGGESTION] test/functional/feature_llmq_chainlocks_automatic.py:50 — Functional test does not exercise the BlockConnected coinbase-CL path

The test mines blocks and waits for chainlocks via wait_for_chainlocked_block / wait_for_chainlocked_block_all_nodes, then inspects coinbase fields. On this single-MN setup chainlocks always arrive through the normal LLMQ signing / gossip flow in CChainLocksHandler — independent of the new BlockConnected code path. The edge-case sub-test isolates node 1 (a non-MN) but later calls self.sync_blocks(), which re-enables the gossip path, so the new node still receives the chainlock via normal channels. As a result the test passes even though the off-by-one bug in chainlocks.cpp:395 makes coinbase-derived chainlocks unverifiable.

Additionally, line 92 computes cl_height = height - cbtx["bestCLHeightDiff"] (without -1), which is the same incorrect formula as the production bug — so even the test's own assertion (verifychainlock(cl_block_hash, ...)) would fail to confirm correctness; it's wrapped in try/except that swallows failures, so it logs success regardless.

To actually validate the feature: start (or restart) a node that cannot receive chainlocks via gossip (e.g. disconnected from the MN before any CLSIG message arrives, then fed only blocks whose coinbases carry CLs) and assert via getbestchainlock that the new node ends up with the chainlock. Compute the expected height as height - heightDiff - 1 to match the on-the-wire convention.

4. [NITPICK] src/test/llmq_chainlock_tests.cpp:180 — Several new unit tests assert tautologies, not behavior

automatic_chainlock_detection_logic_test (180), automatic_chainlock_edge_cases_test (233), chainlock_ancestor_lookup_edge_cases_test (295), and chainlock_comparison_and_validation_test (325) compute integer arithmetic locally (1000 - 5 == 995, 600 > 500, etc.) and assert the result of the local computation. They never invoke BlockConnected, GetCoinbaseChainlock, or any other production function. They also reinforce the incorrect block_height - height_diff formula (no -1), so they would not have surfaced the off-by-one in chainlocks.cpp:395. Replace with tests that construct a CBlock with a coinbase CCbTx, hand it to GetCoinbaseChainlock, and assert the derived height matches the on-the-wire convention used by the miner and validation code.

5. [NITPICK] src/evo/cbtx.cpp:260 — Redundant V20 deployment check in GetNonNullCoinbaseChainlock

GetNonNullCoinbaseChainlock checks DeploymentActiveAt(..., DEPLOYMENT_V20) at line 267, then calls GetCoinbaseChainlock which repeats the identical check at line 238. Either drop the outer check or have GetNonNullCoinbaseChainlock operate on the already-fetched block via the inner helper. Not a correctness issue.

6. [NITPICK] src/test/util/llmq_tests.h:27 — Unused test helpers added in this PR

GetLLMQParams, CreateValidCommitment, CreateChainLock, CreateBitVector, and TestSerializationRoundtrip are added but never called by any new test in this PR (the new tests only consume CreateRandomBLSSignature, GetTestQuorumHash, GetTestBlockHash). One reference exists at src/test/llmq_chainlock_tests.cpp:76, but only as a comment noting TestSerializationRoundtrip is broken and skipped. Trim the unused helpers, or land them in the PR where they are actually used.

Inline review posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[github-actions]

Potential PR merge conflicts

This is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order.

If this PR merges first

These open PRs will likely need a rebase:

• #7107: feat(rpc): add mining-transaction snapshot proofs Changed files: src/test/llmq_chainlock_tests.cpp, test/functional/test_runner.py.

If these PRs merge first

This PR will likely need a rebase:

• #7107: feat(rpc): add mining-transaction snapshot proofs Changed files: src/test/llmq_chainlock_tests.cpp, test/functional/test_runner.py.
• #7536: refactor(utils): consolidate masternode test helpers Changed files: src/Makefile.test_util.include.

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The implementation correctly extracts valid ChainLock signatures from connected coinbase transactions only when synchronization, ChainLocks, and V20 activation prerequisites are satisfied. It validates the height offset before conversion, derives the signed ancestor safely, and reuses the existing ChainLock verification, duplicate suppression, enforcement, and relay path; the accompanying unit and functional tests cover the relevant recovery and relay scenarios.

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

• Triage: normal by gpt-6-astra (effort low) — The change adds and wires ChainLock recovery logic across the handler, peer manager, unit tests, and functional tests, but it does not alter consensus validation, cryptographic primitives, peer-facing deserialization, funds movement, or persistence migrations.
• Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — dash-core-commit-history (completed, effort high); agent phase1-reviewer
• Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 84% left, 5h 93% left
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort high); agent phase2-reviewer, gpt-6-astra — dash-core-commit-history (completed, effort high); agent phase2-reviewer