Consensus: Fail closed on Spark batch verification by reubenyap · Pull Request #1863 · firoorg/firo

reubenyap · 2026-06-13T18:18:48Z

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. In particular, a crash or restart during a long sync permanently skips verification of the proofs that were pending in memory, because the batch container does not survive the restart while the flushed chainstate does. In addition, the old throwing failure path could be swallowed entirely when batch verification was reached from the network message thread (ProcessMessages() catches and continues), making the previous failure mode effectively fail-open.

This PR makes the 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 old failure path was also unhelpful to the operator: when it did crash the node, a plain restart redid the reindex/sync and ran into the same batched failure again, and the only way to learn which spend was invalid was a manual diagnostic reindex with -batching=0.

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, and that failure gave the operator neither the offending transaction nor a working plain-restart path.

Code changes brief

Make Spark batch verification return success/failure instead of throwing from the batch verifier path; remove the now-unused BatchProofContainer::verify() and make batch_spark() private.
Add BatchProofContainer::verify_pending() to discard in-progress per-block collection and verify the pending batch explicitly.
Preserve pending Spark spends on verification failure, and latch the failure (fBatchFailed) so the abort path fails fast at later boundaries instead of re-running full proof verification; removing spends from the pending batch (e.g. by disconnect) clears the latch so the changed batch is re-verified.
Record the txid alongside each collected proof; when a batch fails, re-verify the retained proofs individually (the cover sets are already built) and log the exact invalid spend transactions to debug.log, so the operator does not need a diagnostic reindex to identify the offender.
Write a one-shot sparkbatchfailed marker file in the datadir when a pending batch fails; on the next start the node consumes it in AppInitMain (right after fReindex is determined, before any database is opened) and forces -batching=0 for that run, so a restart re-verifies Spark proofs block by block and rejects the invalid block through the normal consensus path instead of looping into the same batched abort. A datadir file rather than a block-tree DB flag so the marker survives the database wipe of a run restarted with -reindex.
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/evoDb flushes and the prune flag in FlushStateToDisk().
Require pending Spark batch verification before connecting a non-batched (recent) block in ActivateBestChainStep().
Require pending Spark batch verification (under cs_main) before clearing the reindex flag in ThreadImport().
Share the one-day batching predicate between ConnectBlock() and the new guard via ShouldBatchSparkProofs() so the collector and the guard cannot silently diverge.
Shutdown needs no separate check: the final FlushStateToDisk() is already guarded and refuses to persist validation state if the pending batch fails, while node teardown still completes cleanly.
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. Cover sets for the batch are still built with GetCoinSet() exactly as before; supersets are safe because the batch verifier slices each proof's set by its recorded size.

The failure mode is intentionally fail-closed: if a pending Spark batch fails verification, the node logs the invalid spend txids, persists the one-shot sparkbatchfailed marker, and aborts instead of persisting validation progress past that point. On the next start batching is disabled automatically for that run, so the invalid spend is rejected through normal block-by-block verification without operator intervention; restarting manually with -reindex -batching=0 remains available as a diagnostic path.

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.

Tests

spark_batch_tests (Boost unit test, src/test/spark_batch_test.cpp): drives real wallet-created Spark spends through the deferred path — collection instead of inline verification, successful batch verification of a valid proof, fail-closed behavior with retained proofs when cover sets cannot be rebuilt, the failed-batch latch, and recovery in both removal shapes: removing the only offending spend from a mixed valid/invalid batch (the remaining valid proof verifies again) and removal that empties the batch.
spark_batching.py (regtest): mines a Spark chain with timestamps older than one day, performs Spark mints and spends, then reindexes with -batching=1 and asserts the node batch-verified the spends (via debug.log) and reached the same tip as a -batching=0 reindex.

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: resolve each unique cover-set reference once per pending batch instead of once per spend, bind cached entries to the exact group/reference block data they were derived from, invalidate across disconnect/reorg and forced verification boundaries, and keep -batching=0 as the conservative diagnostic path. 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: the consensus-code diff is confined to the batch container, the VerifyPendingSparkBatch() guard and its call sites, and the failure-recovery marker consumed at startup. It does not add the larger cover-set metadata cache or broader Spark reindex acceleration yet.

Validation

git diff --check across the touched files.
src/batchproof_container.cpp syntax-checked locally; full builds, unit tests, and RPC tests run in CI (the environment used to author this cannot fetch the bls-dash/tor depends).

coderabbitai · 2026-06-13T18:18:57Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 172a0867-aed3-47fb-9891-37ad758a0b1f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

Pending Spark proof verification now returns explicit success or failure, retains failed batches for fail-closed behavior, and runs at validation, flush, shutdown, and reindex boundaries. Unit and RPC tests cover failure recovery and reindex batching.

Changes

Spark Batch Proof Verification Hardening

Layer / File(s)	Summary
BatchProofContainer pending verification API `src/batchproof_container.h`, `src/batchproof_container.cpp`	Replaces `verify()` with `verify_pending()`, changes `batch_spark()` to return `bool`, clears finalized temporary transactions, and retains failed batches after verification errors.
Validation boundary checks `src/validation.h`, `src/validation.cpp`	Adds `VerifyPendingSparkBatch()` and `ShouldBatchSparkProofs()`. Pending batches are verified before durable flushes and before connecting blocks outside the batching path.
Reindex and shutdown integration `src/init.cpp`	Defers shutdown verification to the flush path and verifies pending Spark batches before clearing reindex state.
Unit and RPC regression coverage `src/test/spark_batch_test.cpp`, `src/test/CMakeLists.txt`, `qa/rpc-tests/spark_batching.py`, `qa/pull-tester/rpc-tests.py`	Adds fail-closed unit coverage and reindex batching coverage, including the new RPC test in the default suite.

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

Suggested labels: Spark

Suggested reviewers: psolstice, levoncrypto, levonpetrosyan93

🚥 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
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 and concisely summarizes the main change: fail-closed handling for Spark batch verification.
Description check	✅ Passed	The description includes the required PR intention and code changes brief sections with detailed rationale, implementation scope, and test coverage.

✨ Finishing Touches

🧪 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

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

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 · 2026-06-17T17:23:18Z

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

Review follow-up that keeps the fail-closed invariant while cutting the surface of the change: - Restore the connected-block SyncTransaction loop to its original position under cs_main in ActivateBestChain; moving it out of the lock changed notification ordering guarantees for wallet and LLMQ listeners and was unrelated to this PR. - Drop the nChainHeight plumbing and GetSparkBatchVerificationHeight. With ZC_MINT_CONFIRMATIONS == 1 the pinned-height cover set equals what GetCoinSet derives from the active tip at every guarded call site, so the parameter was a behavioral no-op. - Remove the now-unused BatchProofContainer::verify and make batch_spark private. - Share the batching predicate between ConnectBlock and ActivateBestChainStep via ShouldBatchSparkProofs so the collector and the guard cannot silently diverge. - Latch a failed batch (fBatchFailed) so the abort path does not re-run full proof verification at every subsequent boundary; the failed proofs are still retained until removed by disconnect. - Drop the Shutdown early-return: the final FlushStateToDisk already refuses to persist validation state under cs_main when the pending batch fails, and completing teardown avoids exiting with live network threads and an unflushed wallet. - Hold cs_main while verifying the pending batch before clearing the reindex flag in ThreadImport. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

Add a unit test that drives a real wallet-created Spark spend through the deferred-batch path: collection instead of inline verification, successful batch verification of a valid proof, fail-closed behavior with retained proofs when cover sets cannot be rebuilt, the failed batch latch, and recovery once the offending spend is removed. Add a spark_batching.py regtest that mines a Spark chain with timestamps older than one day, then reindexes with -batching=1 and asserts the node batch verifies the spends before completing the reindex (and reaches the same tip as -batching=0). Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

codeant-ai · 2026-07-26T03:57:31Z

User [email protected] does not have a PR Review subscription.

Go to Team management and add this email to the PR Review subscription.

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/batchproof_container.cpp (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use Linux braces for the new method definitions.

Move each opening function brace to its own line. As per coding guidelines, use Linux brace style: new line for function definitions.

Also applies to: 45-85
🤖 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 - 31, Update the new method
definitions in BatchProofContainer, including verify_pending and the methods in
the additional referenced range, to use Linux brace style by placing each
opening function brace on its own line. Preserve the existing method bodies and
behavior.
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 `@qa/rpc-tests/spark_batching.py`:
- Around line 36-41: Update the reindex wait loop in the batching test to
require both the expected block height and the final batch-verification success
marker when batching=True. Poll both conditions under the existing deadline
before proceeding to the final block count and hash assertions, while preserving
the current height-only behavior for non-batching cases.

In `@src/batchproof_container.h`:
- Around line 35-37: Update BatchProofContainer::remove() to clear fBatchFailed
only when the removal actually erases an element from the pending-proofs vector.
Leave the failure latch unchanged when no proof matches, while preserving the
existing proof-removal behavior.

---

Nitpick comments:
In `@src/batchproof_container.cpp`:
- Around line 28-31: Update the new method definitions in BatchProofContainer,
including verify_pending and the methods in the additional referenced range, to
use Linux brace style by placing each opening function brace on its own line.
Preserve the existing method bodies and behavior.

🪄 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 Plus

Run ID: 626b5807-bc41-4dc0-89db-a235cc5966d8

📥 Commits

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

📒 Files selected for processing (9)

qa/pull-tester/rpc-tests.py
qa/rpc-tests/spark_batching.py
src/batchproof_container.cpp
src/batchproof_container.h
src/init.cpp
src/test/CMakeLists.txt
src/test/spark_batch_test.cpp
src/validation.cpp
src/validation.h

reubenyap · 2026-07-26T04:13:47Z

@CodeAnt-AI review

codeant-ai · 2026-07-26T04:13:52Z

CodeAnt AI is running the review.

codeant-ai · 2026-07-26T04:16:25Z

CodeAnt AI finished running the review.

The fBatchFailed latch made batch_spark() fail fast after a failed verification, but remove() never cleared it. After a disconnect removed the offending spend, any remaining pending proofs kept failing fast forever, so recovery was only possible when removal emptied the batch. Clear the latch when remove() actually erases proofs: the pending batch changed, so the previous failure verdict no longer applies and the next boundary re-runs full verification. Extend spark_batch_tests with the partial-removal scenario: a batch holding a valid proof and an invalid one (a raw-parsed spend whose binding data is absent) fails and latches, and removing only the invalid spend lets the remaining valid proof verify again. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

The tip can reach the target height while the final deferred Spark batch is still pending, so asserting on the debug.log success marker right after the height check could race and flake. For a batched reindex, wait until both the expected height and the batch verification success marker are present under the same deadline. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

The rpc-tests runner executes test scripts directly, so a script registered in rpc-tests.py without the executable bit fails the whole RPC test phase with PermissionError. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

Address maintainer feedback that aborting on a failed Spark batch reproduced the old behavior's worst property: a plain restart redoes the reindex/sync and hits the same batched failure again, and the operator only learns which spend is invalid by manually reindexing with -batching=0. Two changes make the failure path actionable: - Identify offenders at failure time: the batch container now records the txid alongside each collected proof, and when the batch fails it re-verifies the retained proofs individually (cover sets are already built) and logs the exact invalid spend transactions to debug.log. - Auto-recover on plain restart: on failure the node persists a sparkbatchfailed flag in the block tree DB before aborting. On the next start LoadBlockIndexDB() reads and clears the flag and forces -batching=0 for that run, so the restart verifies Spark proofs block by block and rejects the invalid block through the normal consensus path instead of looping into the same batched abort. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

The RPC test failed in CI with "Spark spend creation failed": CreateSparkSpendTransaction() refuses to build a spend unless the anonymity set contains at least two coins, and the test minted only one. Mint two coins before spending. Also truncate debug.log before each reindex so the batch verification success marker asserted by the test can only come from the reindex run itself, not from the live mining phase or the shutdown flush that precedes it. While at it, make verify_pending() discard the in-progress collection via init() so the temporary txid vector stays in lockstep with the temporary proof vector. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b872ab9129

ℹ️ 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".

The sparkbatchfailed recovery marker was stored as a block tree DB flag and read in LoadBlockIndexDB(). As Codex review pointed out, a node whose failed run was started with -reindex and which gets restarted with the same arguments (e.g. by a process manager) never performs the recovery: -reindex wipes the block tree database, destroying the flag, and LoadBlockIndex() skips LoadBlockIndexDB() entirely, so batching stayed enabled and the run looped into the same batched failure. Store the marker as a plain datadir file instead and consume it in AppInitMain right after fReindex is determined, before any database is opened. The marker now survives the reindex wipe and disables batching for the next run regardless of how the node is restarted. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

The post-reindex balance assertions read getsparkbalance() immediately after the tip reached the target height, racing the wallet's asynchronous catch-up: a Debug CI run failed with fullBalance briefly equal to just the two spend change coins while coin metadata was still being rewritten. Poll for the expected balance with a deadline before asserting, on both the batched and block-by-block reindex paths. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44

reubenyap · 2026-07-26T08:18:38Z

CI status on 6140fc0 — the two red checks are infrastructure/flake, not related to this PR's changes:

linux-cmake-Release (run 30193325400): single failure, llmq-is-cl-conflicts.py — "wait_for_chainlock timed out". This is a timing flake: the sibling run on the exact same commit passed linux-cmake-Release fully, the other four LLMQ tests (llmq-chainlocks, llmq-cl-evospork, llmq-is-retroactive, llmq-signing) passed within the failing run itself, and this PR touches only Spark batch verification — no llmq/ or evo/ code.
guix-x86_64-linux-gnu: git.savannah.gnu.org is returning 502 on the Guix installer download (ongoing upstream outage, same as previous runs). The other guix jobs got past that step and are still building.

Everything else is green on this commit: mac + windows Release/Debug, and the full RPC suite on the passing linux-cmake-Release run (including the new spark_batching.py). linux-cmake-Debug is still in progress.

I don't have permission to re-run failed jobs, so a maintainer re-run of the two failed jobs would clear the red checks.

Generated by Claude Code

codeant-ai · 2026-08-14T15:27:56Z

🤖 CodeAnt AI — Review Status

Status	Commit	Started (UTC)	Finished (UTC)
✅ Incremental review completed	`d589a8a`	Aug 23, 2026 · 07:35	07:35
✅ Incremental review completed	`0195f12`	Aug 14, 2026 · 15:27	15:28

codeant-ai · 2026-08-14T15:28:18Z

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. In particular, a crash or restart during a long sync permanently skips verification of the proofs that were pending in memory, because the batch container does not survive the restart while the flushed chainstate does. In addition, the old throwing failure path could be swallowed entirely when batch verification was reached from the network message thread (ProcessMessages() catches and continues), making the previous failure mode effectively fail-open.

This PR makes the 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 old failure path was also unhelpful to the operator: when it did crash the node, a plain restart redid the reindex/sync and ran into the same batched failure again, and the only way to learn which spend was invalid was a manual diagnostic reindex with -batching=0.

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, and that failure gave the operator neither the offending transaction nor a working plain-restart path.

Code changes brief

Make Spark batch verification return success/failure instead of throwing from the batch verifier path; remove the now-unused BatchProofContainer::verify() and make batch_spark() private.
Add BatchProofContainer::verify_pending() to discard in-progress per-block collection and verify the pending batch explicitly.
Preserve pending Spark spends on verification failure, and latch the failure (fBatchFailed) so the abort path fails fast at later boundaries instead of re-running full proof verification; removing spends from the pending batch (e.g. by disconnect) clears the latch so the changed batch is re-verified.
Record the txid alongside each collected proof; when a batch fails, re-verify the retained proofs individually (the cover sets are already built) and log the exact invalid spend transactions to debug.log, so the operator does not need a diagnostic reindex to identify the offender.
Write a one-shot sparkbatchfailed marker file in the datadir when a pending batch fails; on the next start the node consumes it in AppInitMain (right after fReindex is determined, before any database is opened) and forces -batching=0 for that run, so a restart re-verifies Spark proofs block by block and rejects the invalid block through the normal consensus path instead of looping into the same batched abort. A datadir file rather than a block-tree DB flag so the marker survives the database wipe of a run restarted with -reindex.
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/evoDb flushes and the prune flag in FlushStateToDisk().
Require pending Spark batch verification before connecting a non-batched (recent) block in ActivateBestChainStep().
Require pending Spark batch verification (under cs_main) before clearing the reindex flag in ThreadImport().
Share the one-day batching predicate between ConnectBlock() and the new guard via ShouldBatchSparkProofs() so the collector and the guard cannot silently diverge.
Shutdown needs no separate check: the final FlushStateToDisk() is already guarded and refuses to persist validation state if the pending batch fails, while node teardown still completes cleanly.
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. Cover sets for the batch are still built with GetCoinSet() exactly as before; supersets are safe because the batch verifier slices each proof's set by its recorded size.

The failure mode is intentionally fail-closed: if a pending Spark batch fails verification, the node logs the invalid spend txids, persists the one-shot sparkbatchfailed marker, and aborts instead of persisting validation progress past that point. On the next start batching is disabled automatically for that run, so the invalid spend is rejected through normal block-by-block verification without operator intervention; restarting manually with -reindex -batching=0 remains available as a diagnostic path.

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.

Tests

spark_batch_tests (Boost unit test, src/test/spark_batch_test.cpp): drives real wallet-created Spark spends through the deferred path — collection instead of inline verification, successful batch verification of a valid proof, fail-closed behavior with retained proofs when cover sets cannot be rebuilt, the failed-batch latch, and recovery in both removal shapes: removing the only offending spend from a mixed valid/invalid batch (the remaining valid proof verifies again) and removal that empties the batch.
spark_batching.py (regtest): mines a Spark chain with timestamps older than one day, performs Spark mints and spends, then reindexes with -batching=1 and asserts the node batch-verified the spends (via debug.log) and reached the same tip as a -batching=0 reindex.

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: resolve each unique cover-set reference once per pending batch instead of once per spend, bind cached entries to the exact group/reference block data they were derived from, invalidate across disconnect/reorg and forced verification boundaries, and keep -batching=0 as the conservative diagnostic path. 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: the consensus-code diff is confined to the batch container, the VerifyPendingSparkBatch() guard and its call sites, and the failure-recovery marker consumed at startup. It does not add the larger cover-set metadata cache or broader Spark reindex acceleration yet.

Validation

git diff --check across the touched files.
src/batchproof_container.cpp syntax-checked locally; full builds, unit tests, and RPC tests run in CI (the environment used to author this cannot fetch the bls-dash/tor depends).

CodeAnt-AI Description

Fail closed on deferred Spark proof verification before validation state is saved

What Changed

Deferred Spark proofs are verified before the node writes chain state, completes reindexing, or records pruning progress.
Failed batches stop validation, retain the failed proofs, and prevent repeated verification attempts until the affected spend is removed.
Failure logs identify the invalid Spark transaction IDs, and the next restart automatically disables batching so proofs are checked individually.
Added unit and end-to-end coverage for successful batches, invalid proofs, reindexing, and recovery after removing a failed spend.

Impact

✅ No durable validation state after failed Spark verification
✅ Clearer logs identifying invalid Spark spends
✅ Safer reindex recovery after a batch failure

💡 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.

codeant-ai · 2026-08-23T07:35:44Z

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. In particular, a crash or restart during a long sync permanently skips verification of the proofs that were pending in memory, because the batch container does not survive the restart while the flushed chainstate does. In addition, the old throwing failure path could be swallowed entirely when batch verification was reached from the network message thread (ProcessMessages() catches and continues), making the previous failure mode effectively fail-open.

This PR makes the 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 old failure path was also unhelpful to the operator: when it did crash the node, a plain restart redid the reindex/sync and ran into the same batched failure again, and the only way to learn which spend was invalid was a manual diagnostic reindex with -batching=0.

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, and that failure gave the operator neither the offending transaction nor a working plain-restart path.

Code changes brief

Make Spark batch verification return success/failure instead of throwing from the batch verifier path; remove the now-unused BatchProofContainer::verify() and make batch_spark() private.
Add BatchProofContainer::verify_pending() to discard in-progress per-block collection and verify the pending batch explicitly.
Preserve pending Spark spends on verification failure, and latch the failure (fBatchFailed) so the abort path fails fast at later boundaries instead of re-running full proof verification; removing spends from the pending batch (e.g. by disconnect) clears the latch so the changed batch is re-verified.
Record the txid alongside each collected proof; when a batch fails, re-verify the retained proofs individually (the cover sets are already built) and log the exact invalid spend transactions to debug.log, so the operator does not need a diagnostic reindex to identify the offender.
Write a one-shot sparkbatchfailed marker file in the datadir when a pending batch fails; on the next start the node consumes it in AppInitMain (right after fReindex is determined, before any database is opened) and forces -batching=0 for that run, so a restart re-verifies Spark proofs block by block and rejects the invalid block through the normal consensus path instead of looping into the same batched abort. A datadir file rather than a block-tree DB flag so the marker survives the database wipe of a run restarted with -reindex.
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/evoDb flushes and the prune flag in FlushStateToDisk().
Require pending Spark batch verification before connecting a non-batched (recent) block in ActivateBestChainStep().
Require pending Spark batch verification (under cs_main) before clearing the reindex flag in ThreadImport().
Share the one-day batching predicate between ConnectBlock() and the new guard via ShouldBatchSparkProofs() so the collector and the guard cannot silently diverge.
Shutdown needs no separate check: the final FlushStateToDisk() is already guarded and refuses to persist validation state if the pending batch fails, while node teardown still completes cleanly.
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. Cover sets for the batch are still built with GetCoinSet() exactly as before; supersets are safe because the batch verifier slices each proof's set by its recorded size.

The failure mode is intentionally fail-closed: if a pending Spark batch fails verification, the node logs the invalid spend txids, persists the one-shot sparkbatchfailed marker, and aborts instead of persisting validation progress past that point. On the next start batching is disabled automatically for that run, so the invalid spend is rejected through normal block-by-block verification without operator intervention; restarting manually with -reindex -batching=0 remains available as a diagnostic path.

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.

Tests

spark_batch_tests (Boost unit test, src/test/spark_batch_test.cpp): drives real wallet-created Spark spends through the deferred path — collection instead of inline verification, successful batch verification of a valid proof, fail-closed behavior with retained proofs when cover sets cannot be rebuilt, the failed-batch latch, and recovery in both removal shapes: removing the only offending spend from a mixed valid/invalid batch (the remaining valid proof verifies again) and removal that empties the batch.
spark_batching.py (regtest): mines a Spark chain with timestamps older than one day, performs Spark mints and spends, then reindexes with -batching=1 and asserts the node batch-verified the spends (via debug.log) and reached the same tip as a -batching=0 reindex.

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: resolve each unique cover-set reference once per pending batch instead of once per spend, bind cached entries to the exact group/reference block data they were derived from, invalidate across disconnect/reorg and forced verification boundaries, and keep -batching=0 as the conservative diagnostic path. 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: the consensus-code diff is confined to the batch container, the VerifyPendingSparkBatch() guard and its call sites, and the failure-recovery marker consumed at startup. It does not add the larger cover-set metadata cache or broader Spark reindex acceleration yet.

Validation

git diff --check across the touched files.
src/batchproof_container.cpp syntax-checked locally; full builds, unit tests, and RPC tests run in CI (the environment used to author this cannot fetch the bls-dash/tor depends).

CodeAnt-AI Description

Prevent unverified Spark batches from being persisted

What Changed

Deferred Spark proofs are verified before chainstate, block-index, pruning, or reindex completion state is written.
A failed batch stops validation, retains the failed proofs, identifies affected transactions in the debug log, and prevents repeated verification until the batch changes.
After restart, batching is disabled automatically so invalid spends are checked individually through the normal validation path.
Added unit and RPC coverage for successful batches, failed batches, reindexing, and recovery after removing invalid spends.

Impact

✅ No durable validation state after failed Spark verification
✅ Automatic block-by-block recovery after restart
✅ Clearer logs identifying 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.

* Qt: split Spark payments into single-input transactions Plan ordinary private payments as one transaction per Spark coin, commit the resulting batch sequentially, restrict name registration to one fundable coin, and disable the exchange-address flow that requires a single first-stage transaction ID. * Qt: harden single-input Spark batch spending Use a compiled and unit-tested single-input planner, preserve explicit Coin Control semantics, release global locks before proof generation, and revalidate each selected outpoint during construction. Make batch fees, linkability, limits, and partial-commit behavior explicit in the interface and error reporting. * Spark: add versioned Chaum proofs Preserve the deployed proof serialization as V1 and add a componentwise V2 representation with a distinct transcript domain. Use explicit versioned verifier names, enforce protocol-sized dimensions, and keep the Chaum fuzz harnesses bounded and internally consistent. * Consensus: activate versioned Spark spends Add an explicitly versioned transaction type and bounded payload parser for V2 spends. Gate consensus, mempool, mining, and wallet selection on a disabled-by-default deployment height while preserving historical V1 validation. Exercise cross-version rejection through the transaction-level parser selected by consensus. * Fuzz: cover canonical Spark V2 payloads Exercise bounded V2 payload parsing across version, dimension, truncation, and trailing-data failures. Require every accepted payload to consume the complete stream and serialize back to the exact canonical bytes. * Consensus: bind canonical Spark V2 context Commit proofs to canonical outputs, amounts, extension data, and the exact cover-set reference map. Enforce canonical Spark Name suffixes and keep transaction classification, relay, mining, wallet construction, RPC, and display paths consistent with the versioned type. * Wallet: bind Spark construction to one tip Hold the chain and wallet locks while selecting the spend format and coins. Prepare Spark Name metadata from the same chain snapshot, require the expected next-block height, and reject construction if the tip changes before completion. * Validation: verify Spark batches before state commit Finish per-block batch verification before committing chain state and clear all accumulated proofs on every exit path. Reject unknown active-format cover-set references while retaining the historical fallback, with non-punitive mempool policy handling. * Validation: isolate VerifyDB reconnection state Recheck Spark proofs during level-4 VerifyDB reconnects while advancing only temporary coin and Evo views. Suppress persistent Spark, masternode, quorum, notification, mining, and cache mutations and roll back transient spork state on every exit. * P2P: keep Spark Name policy failures non-punitive Classify malformed Spark Name payloads received through mempool admission as zero-score policy failures while retaining ordinary invalid-block scoring for authoritative validation. Cover both direct admission and the generic transaction-check path. * HF block set * Batch verifier changes reverted * more merge issues resolved * Unittests fixed * Tests: harden Spark review fallout and restore master proof cache * Drop Spark mint uniqueness checks, prefere PR1902 * one more unittest fix * Ai review comments resolved, rpc test fixed * More comments resolved * More minor comments resolved * Consensus: reject duplicate Spark mint coins (#1902) * Consensus: reject duplicate Spark mints Reject duplicate Spark mint coins within a block or against the active chain before mutating Spark state. Track mint reservations per transaction pool so duplicate transactions are rejected and conflicts are evicted symmetrically. * Consensus: harden duplicate mint reorgs Validate completed Spark mint sets before later ConnectBlock side effects, and use stored mint heights so historical verification can distinguish an earlier duplicate from its own replay.\n\nPreserve older mint occurrences when disconnecting legacy duplicate indexes, balance their metadata, use conflict removal notifications, and add full block, mempool, rebuild, same-group, cross-group, and same-block regressions. --------- Co-authored-by: levonpetrosyan93 <[email protected]> * revert removed check, comments added * Spark: synchronize minted coin state (#1910) * Wallet: Fix deadlock between walletpassphrase and the lockwallet relock timer (#1889) * Fix deadlock between walletpassphrase and the wallet relock timer RPCRunLater() erases any previously scheduled lockwallet timer, and destroying a libevent timer blocks until that timer's callback has finished executing. walletpassphrase() called it while still holding LOCK2(cs_main, pwallet->cs_wallet), so when the LockWallet() callback happened to be running at that moment it was itself blocked acquiring cs_wallet, and the two threads deadlocked: the RPC worker waiting on libevent, the HTTP event loop waiting on cs_wallet. Because the RPC worker also holds cs_main, every thread that needs cs_main then piles up behind it (net message handling, the scheduler, connection handling) and the node stops making progress entirely. Nothing is logged, and SIGTERM does not complete either, since the shutdown path needs the same locks. Scope the lock so it is released before scheduling the relock, and pass the expected nRelockTime into LockWallet() so an obsolete callback that fires after the wallet was re-unlocked or already locked does nothing. * Serialize walletpassphrase and guard the RPC timer map Moving the relock scheduling out of the cs_wallet scope also removed the serialization that cs_main used to provide for the two things walletpassphrase does after a successful unlock: publishing pwallet->nRelockTime and installing the lockwallet timer. RPCRunLater() can now be entered concurrently by up to -rpcthreads (default 4) HTTP worker threads, and both steps are racy: - deadlineTimers in rpc/server.cpp is a plain std::map that RPCRunLater() erases from and emplaces into, and that StopRPC() clears. Nothing serialized those accesses other than walletpassphrase happening to hold cs_main, so concurrent calls can now corrupt the map. - Two overlapping walletpassphrase calls can publish nRelockTime and schedule the timer in opposite orders. The slower call then installs the surviving timer while the faster call owns nRelockTime; when that timer fires, the generation check in LockWallet() correctly rejects it, no other timer is left, and the wallet stays unlocked past the requested timeout. Add a per-wallet cs_unlock that walletpassphrase holds across both the unlock and RPCRunLater(), so the two steps are atomic with respect to each other. It deliberately is not cs_wallet: the lock is held across RPCRunLater(), which blocks until a running relock callback returns, and that callback takes cs_wallet - which is exactly the deadlock fixed in the previous commit. Give deadlineTimers its own cs_deadlineTimers as well, so the map stays safe regardless of which RPC calls RPCRunLater() and independently of the fact that this build only ever has one wallet. It also closes the existing race between StopRPC() and an in-flight walletpassphrase, as StopRPC() runs before the HTTP worker threads have been joined. Neither new lock is acquired by any timer callback, so the resulting lock order (cs_unlock -> cs_main -> cs_wallet, and cs_unlock -> cs_deadlineTimers) has no cycle and cannot reintroduce the deadlock. * Validate walletpassphrase timeout before unlock Reject a negative timeout with RPC_INVALID_PARAMETER instead of accepting it and scheduling a relock in the past, and clamp the timeout to 100000000 seconds so GetTime() + nSleepTime cannot overflow nRelockTime. Both checks run before the wallet is unlocked, so a rejected call leaves the wallet state untouched. This mirrors the validation upstream Bitcoin Core performs in walletpassphrase (the negative-timeout error and MAX_SLEEP_TIME clamp). Also cover the negative-timeout rejection in wallet-encryption.py, including that the wallet stays locked afterwards. Follow-up to #1889. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UTd2DsfD7pTdvxDbubqASZ --------- Co-authored-by: Claude <[email protected]> * Spark: synchronize minted coin state Protect the active minted-coin map with a dedicated short-lived lock so asynchronous wallet scans cannot race block connection or disconnection. Return snapshots to callers instead of exposing map references. * Spark: preserve legacy duplicate mints on disconnect --------- Co-authored-by: Danswar <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: levonpetrosyan93 <[email protected]> * Build fix * Review commets resolved * Consensus: validate Spark coin output types (#1901) * Consensus: validate Spark coin output types * Validation: contextualize Spark block checks Defer the preliminary body check for Spark type mismatches until AcceptBlock has validated the header and established its indexed height. This prevents unknown-parent and context-invalid blocks from reaching legacy Spark parsing while preserving historical block behavior. Add regressions for unknown parents, contextual difficulty, activated mismatches, stale unsolicited forks, and the INT_MAX caller sentinel. * Tests: qualify chain parameters * Consensus: enforce Spark coin types at Chaum V2 height Drop the unused nSparkCoinTypeFixStartBlock and reuse nSparkChaumV2StartBlock so opcode/coin-type pairs activate with the existing hard fork. --------- Co-authored-by: levonpetrosyan93 <[email protected]> * Build fix * Consensus: Fail closed on Spark batch verification (#1863) * Spark: fail closed on deferred batch verification * Consensus: address Spark batch feedback * Docs: document Spark batch APIs * Consensus: address remaining Spark batch feedback * Consensus: simplify Spark batch fail-closed boundary Review follow-up that keeps the fail-closed invariant while cutting the surface of the change: - Restore the connected-block SyncTransaction loop to its original position under cs_main in ActivateBestChain; moving it out of the lock changed notification ordering guarantees for wallet and LLMQ listeners and was unrelated to this PR. - Drop the nChainHeight plumbing and GetSparkBatchVerificationHeight. With ZC_MINT_CONFIRMATIONS == 1 the pinned-height cover set equals what GetCoinSet derives from the active tip at every guarded call site, so the parameter was a behavioral no-op. - Remove the now-unused BatchProofContainer::verify and make batch_spark private. - Share the batching predicate between ConnectBlock and ActivateBestChainStep via ShouldBatchSparkProofs so the collector and the guard cannot silently diverge. - Latch a failed batch (fBatchFailed) so the abort path does not re-run full proof verification at every subsequent boundary; the failed proofs are still retained until removed by disconnect. - Drop the Shutdown early-return: the final FlushStateToDisk already refuses to persist validation state under cs_main when the pending batch fails, and completing teardown avoids exiting with live network threads and an unflushed wallet. - Hold cs_main while verifying the pending batch before clearing the reindex flag in ThreadImport. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Tests: add Spark batch verification coverage Add a unit test that drives a real wallet-created Spark spend through the deferred-batch path: collection instead of inline verification, successful batch verification of a valid proof, fail-closed behavior with retained proofs when cover sets cannot be rebuilt, the failed batch latch, and recovery once the offending spend is removed. Add a spark_batching.py regtest that mines a Spark chain with timestamps older than one day, then reindexes with -batching=1 and asserts the node batch verifies the spends before completing the reindex (and reaches the same tip as -batching=0). Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Consensus: reset Spark batch failure latch on removal The fBatchFailed latch made batch_spark() fail fast after a failed verification, but remove() never cleared it. After a disconnect removed the offending spend, any remaining pending proofs kept failing fast forever, so recovery was only possible when removal emptied the batch. Clear the latch when remove() actually erases proofs: the pending batch changed, so the previous failure verdict no longer applies and the next boundary re-runs full verification. Extend spark_batch_tests with the partial-removal scenario: a batch holding a valid proof and an invalid one (a raw-parsed spend whose binding data is absent) fails and latches, and removing only the invalid spend lets the remaining valid proof verify again. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Tests: wait for batch marker in spark_batching.py The tip can reach the target height while the final deferred Spark batch is still pending, so asserting on the debug.log success marker right after the height check could race and flake. For a batched reindex, wait until both the expected height and the batch verification success marker are present under the same deadline. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Tests: make spark_batching.py executable The rpc-tests runner executes test scripts directly, so a script registered in rpc-tests.py without the executable bit fails the whole RPC test phase with PermissionError. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Consensus: improve Spark batch failure recovery Address maintainer feedback that aborting on a failed Spark batch reproduced the old behavior's worst property: a plain restart redoes the reindex/sync and hits the same batched failure again, and the operator only learns which spend is invalid by manually reindexing with -batching=0. Two changes make the failure path actionable: - Identify offenders at failure time: the batch container now records the txid alongside each collected proof, and when the batch fails it re-verifies the retained proofs individually (cover sets are already built) and logs the exact invalid spend transactions to debug.log. - Auto-recover on plain restart: on failure the node persists a sparkbatchfailed flag in the block tree DB before aborting. On the next start LoadBlockIndexDB() reads and clears the flag and forces -batching=0 for that run, so the restart verifies Spark proofs block by block and rejects the invalid block through the normal consensus path instead of looping into the same batched abort. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Tests: fix spark_batching.py spend setup The RPC test failed in CI with "Spark spend creation failed": CreateSparkSpendTransaction() refuses to build a spend unless the anonymity set contains at least two coins, and the test minted only one. Mint two coins before spending. Also truncate debug.log before each reindex so the batch verification success marker asserted by the test can only come from the reindex run itself, not from the live mining phase or the shutdown flush that precedes it. While at it, make verify_pending() discard the in-progress collection via init() so the temporary txid vector stays in lockstep with the temporary proof vector. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Consensus: survive -reindex restart in batch failure recovery The sparkbatchfailed recovery marker was stored as a block tree DB flag and read in LoadBlockIndexDB(). As Codex review pointed out, a node whose failed run was started with -reindex and which gets restarted with the same arguments (e.g. by a process manager) never performs the recovery: -reindex wipes the block tree database, destroying the flag, and LoadBlockIndex() skips LoadBlockIndexDB() entirely, so batching stayed enabled and the run looped into the same batched failure. Store the marker as a plain datadir file instead and consume it in AppInitMain right after fReindex is determined, before any database is opened. The marker now survives the reindex wipe and disables batching for the next run regardless of how the node is restarted. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 * Tests: wait for wallet to settle after reindex in spark_batching.py The post-reindex balance assertions read getsparkbalance() immediately after the tip reached the target height, racing the wallet's asynchronous catch-up: a Debug CI run failed with fullBalance briefly equal to just the two spend change coins while coin metadata was still being rewritten. Poll for the expected balance with a deadline before asserting, on both the batched and block-by-block reindex paths. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Ci8HSbWLZHxA1CXVPKdL44 --------- Co-authored-by: Claude <[email protected]> Co-authored-by: levonpetrosyan93 <[email protected]> * Merge conflicts and leftover bugs fixed * Unittest fixed * Consensus: Fix Spark group ID aliasing (#1907) * Consensus: Fix Spark group ID aliasing * Spark: avoid copying spend block hash map Bind the result of getBlockHashes() to a const reference instead of copying the std::map<uint64_t, uint256> on every Spark spend validation. The accessor returns a const reference into the spend object, which outlives all uses, and the map is only read afterwards, matching the reference binding already used for getCoinGroupIds(). Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_01848eNswZ82pAqdJcFA4Zi6 * Spark: avoid copy in batch cover-set caching Replace the count()-then-operator[] insertion in both batch_spark cover-set cache loops with a find() guard and emplace of the moved vector. This drops a default construction plus full copy of each retrieved cover set and follows the project guideline of using find() instead of operator[] on maps. GetCoinSet usage is unchanged. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_01848eNswZ82pAqdJcFA4Zi6 * Consensus: Gate canonical Spark group IDs on Chaum V2 Drop the separate nSparkCanonicalGroupIdStartBlock height and enforce canonical 32-bit cover-set IDs at nSparkChaumV2StartBlock instead. --------- Co-authored-by: Claude <[email protected]> Co-authored-by: levonpetrosyan93 <[email protected]> Co-authored-by: levonpetrosyan93 <[email protected]> * Spark: fix out-of-range vector writes in SpendKey derivation (#1894) * Tests: pin Spark spend key derivation of s1 and s2 The derivation of (s1, s2) from r in SpendKey is consensus-critical for wallets: any change alters every wallet's view keys and addresses, orphaning previously received funds. The deployed derivation is s1 = memberFromSeed(SHA256d("s1_generation" || ser(r))) s2 = memberFromSeed(SHA256d("s2_generation")) where the s2 seed intentionally commits only to its prefix, so s2 is identical for every spend key. PR #1767 accidentally changed this by keeping the serialized s1 in the hashed buffer, and was reverted in PR #1784; PR #1893 proposed the same change again. This test pins the deployed derivation so any future drift fails CI: - determinism of (s1, s2) in r - s1 depends on r while s2 does not - both scalars match an explicit re-derivation via CHash256 refs #1893 Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HVp6JH7LvzRpZXU4VEjdDG * Spark: fix out-of-range vector writes in SpendKey SpendKey::SpendKey(params, r) cleared its hash input and output vectors and then kept writing through data()/&result[0], accessing storage outside the vectors' live element range. This is undefined behavior (and asserts in MSVC debug builds), although in practice the writes stayed inside the retained capacity of the same allocations, so it never corrupted memory in release builds. Keep both buffers at their required sizes for the whole derivation and wipe them with memory_cleanse() before scope exit instead of clear(), so sensitive intermediate material is not left behind in freed heap memory. Behavior is byte-for-byte identical to the deployed derivation: the cleared `data` vector previously contributed zero bytes to the s2 seed hash, so the s2 seed commits only to the "s2_generation" prefix. The dead s1.serialize() into that buffer is dropped and the invariant is documented in place; the spend_key_derivation unit test pins it. Unlike #1767 (reverted in #1784) and #1893, this does not change the derived s2, so existing wallets keep their view keys and addresses. refs #1893 Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HVp6JH7LvzRpZXU4VEjdDG --------- Co-authored-by: Claude <[email protected]> * Unittests fixed * Guix build fixed * Spark: bound cover-set reference work * Wallet: Remove Spark mints and restore spends on reorg (#1895) * Wallet: Roll back Spark coins on block disconnect DisconnectTip fed the wallet rollback with a stateless CheckTransaction re-run over the transactions of the disconnected block, which cannot produce usable data for the wallet: - Since #1873 CheckSparkSpendTransaction returns before the SMint outputs are collected when fStatefulSigmaCheck is false, so mints created by spend transactions (private change) never reach block.sparkTxInfo and RemoveSparkMints() is called with nothing. - The coins that path does collect are built by ParseSparkMintCoin() alone, and Coin::serial_context is not serialized. Spend-created coins are bound to the spent linking tags and pure mints to the serialized inputs, so without the serial context Coin::identify() fails the serial commitment check, throws, and RemoveSparkMints() skips the coin. The rollback of pure mints has therefore never worked either. - spentLTags is filled behind the same stateful gate, so RemoveSparkSpends() has always been given an empty set and the wallet side of a spend was never rolled back at all. Take the rollback data from the transactions of the block instead. spark::GetSparkMintCoins() parses the coins and binds them to the serial context of their transaction, covering spend-created SMints as well as pure mints, and spark::GetSparkUsedTags() returns the linking tags of a spend. Transactions that DisconnectTip resurrected into the pools are skipped: they still exist, only unconfirmed again, their coins are still the wallet's and their inputs are still spent, and pool acceptance has re-registered both. Only what did not make it back is rolled back. RemoveSparkSpends() now takes the linking tags directly. The group ids of its unordered_map parameter were never read and its only caller passed block.sparkTxInfo->spentLTags; AbandonSpends(), which had the same body, forwards to it. The regression test mints coins, spends part of one so that the change comes back as an SMint output of the spend, mines the spend and disconnects that block with the spend conflicting in the pools, the way a competing spend on the new chain would leave it. It asserts on wallet state, not on the collected coins, because Coin::operator== ignores serial_context. fixes #1887 * P2P: Set ban score on two Spark spend checks The mixed mint check and the private output limit check in CheckSparkSpendTransaction() returned a bare false without touching the validation state. Both only run in the stateful path today, where ConnectBlock's wrapper happens to re-wrap the failure in DoS(100) and mempool acceptance rejects the transaction with no reject reason and no penalty for the peer that relayed it. Set the score and a descriptive reject reason at the check itself, so the rejection is self-describing and the peer penalty stays correct if these checks ever run in the stateless path again. No block or transaction changes validity, only reject metadata and ban score. * Wallet: Hold cs_spark_wallet across RemoveSparkMints The erase of each mint took the lock through eraseMint(), but the loop around them did not, so a removal was not atomic against the wallet jobs running on the thread pool. Take the lock for the whole loop, the way RemoveSparkSpends() and AbandonSpends() already do. The lock is recursive, so the nested eraseMint() is unaffected, and the order stays the established cs_wallet -> cs_spark_wallet of AddToWallet() -> HandleSparkTransaction(). * Wallet: Open the wallet DB once in RemoveSparkSpends CWalletDB was constructed inside the loop, so a rollback opened and closed a handle once per linking tag. Hoist it above the loop, the way UpdateMintStateFromBlock() already does. This function only started receiving a non-empty set with the previous commit, so the per-tag construction had never run in the reorg path before. Read coinMeta through a single find() instead of count() followed by operator[], per the map access guideline in doc/developer-notes.md. The count() guard meant the read never inserted a default; this only drops the second lookup. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UQdhRWx2azweZvrcHDk33y --------- Co-authored-by: Claude <[email protected]> Co-authored-by: levonpetrosyan93 <[email protected]> * Tests: avoid replacing global Spark state * Tests: use valid Spark group topology * Qt: restore Rosen Bridge OP_RETURN * Tests: preserve legacy Spark reference fixture * Include first mint block in cover-set hashes * Unittest fixed * Spark: H2 cover-set hash policy and cache clear - Clear spend-proof cache when HF-1 connects - Mempool-only ATMP gate (null info + INT_MAX) - Require 32-byte cover-set hash: mempool H2-10, consensus H2 - Wallet refuses unbound cover sets from H2-10 - Tests for rollover/cache/policy and wallet reject * Qt: report partial Spark mint commits * HF block set * Unittest fixed * Review comments resolved - Skip the txid-only spend-proof cache during VerifyDB - Relay only when -walletbroadcast=1; fCheckTransaction still accepts to mempool - Spark spend commits keep mempool checks without forcing relay - Show Spark fee-mismatch amounts in the GUI display unit * Unittest fixed * More review comments resolved - After a Spark batch failure, disable batching and force -reindex - Drop leftover Spark spends at H2-1 that fail H2 consensus - Record LTags on the admitting pool and evict conflicts from mempool and stem - Drop queued Spark wallet updates before disconnect resurrection - Fail mint commits on mempool rejection and keep one serialization per mint tx - Report a partial mint failure from commits that landed, not prepared tx count * Unittests fixed * Restore master batch-verify timing * Unittest fixed * Review comments resolved - Verify the terminal Spark batch before clearing the reindex flag - Switch and verify the pending batch under cs_main - Keep sparkbatchfailed until a successful -reindex -batching=0 run - Skip only dead Spark wallet updates instead of a global epoch * Making rpc test dip3-deterministicmns.py stable * Review comments resolved - Write sparkbatchfailed when batch collection starts; remove after success - Re-check mempool/chain under the wallet write locks - Run Spark mint/spend wallet jobs on one worker - Zap Spark mints and spends on -reindex from scratch * Spark: avoid wallet shutdown in state constructor --------- Co-authored-by: sneurlax <[email protected]> Co-authored-by: Reuben Yap <[email protected]> Co-authored-by: Danswar <[email protected]> Co-authored-by: Claude <[email protected]>

reubenyap force-pushed the codex/spark-batch-fail-closed branch from e5cc472 to 629f1d6 Compare June 15, 2026 03:48

Spark: fail closed on deferred batch verification

a4d9f1a

reubenyap force-pushed the codex/spark-batch-fail-closed branch from 629f1d6 to a4d9f1a Compare June 15, 2026 08:17

reubenyap marked this pull request as ready for review June 15, 2026 08:29

coderabbitai Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread src/batchproof_container.cpp Resolved

Comment thread src/validation.cpp Resolved

chatgpt-codex-connector Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread src/validation.cpp Resolved

Comment thread src/batchproof_container.cpp Resolved

Comment thread src/validation.cpp Resolved

Consensus: address Spark batch feedback

a92b713

codeant-ai Bot added the size:M This PR changes 30-99 lines, ignoring generated files label Jun 17, 2026

codeant-ai Bot reviewed Jun 17, 2026

View reviewed changes

Docs: document Spark batch APIs

5ede8cd

codeant-ai Bot reviewed Jun 17, 2026

View reviewed changes

Comment thread src/init.cpp Resolved

coderabbitai Bot reviewed Jun 17, 2026

View reviewed changes

Consensus: address remaining Spark batch feedback

c605406

coderabbitai Bot requested review from levonpetrosyan93 and psolstice June 17, 2026 17:42

claude added 2 commits July 26, 2026 03:56

coderabbitai Bot requested a review from levoncrypto July 26, 2026 03:58

coderabbitai Bot added the Spark label Jul 26, 2026

coderabbitai Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread qa/rpc-tests/spark_batching.py Resolved

Comment thread src/batchproof_container.h Resolved

reubenyap marked this pull request as draft July 26, 2026 04:13

claude added 3 commits July 26, 2026 04:20

claude added 2 commits July 26, 2026 04:32

reubenyap marked this pull request as ready for review July 26, 2026 06:52

chatgpt-codex-connector Bot reviewed Jul 26, 2026

View reviewed changes

Comment thread src/validation.cpp Resolved

claude added 2 commits July 26, 2026 07:03

coderabbitai Bot mentioned this pull request Aug 11, 2026

Allow only single input Spark #1899

Merged

Merge master into Spark batch fail-closed

0195f12

codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:M This PR changes 30-99 lines, ignoring generated files labels Aug 14, 2026

coderabbitai Bot mentioned this pull request Aug 20, 2026

Consensus: Fix Spark group ID aliasing #1907

Merged

levonpetrosyan93 changed the base branch from master to h2 August 23, 2026 07:32

Merge branch 'h2' into codex/spark-batch-fail-closed

d589a8a

codeant-ai Bot removed the size:L This PR changes 100-499 lines, ignoring generated files label Aug 23, 2026

levonpetrosyan93 merged commit cac6504 into h2 Aug 23, 2026
1 check passed

codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Aug 23, 2026

justanwar deleted the codex/spark-batch-fail-closed branch August 27, 2026 09:45

Conversation

reubenyap commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR intention

Why this is desirable

Existing behavior this avoids

Code changes brief

Security and correctness notes

Tests

Intended follow-up speed optimization

Scope

Validation

Uh oh!

coderabbitai Bot commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Reviews paused

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codeant-ai Bot commented Jun 17, 2026

User description

PR intention

Why this is desirable

Existing behavior this avoids

Code changes brief

Security and correctness notes

Intended follow-up speed optimization

Scope

Validation

CodeAnt-AI Description

What Changed

Impact

Checking Your Pull Request

Talking to CodeAnt AI

Example

Preserve Org Learnings with CodeAnt

Example

Retrigger review

Check Your Repository Health

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codeant-ai Bot commented Jul 26, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

reubenyap commented Jul 26, 2026

Uh oh!

codeant-ai Bot commented Jul 26, 2026

Uh oh!

codeant-ai Bot commented Jul 26, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

reubenyap commented Jun 13, 2026 •

edited

Loading

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

codeant-ai Bot commented Aug 14, 2026 •

edited

Loading