Consensus: Fail closed on Spark batch verification

[reubenyap]

PR intention

Make deferred Spark batch proof verification fail closed before validation state is persisted as accepted.

This PR is the first security-first step toward improving Spark-era reindex performance. It keeps Spark proof batching available for old blocks, where doing every proof immediately is expensive, while ensuring that any deferred batch must be verified successfully before the node writes durable validation state or marks reindexing complete.

Why this is desirable

Reindexing becomes noticeably slower once Spark-era blocks are reached because Spark spend validation has expensive cover-set and proof-verification work. The existing batching mode is intended to reduce that cost by collecting Spark spend proofs from old blocks and verifying them together instead of doing every proof immediately block by block.

That performance direction is desirable, but the deferred-verification boundary needs to be explicit and fail-closed before further optimization. If a node is allowed to continue writing chainstate, block-index state, or the reindex completion flag while Spark proofs are still pending, then the local durable state can get ahead of the proof-verification result. Even if a later verification failure would eventually stop the node, that is the wrong security shape for consensus-sensitive validation: pending privacy-spend proofs should be treated as unaccepted until the deferred batch has passed.

This PR makes that invariant visible in the validation layer so future reindex-speed work can build on it safely.

Existing behavior this avoids

In the current codebase, when -batching is enabled and the node is processing blocks older than one day, Spark spends may be collected into the batch container instead of being fully verified immediately. ConnectBlock() finalizes the per-block collection, and ActivateBestChain() only runs the batch verifier when it leaves the old-block batching mode.

That means a long reindex or initial sync can accumulate pending Spark proof work while other validation paths continue. Periodic/full flushes and the reindex completion path did not explicitly require those pending Spark proofs to be verified first. The old batch verifier also reported failure by throwing from batch_spark(), which made failure handling depend on where verification happened rather than on a clear validation-state result.

The undesirable part is not batching itself. Batching is useful for performance. The undesirable part is that deferred proof verification was not clearly coupled to the persistence boundaries that make validation progress durable.

Code changes brief

• Make Spark batch verification return success/failure instead of throwing from the batch verifier path.
• Add BatchProofContainer::verify_pending() to finalize any collected Spark spends and verify the pending batch explicitly.
• Preserve pending Spark spends on verification failure and clear them only after successful batch verification.
• Add VerifyPendingSparkBatch() as a validation-layer guard that aborts the node with a recovery hint if pending Spark proofs fail.
• Require pending Spark batch verification before durable block-index/chainstate flushes.
• Require pending Spark batch verification before clearing the reindex flag.
• Check pending Spark batch verification during shutdown before the final flush path.
• Keep normal batching behavior intact so this PR does not deliberately regress Spark-era reindex speed.

Security and correctness notes

This PR does not change Spark consensus rules, proof construction, cover-set selection, or the underlying Spark verifier. It only changes when a deferred batch must be forced to a verification result and how that result is propagated.

The failure mode is intentionally fail-closed: if a pending Spark batch fails verification, the node aborts instead of persisting validation progress past that point. The recovery hint remains to restart with -reindex -batching=0, which forces block-by-block verification and helps identify the invalid Spark spend.

The PR also keeps failed batch contents in memory rather than clearing them, so a failure does not silently discard the evidence needed by the current process to remain stopped at the failed deferred-verification boundary.

Intended follow-up speed optimization

After this fail-closed boundary is reviewed and merged, the real reindex-speed work should focus on reducing repeated Spark anonymity-set history work while preserving the same verification result.

The likely next step is to introduce a validation-owned, in-memory Spark batch context/cache for old-block reindex and IBD paths. Instead of having each Spark spend independently walk block history and reconstruct the same cover-set metadata, the batch path should collect the unique Spark cover-set references needed by the pending spends, keyed by the coin group and the referenced accumulator/block hash. The node can then resolve each unique reference once, derive the cover-set size and representation from chain state once, and reuse that derived metadata for all spends in the pending batch.

The batch verifier should likewise avoid rebuilding the same full cover sets repeatedly. For each unique group/reference needed by the pending batch, it should construct or fetch the corresponding derived cover-set data once, then pass those deterministic chain-derived results into the existing Spark proof verifier for the collected spends.

Important security constraints for that follow-up:

• Do not skip Spark proof verification; only avoid repeated derivation of identical inputs to the verifier.
• Do not treat cached data as consensus-authoritative. The cache should be derived from the current chain state and safe to discard/rebuild.
• Bind cached entries to the exact group/reference block data they were derived from, not just to a loose height or mutable tip assumption.
• Clear or invalidate cached entries across disconnect/reorg paths and at forced verification boundaries.
• Preserve the current fail-closed behavior from this PR before any durable validation state is written.
• Keep -batching=0 as the conservative diagnostic path that verifies spends without the deferred batch optimization.

That follow-up is expected to address the practical slowdown around Spark activation much more directly than this PR. This PR intentionally prepares the correctness boundary first so the later cache/de-duplication work can optimize without weakening verification integrity.

Scope

This is deliberately minimal. It does not add the larger cover-set metadata cache or broader Spark reindex acceleration yet. Those optimizations should be easier to reason about after this fail-closed boundary is in place.

Validation

• git diff --check origin/master...HEAD -- src/batchproof_container.cpp src/batchproof_container.h src/init.cpp src/validation.cpp src/validation.h
• Build/tests not run locally because cmake and ninja are not available on PATH in this environment.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 067170dd-b4ca-42a7-a545-107ad2f0843d

📥 Commits

Reviewing files that changed from the base of the PR and between 5ede8cd and c605406.

📒 Files selected for processing (2)

• src/init.cpp
• src/validation.cpp

---

Summary by CodeRabbit

• Bug Fixes

  • Improved Spark batch verification by adding explicit “pending” verification during shutdown, reindexing, and state flushes to prevent proceeding when finalized proofs can’t be confirmed.
  • Verification failures now stop the node early with clear messaging and restart guidance, avoiding unsafe continuation (and blocking related reindex flag updates when needed).

• Refactor

  • Updated Spark batch verification APIs to return bool and added a dedicated pending verification method with optional chain-height support for cover-set rebuilding.

Walkthrough

BatchProofContainer::verify(), batch_spark(), and new verify_pending() return bool with optional nChainHeight parameter. A new VerifyPendingSparkBatch() helper in validation.cpp calls verify_pending() and aborts the node on failure. Call sites in validation.cpp (best-chain activation, FlushStateToDisk) and init.cpp (Shutdown, ThreadImport reindex) are updated to use the new return values and abort or halt early on verification failure.

Changes

Spark Batch Proof Verification Hardening

Layer / File(s)	Summary
BatchProofContainer API: bool returns and verify_pending() src/batchproof_container.h, src/batchproof_container.cpp	verify() and batch_spark() return bool with optional int nChainHeight parameter. verify() returns true immediately when collecting proofs, otherwise delegates to batch_spark(). New verify_pending() clears pending transactions, disables collection, and runs batch_spark(), returning its result. finalize() conditionally clears tempSparkTransactions only when collecting. batch_spark() returns true on empty set, conditionally acquires coin sets via GetCoinSetForSpend() when nChainHeight >= 0 otherwise GetCoinSet(), and returns false on failure or true on success instead of throwing exceptions.
VerifyPendingSparkBatch helper in validation src/validation.h, src/validation.cpp	New VerifyPendingSparkBatch(CValidationState&, const std::string&, int nChainHeight = -1) declared with Doxygen documentation. GetSparkBatchVerificationHeight() computes verification height from best-block state. VerifyPendingSparkBatch() calls verify_pending() and invokes AbortNode with restart guidance on failure.
FlushStateToDisk verification gates src/validation.cpp	FlushStateToDisk() computes nSparkBatchVerificationHeight at entry and gates disk writes: before writing pruned-block-files flag during pruning, and before full-flush or periodic-write operations. VerifyPendingSparkBatch failure returns false from FlushStateToDisk to prevent the flush.
Best-chain activation and notification verification src/validation.cpp	ActivateBestChainStep() inserts VerifyPendingSparkBatch() check before connecting blocks when proof-collection is disabled. ActivateBestChain() removes unconditional batchProofContainer->verify() after tip switching and proceeds directly to transaction notification and callback loop.
init.cpp: replace direct BatchProofContainer calls with VerifyPendingSparkBatch src/init.cpp	Removes batchproof_container.h include. Shutdown() replaces BatchProofContainer finalize/verify with VerifyPendingSparkBatch using local CValidationState, logging failure and returning early to halt remaining shutdown steps. ThreadImport() reindex path inserts VerifyPendingSparkBatch before clearing reindex state and returns early on verification failure, preventing subsequent WriteReindexing(false), fReindex = false, and InitBlockIndex() operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• firoorg/firo#1787: Both PRs touch the Spark spend proof/verification pipeline around BatchProofContainer; this PR refactors batch proof verification APIs and adds pending-batch verification, while #1787 adjusts async proof scheduling and cache behavior where BatchProofContainer is used.

Suggested reviewers

• levonpetrosyan93
• psolstice

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% 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
Title check	✅ Passed	The title concisely and clearly describes the main change: implementing fail-closed behavior for Spark batch verification, which matches the primary intent across all modified files.
Description check	✅ Passed	The description fully covers the mandatory PR intention section, explaining the purpose, context, and rationale. It also provides comprehensive optional code-change details, security notes, and implementation scope.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/spark-batch-fail-closed

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/batchproof_container.cpp (1)

28-35: 💤 Low value

Clarify the conditional logic in verify() with a comment.

The current logic returns true (success) when fCollectProofs is true, which skips batch verification. This appears intentional—when proofs are still being collected, there's nothing finalized to verify yet—but the conditional is non-obvious. A brief comment would help future readers understand this is expected behavior rather than a bug.

📝 Suggested comment

 bool BatchProofContainer::verify() {
     if (fCollectProofs) {
+        // Still in collection mode; nothing finalized to verify yet.
         fCollectProofs = false;
         return true;
     }
 
     return batch_spark();
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/batchproof_container.cpp` around lines 28 - 35, The verify() method in
BatchProofContainer has a non-obvious conditional that returns true when
fCollectProofs is true, which skips batch verification. Add a clarifying comment
above or within the if (fCollectProofs) block explaining that this early return
is intentional because when proofs are still being collected, there is nothing
finalized to verify yet, so the function returns success without performing the
actual batch verification that happens in the batch_spark() call.

src/validation.cpp (2)

2237-2246: ⚡ Quick win

Document the aborting verification boundary.

VerifyPendingSparkBatch is now a cross-layer helper with non-obvious side effects: it verifies pending proofs, aborts the node, starts shutdown, and mutates state on failure. Please add a Doxygen contract that captures the guarded boundary and caller preconditions.

Suggested documentation

+/**
+ * Verify any deferred Spark proof batch before crossing a durable validation boundary.
+ *
+ * `@param` state Validation state populated when verification fails.
+ * `@param` reason Human-readable operation being guarded, used in diagnostics.
+ * `@return` true if no pending batch exists or verification succeeds; false after aborting the node.
+ * `@pre` Call before persisting or publishing state that depends on the pending Spark batch.
+ */
 bool VerifyPendingSparkBatch(CValidationState& state, const std::string& reason)

As per coding guidelines, “Use Doxygen-compatible comments with @param, @return, and @pre tags for function documentation.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/validation.cpp` around lines 2237 - 2246, Add Doxygen documentation to
the VerifyPendingSparkBatch function that documents its contract and side
effects. Include tags for `@param` describing the CValidationState reference and
reason string parameter, `@return` describing the boolean return value indicating
success or failure, and `@pre` describing caller preconditions. The documentation
should clearly capture that this function verifies pending Spark proofs, may
abort the node and trigger shutdown on verification failure, and will mutate the
state parameter when verification fails, so callers understand the cross-layer
behavior and non-obvious side effects involved.

Source: Coding guidelines

---

3098-3099: 🏗️ Heavy lift

Avoid doing Spark batch verification inside the disk-flush lock scope.

FlushStateToDisk holds cs_main and cs_LastBlockFile from Line 3051, so Line 3098 can now run expensive Spark proof verification while blocking validation and block-file operations. Consider restructuring this into a smaller scoped lock/retry boundary: compute that a durable write is needed, release the locks for VerifyPendingSparkBatch, then reacquire/recompute before writing so the fail-closed guarantee is preserved without extending the critical section.

As per coding guidelines, “Scope lock regions carefully with braces to minimize critical sections,” and the PR context identifies Spark proof verification as expensive.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/validation.cpp` around lines 3098 - 3099, The VerifyPendingSparkBatch
call at line 3098 is executing while holding the cs_main and cs_LastBlockFile
locks (acquired at line 3051), which blocks validation and block-file operations
during expensive Spark proof verification. Restructure the FlushStateToDisk
function to move the VerifyPendingSparkBatch call outside the critical section:
first determine whether a durable write is needed with the locks held, then
release both cs_main and cs_LastBlockFile, execute VerifyPendingSparkBatch
outside the locks, and finally reacquire the locks before performing the actual
disk write operations. Ensure the fail-closed guarantee is preserved by
recomputing conditions after reacquiring the locks to verify the state has not
changed.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/batchproof_container.cpp`:
- Around line 84-86: The LogPrintf call with the message "Spark batch
verification failed." in the conditional block checking !passed is missing a
trailing newline character, while other LogPrintf calls in the same function
include it for consistent formatting. Add a `\n` character at the end of the log
message string in the LogPrintf call to match the formatting convention used
elsewhere in the function.

In `@src/validation.cpp`:
- Around line 3814-3818: The batch proof verification check via
batchProofContainer->verify() is occurring too late in the validation flow.
Currently it happens after ActivateBestChainStep() returns, which means
ConnectBlock() has already written to pblocktree and ConnectTip() has already
updated wallet/listener state. Move the batchProofContainer->verify() check to
occur before these side effects happen, either by moving it earlier in the code
flow before ActivateBestChainStep() is called, or by restructuring the logic to
defer the durable index writes and state updates until after batch verification
succeeds. This ensures that if verification fails, no side effects like
pblocktree writes or wallet state changes have already been applied.

---

Nitpick comments:
In `@src/batchproof_container.cpp`:
- Around line 28-35: The verify() method in BatchProofContainer has a
non-obvious conditional that returns true when fCollectProofs is true, which
skips batch verification. Add a clarifying comment above or within the if
(fCollectProofs) block explaining that this early return is intentional because
when proofs are still being collected, there is nothing finalized to verify yet,
so the function returns success without performing the actual batch verification
that happens in the batch_spark() call.

In `@src/validation.cpp`:
- Around line 2237-2246: Add Doxygen documentation to the
VerifyPendingSparkBatch function that documents its contract and side effects.
Include tags for `@param` describing the CValidationState reference and reason
string parameter, `@return` describing the boolean return value indicating success
or failure, and `@pre` describing caller preconditions. The documentation should
clearly capture that this function verifies pending Spark proofs, may abort the
node and trigger shutdown on verification failure, and will mutate the state
parameter when verification fails, so callers understand the cross-layer
behavior and non-obvious side effects involved.
- Around line 3098-3099: The VerifyPendingSparkBatch call at line 3098 is
executing while holding the cs_main and cs_LastBlockFile locks (acquired at line
3051), which blocks validation and block-file operations during expensive Spark
proof verification. Restructure the FlushStateToDisk function to move the
VerifyPendingSparkBatch call outside the critical section: first determine
whether a durable write is needed with the locks held, then release both cs_main
and cs_LastBlockFile, execute VerifyPendingSparkBatch outside the locks, and
finally reacquire the locks before performing the actual disk write operations.
Ensure the fail-closed guarantee is preserved by recomputing conditions after
reacquiring the locks to verify the state has not changed.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5a7a95f0-1604-4511-a070-ca6ac125bb75

📥 Commits

Reviewing files that changed from the base of the PR and between 7fabe1e and a4d9f1a.

📒 Files selected for processing (5)

• src/batchproof_container.cpp
• src/batchproof_container.h
• src/init.cpp
• src/validation.cpp
• src/validation.h

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4d9f1a1b0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[codeant-ai]

User description

PR intention

Make deferred Spark batch proof verification fail closed before validation state is persisted as accepted.

This PR is the first security-first step toward improving Spark-era reindex performance. It keeps Spark proof batching available for old blocks, where doing every proof immediately is expensive, while ensuring that any deferred batch must be verified successfully before the node writes durable validation state or marks reindexing complete.

Why this is desirable

Reindexing becomes noticeably slower once Spark-era blocks are reached because Spark spend validation has expensive cover-set and proof-verification work. The existing batching mode is intended to reduce that cost by collecting Spark spend proofs from old blocks and verifying them together instead of doing every proof immediately block by block.

That performance direction is desirable, but the deferred-verification boundary needs to be explicit and fail-closed before further optimization. If a node is allowed to continue writing chainstate, block-index state, or the reindex completion flag while Spark proofs are still pending, then the local durable state can get ahead of the proof-verification result. Even if a later verification failure would eventually stop the node, that is the wrong security shape for consensus-sensitive validation: pending privacy-spend proofs should be treated as unaccepted until the deferred batch has passed.

This PR makes that invariant visible in the validation layer so future reindex-speed work can build on it safely.

Existing behavior this avoids

In the current codebase, when -batching is enabled and the node is processing blocks older than one day, Spark spends may be collected into the batch container instead of being fully verified immediately. ConnectBlock() finalizes the per-block collection, and ActivateBestChain() only runs the batch verifier when it leaves the old-block batching mode.

That means a long reindex or initial sync can accumulate pending Spark proof work while other validation paths continue. Periodic/full flushes and the reindex completion path did not explicitly require those pending Spark proofs to be verified first. The old batch verifier also reported failure by throwing from batch_spark(), which made failure handling depend on where verification happened rather than on a clear validation-state result.

The undesirable part is not batching itself. Batching is useful for performance. The undesirable part is that deferred proof verification was not clearly coupled to the persistence boundaries that make validation progress durable.

Code changes brief

• Make Spark batch verification return success/failure instead of throwing from the batch verifier path.
• Add BatchProofContainer::verify_pending() to finalize any collected Spark spends and verify the pending batch explicitly.
• Preserve pending Spark spends on verification failure and clear them only after successful batch verification.
• Add VerifyPendingSparkBatch() as a validation-layer guard that aborts the node with a recovery hint if pending Spark proofs fail.
• Require pending Spark batch verification before durable block-index/chainstate flushes.
• Require pending Spark batch verification before clearing the reindex flag.
• Check pending Spark batch verification during shutdown before the final flush path.
• Keep normal batching behavior intact so this PR does not deliberately regress Spark-era reindex speed.

Security and correctness notes

This PR does not change Spark consensus rules, proof construction, cover-set selection, or the underlying Spark verifier. It only changes when a deferred batch must be forced to a verification result and how that result is propagated.

The failure mode is intentionally fail-closed: if a pending Spark batch fails verification, the node aborts instead of persisting validation progress past that point. The recovery hint remains to restart with -reindex -batching=0, which forces block-by-block verification and helps identify the invalid Spark spend.

The PR also keeps failed batch contents in memory rather than clearing them, so a failure does not silently discard the evidence needed by the current process to remain stopped at the failed deferred-verification boundary.

Intended follow-up speed optimization

After this fail-closed boundary is reviewed and merged, the real reindex-speed work should focus on reducing repeated Spark anonymity-set history work while preserving the same verification result.

The likely next step is to introduce a validation-owned, in-memory Spark batch context/cache for old-block reindex and IBD paths. Instead of having each Spark spend independently walk block history and reconstruct the same cover-set metadata, the batch path should collect the unique Spark cover-set references needed by the pending spends, keyed by the coin group and the referenced accumulator/block hash. The node can then resolve each unique reference once, derive the cover-set size and representation from chain state once, and reuse that derived metadata for all spends in the pending batch.

The batch verifier should likewise avoid rebuilding the same full cover sets repeatedly. For each unique group/reference needed by the pending batch, it should construct or fetch the corresponding derived cover-set data once, then pass those deterministic chain-derived results into the existing Spark proof verifier for the collected spends.

Important security constraints for that follow-up:

• Do not skip Spark proof verification; only avoid repeated derivation of identical inputs to the verifier.
• Do not treat cached data as consensus-authoritative. The cache should be derived from the current chain state and safe to discard/rebuild.
• Bind cached entries to the exact group/reference block data they were derived from, not just to a loose height or mutable tip assumption.
• Clear or invalidate cached entries across disconnect/reorg paths and at forced verification boundaries.
• Preserve the current fail-closed behavior from this PR before any durable validation state is written.
• Keep -batching=0 as the conservative diagnostic path that verifies spends without the deferred batch optimization.

That follow-up is expected to address the practical slowdown around Spark activation much more directly than this PR. This PR intentionally prepares the correctness boundary first so the later cache/de-duplication work can optimize without weakening verification integrity.

Scope

This is deliberately minimal. It does not add the larger cover-set metadata cache or broader Spark reindex acceleration yet. Those optimizations should be easier to reason about after this fail-closed boundary is in place.

Validation

• git diff --check origin/master...HEAD -- src/batchproof_container.cpp src/batchproof_container.h src/init.cpp src/validation.cpp src/validation.h
• Build/tests not run locally because cmake and ninja are not available on PATH in this environment.

---

CodeAnt-AI Description

Fail closed on deferred Spark proof verification before reindex, flush, and shutdown

What Changed

• Spark proof batches are now verified before the node writes durable chainstate or block index updates, clears the reindex flag, or finishes shutdown
• If deferred Spark proof verification fails, the node stops with a clear message telling the user to restart with -reindex -batching=0 to find the invalid spend
• Reindexing no longer completes until any pending Spark proof batch has been checked
• Spark batch verification now reports failure cleanly instead of throwing an exception

Impact

✅ Fewer silent consensus failures during reindex
✅ Safer chainstate and block index writes
✅ Clearer recovery after invalid Spark spends

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[coderabbitai]

♻️ Duplicate comments (1)

src/validation.cpp (1)

3816-3822: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Spark batch verification is still placed after connect-side durable effects.

Line 3818 verifies the batch only after ActivateBestChainStep() has completed block connection work. A failed verification at this point can still happen after connection-time persisted/index side effects have already advanced, which weakens the intended fail-closed boundary. The verification gate needs to be enforced before those writes/commits in the connect path, not after step completion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/validation.cpp` around lines 3816 - 3822, The Spark batch verification
using `batchProofContainer->verify(pindexNewTip->nHeight)` is currently
positioned after `ActivateBestChainStep()` has already persisted block
connection side effects to disk, which violates the fail-closed boundary. Move
the batch verification logic and the fCollectProofs assignment for the
BatchProofContainer instance to occur before the `ActivateBestChainStep()` call
completes, ensuring that the verification gate is enforced before any durable
writes or index commits happen in the connect path rather than after step
completion.

🧹 Nitpick comments (1)

src/validation.cpp (1)

3832-3833: Use pre-increment for the loop iterator.

Line 3832 uses i++; project style here is ++i.

Suggested patch

-            for (unsigned int i = 0; i < block.vtx.size(); i++)
+            for (unsigned int i = 0; i < block.vtx.size(); ++i)
                 GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/validation.cpp` around lines 3832 - 3833, The for loop iterating through
block.vtx uses post-increment (i++) for the loop iterator, but the project style
requires pre-increment (++i). Change the increment expression in the for loop
condition from i++ to ++i to match the project's coding standards.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/validation.cpp`:
- Around line 3816-3822: The Spark batch verification using
`batchProofContainer->verify(pindexNewTip->nHeight)` is currently positioned
after `ActivateBestChainStep()` has already persisted block connection side
effects to disk, which violates the fail-closed boundary. Move the batch
verification logic and the fCollectProofs assignment for the BatchProofContainer
instance to occur before the `ActivateBestChainStep()` call completes, ensuring
that the verification gate is enforced before any durable writes or index
commits happen in the connect path rather than after step completion.

---

Nitpick comments:
In `@src/validation.cpp`:
- Around line 3832-3833: The for loop iterating through block.vtx uses
post-increment (i++) for the loop iterator, but the project style requires
pre-increment (++i). Change the increment expression in the for loop condition
from i++ to ++i to match the project's coding standards.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: faa25d77-bcde-4c05-874f-0e23f049fbd0

📥 Commits

Reviewing files that changed from the base of the PR and between a4d9f1a and a92b713.

📒 Files selected for processing (4)

• src/batchproof_container.cpp
• src/batchproof_container.h
• src/validation.cpp
• src/validation.h

🚧 Files skipped from review as they are similar to previous changes (1)

• src/batchproof_container.h