Skip to content

Consensus: Fail closed on Spark batch verification #1863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
reubenyap wants to merge 1 commit into
base: master
Choose a base branch
from
+48 −14
Open

Consensus: Fail closed on Spark batch verification #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/batchproof_container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,23 @@ void BatchProofContainer::init() {
void BatchProofContainer::finalize() {
if (fCollectProofs) {
sparkTransactions.insert(sparkTransactions.end(), tempSparkTransactions.begin(), tempSparkTransactions.end());
tempSparkTransactions.clear();
}
fCollectProofs = false;
}

void BatchProofContainer::verify() {
if (!fCollectProofs) {
batch_spark();
bool BatchProofContainer::verify() {
if (fCollectProofs) {
fCollectProofs = false;
return true;
}
fCollectProofs = false;

return batch_spark();
}

bool BatchProofContainer::verify_pending() {
finalize();

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Don't promote proofs from failed block connections

verify_pending() can be called from shutdown or later flush paths after ConnectBlock() has returned early. Since ConnectBlock() only calls batchProofContainer->finalize() at its successful end (src/validation.cpp:2971), an old block that collected Spark spends and then failed a later validation check leaves those spends in tempSparkTransactions; this unconditional finalize() promotes proofs from the rejected block into the global pending batch. A subsequent forced verification can then abort the node because of a proof that was never part of an accepted block, so stale temp proofs should be cleared on failed connections or only finalized by the successful connect path.

Useful? React with 👍 / 👎.

return batch_spark();
}

void BatchProofContainer::add(const spark::SpendTransaction& tx) {
Expand All @@ -42,12 +50,12 @@ void BatchProofContainer::remove(const spark::SpendTransaction& tx) {
sparkTransactions.end());
}

void BatchProofContainer::batch_spark() {
bool BatchProofContainer::batch_spark() {
if (!sparkTransactions.empty()){
LogPrintf("Spark batch verification started.\n");
uiInterface.UpdateProgressBarLabel("Batch verifying Spark Proofs...");
} else {
return;
return true;
}

std::unordered_map<uint64_t, std::vector<spark::Coin>> cover_sets;
Expand Down Expand Up @@ -75,10 +83,11 @@ void BatchProofContainer::batch_spark() {

if (!passed) {
LogPrintf("Spark batch verification failed.");
throw std::invalid_argument("Spark batch verification failed, please run Firo with -reindex -batching=0");
return false;
Comment on lines 84 to +86

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing newline in log message.

The failure log message on line 85 is missing a trailing \n, unlike the other LogPrintf calls in this function (lines 55 and 90).

🔧 Suggested fix 
     if (!passed) {
-        LogPrintf("Spark batch verification failed.");
+        LogPrintf("Spark batch verification failed.\n");
         return false;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!passed) {
LogPrintf("Spark batch verification failed.");
throw std::invalid_argument("Spark batch verification failed, please run Firo with -reindex -batching=0");
return false;
if (!passed) {
LogPrintf("Spark batch verification failed.\n");
return false;
🤖 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 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.

}

if (!sparkTransactions.empty())
LogPrintf("Spark batch verification finished successfully.\n");
sparkTransactions.clear();
}
return true;
}
5 changes: 3 additions & 2 deletions src/batchproof_container.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ class BatchProofContainer {

void finalize();

void verify();
bool verify();
bool verify_pending();

void add(const spark::SpendTransaction& tx);
void remove(const spark::SpendTransaction& tx);
void batch_spark();
bool batch_spark();
public:
bool fCollectProofs = 0;

Expand Down
11 changes: 8 additions & 3 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
#include "validationinterface.h"
#include "validation.h"
#include "mtpstate.h"
#include "batchproof_container.h"
#include <crypto/progpow/include/ethash/progpow.hpp>
#include "leveldb/env.h"

Expand Down Expand Up @@ -264,8 +263,9 @@ void Shutdown()
StopHTTPServer();
llmq::StopLLMQSystem();

BatchProofContainer::get_instance()->finalize();
BatchProofContainer::get_instance()->verify();
CValidationState sparkBatchState;
if (!VerifyPendingSparkBatch(sparkBatchState, "shutdown"))
LogPrintf("Shutdown continuing after Spark batch verification failure: %s\n", FormatStateMessage(sparkBatchState));

#ifdef ENABLE_WALLET
if (pwalletMain)
Expand Down Expand Up @@ -761,6 +761,11 @@ void ThreadImport(std::vector <boost::filesystem::path> vImportFiles) {
LoadExternalBlockFile(chainparams, file, &pos);
nFile++;
}
CValidationState state;
if (!VerifyPendingSparkBatch(state, "clearing reindex flag")) {
LogPrintf("Reindexing stopped before clearing reindex flag: %s\n", FormatStateMessage(state));
return;
}
pblocktree->WriteReindexing(false);
fReindex = false;
LogPrintf("Reindexing finished\n");
Expand Down
19 changes: 18 additions & 1 deletion src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2234,6 +2234,17 @@ bool AbortNode(CValidationState& state, const std::string& strMessage, const std
return state.Error(strMessage);
}

bool VerifyPendingSparkBatch(CValidationState& state, const std::string& reason)
{
BatchProofContainer* batchProofContainer = BatchProofContainer::get_instance();
if (!batchProofContainer->verify_pending()) {
return AbortNode(state,
strprintf("Spark batch verification failed before %s", reason),
_("Spark batch verification failed. Please restart with -reindex -batching=0 to identify the invalid Spark spend."));
}
return true;
}

enum DisconnectResult
{
DISCONNECT_OK, // All good.
Expand Down Expand Up @@ -3084,6 +3095,8 @@ bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode, int n
bool fPeriodicFlush = mode == FLUSH_STATE_PERIODIC && nNow > nLastFlush + (int64_t)DATABASE_FLUSH_INTERVAL * 1000000;
// Combine all conditions that result in a full cache flush.
bool fDoFullFlush = (mode == FLUSH_STATE_ALWAYS) || fCacheLarge || fCacheCritical || fPeriodicFlush || fFlushForPrune;
if ((fDoFullFlush || fPeriodicWrite) && !VerifyPendingSparkBatch(state, "flushing block index or chainstate"))

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Defer Spark batch checks until the tip is active

When this guard is reached from ConnectTip() via FlushStateToDisk(state, FLUSH_STATE_IF_NEEDED), it runs before UpdateTip(pindexNew, chainparams) advances chainActive (checked src/validation.cpp:3421 and :3430). The batch verifier rebuilds cover sets through CSparkState::GetCoinSet(), which caps them using the current chainActive.Height() (src/spark/state.cpp:1478-1483), so a cache-critical flush while connecting an old Spark block can verify against a one-block-stale active height and reject a valid spend whose cover-set reference is the newest confirmed set for the block being connected. This makes valid reindex/IBD runs abort depending on cache pressure; run the forced batch check only after the new tip is active, or rebuild the cover set from the block context being flushed.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Move verification before writing the pruned flag

When pruning is triggered with a pending Spark batch, this guard runs after the pruning branch has already persisted prunedblockfiles via pblocktree->WriteFlag("prunedblockfiles", true) and set fHavePruned (src/validation.cpp:3065-3069). If the Spark batch then fails, the node still leaves that durable flag behind even though it stops before unlinking files or flushing the verified state; on the next startup, fHavePruned && !fPruneMode forces the user into a reindex path (src/init.cpp:1985-1989). To keep the fail-closed boundary from writing validation metadata first, verify the pending batch before the pruning code writes the block-tree flag.

Useful? React with 👍 / 👎.

return false;
// Write blocks and block index to disk.
if (fDoFullFlush || fPeriodicWrite) {
// Depend on nMinDiskSpace to ensure we can write block index
Expand Down Expand Up @@ -3798,7 +3811,11 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
// Do batch verification if we reach 1 day old block,
BatchProofContainer* batchProofContainer = BatchProofContainer::get_instance();
batchProofContainer->fCollectProofs = ((GetSystemTimeInSeconds() - pindexNewTip->GetBlockTime()) > 86400) && GetBoolArg("-batching", true);
batchProofContainer->verify();
if (!batchProofContainer->verify()) {
return AbortNode(state,
"Spark batch verification failed",
_("Spark batch verification failed. Please restart with -reindex -batching=0 to identify the invalid Spark spend."));
}
Comment on lines +3814 to +3818

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Move the failure check before post-connect side effects escape.

Line 3814 verifies the batch only after ActivateBestChainStep() returns; by then ConnectBlock() has already issued pblocktree index writes, and ConnectTip()/this scope have already updated tip-dependent wallet/listener state. If verify() fails here, the node aborts but those side effects can already reflect the invalid Spark batch. Move the checked batch boundary before those durable/index and notification side effects, or defer those side effects until the batch result is known.

Based on PR objectives, pending Spark batches must be verified before validation progress is allowed to advance durably.

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


// When we reach this point, we switched to a new tip (stored in pindexNewTip).

Expand Down
2 changes: 2 additions & 0 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ bool IsInitialBlockDownload();
bool GetTransaction(const uint256 &hash, CTransactionRef &tx, const Consensus::Params& params, uint256 &hashBlock, bool fAllowSlow = false);
/** Find the best known block, and make it the tip of the block chain */
bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock = std::shared_ptr<const CBlock>());
/** Verify any pending deferred Spark proof batch before persisting validation state. */
bool VerifyPendingSparkBatch(CValidationState& state, const std::string& reason);
CAmount GetBlockSubsidyWithMTPFlag(int nHeight, const Consensus::Params& consensusParams, bool fMTP, bool fShorterBlockDistance);
CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams, int nTime = 1475020800);
CAmount GetMasternodePayment(int nHeight, int nTime, CAmount blockValue);
Expand Down
Loading