Spark: Speed up reindex proof verification#1865
Closed
navidR wants to merge 8 commits into
Closed
Conversation
Keep non-wallet VerifyDB paths from honoring fVerifySparkSpendProof=false for Spark spend proofs, while preserving wallet DB load behavior by marking that call as wallet validation. Extend the Spark tampered-proof regression test to cover both paths.
Do not mark non-stateful CheckBlock results as fully cached for blocks containing Spark spends. This keeps default proof verification and VerifyDB behavior intact while preventing soft-passed Spark proof-input cases from poisoning block.fChecked.\n\nAdd Spark regressions for VerifyDB cache bypass, wallet-load cheap validation with proof skipping, and BatchProofContainer collection around tampered spends.
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughThis PR overhaults Spark spend proof verification to support deferred and batched checking. It hardens low-level crypto (MultiExponent error propagation, Grootle batch-verify accumulation), adds a ChangesSpark Spend Proof Deferred/Batched Verification
Sequence Diagram(s)sequenceDiagram
participant AcceptBlock
participant CheckBlock
participant CheckTransaction
participant CheckSparkSpendTransaction
participant BatchProofContainer
participant ConnectBlock
rect rgba(70, 130, 180, 0.5)
Note over AcceptBlock,CheckBlock: Block acceptance path
AcceptBlock->>AcceptBlock: fDeferSparkSpendProofVerification = (dbp != NULL)
AcceptBlock->>CheckBlock: CheckBlock(..., fDeferSparkSpendProofVerification)
CheckBlock->>CheckTransaction: fVerifySparkSpendProof, &sparkSpendProofResult
CheckTransaction->>CheckSparkSpendTransaction: fVerifySparkSpendProof, sparkSpendProofResult
CheckSparkSpendTransaction-->>CheckTransaction: Verified | Deferred | Batched
CheckBlock->>CheckBlock: gate fChecked on fSparkSpendProofsComplete
end
rect rgba(60, 179, 113, 0.5)
Note over ConnectBlock,BatchProofContainer: Block connection path
ConnectBlock->>CheckBlock: fDeferSparkSpendProofVerification=true
ConnectBlock->>BatchProofContainer: finalize()
ConnectBlock->>BatchProofContainer: verify() [before persistent state updates]
BatchProofContainer-->>ConnectBlock: throws std::invalid_argument on failure
ConnectBlock->>ConnectBlock: reject block on exception
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
coderabbitai
Bot
added
Spark
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/batchproof_container.cpp (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear batch state when verification throws.
batch_spark()only clearssparkTransactionson success. Any throw before Line 134 leaves the singleton holding the failed batch, andinit()only clearstempSparkTransactions, so the next batch can be poisoned unless every caller remembers to callclear().Proposed exception-safe cleanup
void BatchProofContainer::verify() { - if (!fCollectProofs) { - batch_spark(); - } - fCollectProofs = false; + try { + if (!fCollectProofs) { + batch_spark(); + } + fCollectProofs = false; + } catch (...) { + clear(); + throw; + } }Also applies to: 127-134
🤖 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 44 - 49, The verify() method in BatchProofContainer does not clean up state when batch_spark() throws an exception, leaving the singleton with a failed batch. Wrap the batch_spark() call in a try-catch block (or use RAII with a cleanup object) to ensure that sparkTransactions is properly cleared or reset even if batch_spark() throws before completing, preventing the next batch from being poisoned. The cleanup should occur in the catch handler before re-throwing the exception, ensuring exception safety while maintaining the fCollectProofs flag reset.
🧹 Nitpick comments (2)
src/validation.cpp (1)
4161-4161: ⚡ Quick winMove the
CheckBlockopening brace to the next line.This changed function definition doesn’t follow the repository’s Linux brace style for function definitions.
♻️ Proposed style fix
-bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, int nHeight, bool isVerifyDB, bool fDeferSparkSpendProofVerification) { +bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, int nHeight, bool isVerifyDB, bool fDeferSparkSpendProofVerification) +{As per coding guidelines, “Linux brace style: new line for namespaces, classes, and function definitions.”
🤖 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` at line 4161, The function definition for CheckBlock in src/validation.cpp (line 4161) has its opening brace on the same line as the function signature, which violates the repository's Linux brace style guidelines. Move the opening brace to a new line following the function signature so that CheckBlock follows the required style where function definitions have their opening brace on a separate line, consistent with repository conventions.Source: Coding guidelines
src/test/spark_tests.cpp (1)
643-711: ⚡ Quick winUse scoped cleanup for
BatchProofContainerglobal state in this test.This test toggles singleton state (
fCollectProofs,init(),clear()) across many branches. A scoped guard would prevent state leakage if an unexpected exception/abort occurs mid-test.Suggested pattern
+ BatchProofContainer* batchProofContainer = BatchProofContainer::get_instance(); + const bool originalCollectProofs = batchProofContainer->fCollectProofs; + struct BatchProofContainerStateGuard { + BatchProofContainer* c; + bool original; + ~BatchProofContainerStateGuard() { + c->clear(); + c->fCollectProofs = original; + c->init(); + } + } batchGuard{batchProofContainer, originalCollectProofs}; - - BatchProofContainer* batchProofContainer = BatchProofContainer::get_instance();🤖 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/test/spark_tests.cpp` around lines 643 - 711, The test manually toggles `batchProofContainer` global state (setting `fCollectProofs`, calling `init()` and `clear()`) across multiple test branches, which risks state leakage if an exception or assertion failure occurs between initialization and cleanup. Implement a scoped guard class (RAII pattern) that wraps `batchProofContainer` state management—ensure the guard's constructor handles initialization (setting `fCollectProofs` and calling `init()`) and the destructor automatically calls `clear()` regardless of whether the scope exits normally or via exception. Replace the manual state toggles throughout the test with instantiations of this scoped guard to ensure cleanup always happens.
🤖 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 80-83: Add range validation for uint64_t cover-set IDs before
narrowing to int. At line 80 where cover_set_id is used in the
GetCoinGroupInfo() call and at line 108 where it is used in the
GetCoinSetForSpend() call, validate that the uint64_t value falls within the
valid range for int (typically 0 to INT_MAX) before performing the conversion.
If the value is out of range, throw an appropriate exception to prevent silent
narrowing that could cause incorrect cover set lookups.
In `@src/spark/state.cpp`:
- Around line 845-879: The exception handling in both locations is too broad and
conflates internal/runtime errors with actual proof verification failures. In
src/spark/state.cpp lines 845-879, the catch block for std::exception is
converting all exceptions to passVerify=false, which masks internal errors.
Replace the bare catch block to let internal exceptions propagate (either via
return state.Error(...) or by not catching them) while only the verifier result
from spark::SpendTransaction::verify should determine passVerify. Similarly, in
src/batchproof_container.cpp lines 121-130, do not catch and suppress exceptions
from the batch verification logic; instead rethrow internal/runtime failures and
reserve the invalid_argument exception for actual proof verification failures
only.
- Line 761: The batching path sets fNeedFullCoverSets based on !useBatching
(line 761), which causes cover_sets to contain empty vectors when batching is
enabled. However, proofInputHash still includes these empty cover_sets in its
computation, creating a different hash from prior non-batched verification of
the same spend. This causes the cache invalidation logic in the range 841-854 to
incorrectly erase valid cache entries. Fix this by either computing the
proofInputHash using only stable proof inputs that remain consistent between
batched and non-batched paths (excluding cover_sets when they are not
materialized), or by adding a condition in the cache invalidation section to
skip erasing entries when full cover sets were not available (when useBatching
is true).
In `@src/validation.cpp`:
- Around line 2815-2819: The Spark batch is being verified twice: once in the
new pre-state-update verification block at lines 2815-2819 where you call
batchProofContainer->verify() inside the try block, and again later in
ActivateBestChain at line 3817. Remove the later verification call at line 3817
in the ActivateBestChain function, or if that call is in a location that affects
other logic, conditionally skip it after the earlier verification has already
succeeded to prevent duplicate verification and avoid verifying after the block
has been connected.
---
Outside diff comments:
In `@src/batchproof_container.cpp`:
- Around line 44-49: The verify() method in BatchProofContainer does not clean
up state when batch_spark() throws an exception, leaving the singleton with a
failed batch. Wrap the batch_spark() call in a try-catch block (or use RAII with
a cleanup object) to ensure that sparkTransactions is properly cleared or reset
even if batch_spark() throws before completing, preventing the next batch from
being poisoned. The cleanup should occur in the catch handler before re-throwing
the exception, ensuring exception safety while maintaining the fCollectProofs
flag reset.
---
Nitpick comments:
In `@src/test/spark_tests.cpp`:
- Around line 643-711: The test manually toggles `batchProofContainer` global
state (setting `fCollectProofs`, calling `init()` and `clear()`) across multiple
test branches, which risks state leakage if an exception or assertion failure
occurs between initialization and cleanup. Implement a scoped guard class (RAII
pattern) that wraps `batchProofContainer` state management—ensure the guard's
constructor handles initialization (setting `fCollectProofs` and calling
`init()`) and the destructor automatically calls `clear()` regardless of whether
the scope exits normally or via exception. Replace the manual state toggles
throughout the test with instantiations of this scoped guard to ensure cleanup
always happens.
In `@src/validation.cpp`:
- Line 4161: The function definition for CheckBlock in src/validation.cpp (line
4161) has its opening brace on the same line as the function signature, which
violates the repository's Linux brace style guidelines. Move the opening brace
to a new line following the function signature so that CheckBlock follows the
required style where function definitions have their opening brace on a separate
line, consistent with repository conventions.
🪄 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: 069270b1-db23-4b85-ab9f-8e40e2137f99
📒 Files selected for processing (12)
src/batchproof_container.cppsrc/batchproof_container.hsrc/libspark/grootle.cppsrc/libspark/spend_transaction.cppsrc/libspark/test/grootle_test.cppsrc/secp256k1/src/cpp/MultiExponent.cppsrc/spark/state.cppsrc/spark/state.hsrc/test/spark_tests.cppsrc/validation.cppsrc/validation.hsrc/wallet/walletdb.cpp
Comment on lines
80
to
+83
| int cover_set_id = idAndHash.first; | ||
| if (!cover_sets.count(cover_set_id)) { | ||
| std::vector<spark::Coin> cover_set; | ||
| sparkState->GetCoinSet(cover_set_id, cover_set); | ||
| cover_sets[cover_set_id] = cover_set; | ||
| } | ||
| spark::CSparkState::SparkCoinGroupInfo coinGroup; | ||
| if (!sparkState->GetCoinGroupInfo(cover_set_id, coinGroup)) | ||
| throw std::invalid_argument("Spark batch verification missing cover set"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Spark cover-set ID types and state API parameter types.
# Expectation: uint64_t spend cover-set IDs are either range-checked before int APIs,
# or the CSparkState APIs are widened consistently.
rg -nP -C3 '\bgetBlockHashes\s*\(|\bGetCoinGroupInfo\s*\(|\bGetCoinSetForSpend\s*\('
rg -nP -C3 'std::map\s*<\s*uint64_t\s*,\s*uint256\s*>|unordered_map\s*<\s*uint64_t'Repository: firoorg/firo
Length of output: 27235
🏁 Script executed:
# Read the batchproof_container.cpp file to see the actual code and context
cat -n src/batchproof_container.cpp | sed -n '70,115p'Repository: firoorg/firo
Length of output: 2551
🏁 Script executed:
# Search for any range validation or checks on cover_set_id or ID values
rg -n 'cover_set_id|std::numeric_limits|INT_MAX|int::max' src/batchproof_container.cppRepository: firoorg/firo
Length of output: 182
🏁 Script executed:
# Check the types of idAndBlockHashes to confirm uint64_t
rg -n -B5 'getBlockHashes' src/batchproof_container.cpp | head -20Repository: firoorg/firo
Length of output: 396
🏁 Script executed:
# Check if there's any documentation or comments about the ID range
rg -n -A3 'getBlockHashes|setBlockHashes' src/libspark/spend_transaction.hRepository: firoorg/firo
Length of output: 318
🏁 Script executed:
# Check for any other places where uint64_t cover-set IDs are converted to int
rg -n 'GetCoinGroupInfo|GetCoinSetForSpend' src/batchproof_container.cppRepository: firoorg/firo
Length of output: 194
🏁 Script executed:
# Look for how GetLatestCoinID is used and what range of values it returns
rg -n 'GetLatestCoinID' src/spark/state.cpp | head -10Repository: firoorg/firo
Length of output: 304
🏁 Script executed:
# Check the GetCoinGroupInfo and GetCoinSetForSpend function signatures
rg -n -A2 'bool GetCoinGroupInfo|int GetCoinSetForSpend' src/spark/state.hRepository: firoorg/firo
Length of output: 268
🏁 Script executed:
# Search for any documentation or limits on coin group IDs
rg -n 'group_id|coinGroupID|cover.*set.*id' src/spark/state.h | head -20Repository: firoorg/firo
Length of output: 261
🏁 Script executed:
# Check the GetLatestCoinID implementation to see what values it can produce
sed -n '1209,1215p' src/spark/state.cppRepository: firoorg/firo
Length of output: 225
🏁 Script executed:
# Look at how the coin group IDs are managed - are they always expected to fit in int?
rg -n 'coinGroups\[' src/spark/state.cpp | head -5Repository: firoorg/firo
Length of output: 337
🏁 Script executed:
# Check the serialization of spend transaction to see if there's control over ID ranges
rg -n -B3 -A3 'set_id_blockHash\s*=' src/libspark/spend_transaction.cpp | head -20Repository: firoorg/firo
Length of output: 286
🏁 Script executed:
# Check what values latestCoinId can reach - is it bounded?
rg -n 'latestCoinId' src/spark/state.h | head -10Repository: firoorg/firo
Length of output: 82
🏁 Script executed:
# Check if there are any comments or documentation about ID ranges in state.h
rg -n -B2 -A2 'latestCoinId|coinGroups' src/spark/state.h | head -30Repository: firoorg/firo
Length of output: 317
🏁 Script executed:
# Verify the type of latestCoinId member variable
rg -n 'int.*latestCoinId' src/spark/state.hRepository: firoorg/firo
Length of output: 82
🏁 Script executed:
# Check if there's any place where latestCoinId is initialized or set to specific values
rg -n 'latestCoinId\s*=' src/spark/state.cpp | head -15Repository: firoorg/firo
Length of output: 241
🏁 Script executed:
# Check if there's validation on the coin group IDs anywhere during transaction processing
rg -n -B3 -A3 'setBlockHashes' src/spark/state.cppRepository: firoorg/firo
Length of output: 38
🏁 Script executed:
# Check how selection.first is used and where it comes from
sed -n '98,115p' src/batchproof_container.cppRepository: firoorg/firo
Length of output: 811
🏁 Script executed:
# Verify the data flow - where do sparkTransactions come from?
rg -n 'sparkTransactions' src/batchproof_container.cppRepository: firoorg/firo
Length of output: 666
🏁 Script executed:
# Check if there's any validation in the constructor or entry point
rg -n -B5 'BatchProofContainer::verify' src/batchproof_container.cpp | head -20Repository: firoorg/firo
Length of output: 187
🏁 Script executed:
# Check if there's any other location where these uint64_t IDs might be used without validation
rg -n 'getBlockHashes()' src/ -A5 | grep -E 'getBlockHashes|static_cast.*int|int.*=' | head -20Repository: firoorg/firo
Length of output: 646
🏁 Script executed:
# Verify that there's no existing validation in sparkState methods that would catch out-of-range IDs
sed -n '1199,1210p' src/spark/state.cppRepository: firoorg/firo
Length of output: 339
Add range validation before narrowing uint64_t cover-set IDs to int.
The getBlockHashes() API returns std::map<uint64_t, uint256>, but both GetCoinGroupInfo() and GetCoinSetForSpend() expect int parameters. Without validation, an out-of-range serialized ID can be silently narrowed to an incorrect group ID, causing batch verification to process coins against the wrong cover set.
Line 80 uses implicit narrowing; line 108 uses explicit casting—neither validates the range. Add a range check before both conversions:
Proposed range guard
+#include <limits>
`#include` <stdexcept>
`#include` <unordered_map> auto& idAndBlockHashes = itr.getBlockHashes();
for (const auto& idAndHash : idAndBlockHashes) {
- int cover_set_id = idAndHash.first;
+ if (idAndHash.first > static_cast<uint64_t>(std::numeric_limits<int>::max()))
+ throw std::invalid_argument("Spark batch verification cover set id out of range");
+
+ int cover_set_id = static_cast<int>(idAndHash.first);
spark::CSparkState::SparkCoinGroupInfo coinGroup;
if (!sparkState->GetCoinGroupInfo(cover_set_id, coinGroup))
throw std::invalid_argument("Spark batch verification missing cover set"); const int nCoins = sparkState->GetCoinSetForSpend(
&chainActive,
selection.second.maxHeight,
- static_cast<int>(selection.first),
+ static_cast<int>(selection.first),
blockHash,
cover_set,
setHash);Also applies to: line 108
🤖 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 80 - 83, Add range validation for
uint64_t cover-set IDs before narrowing to int. At line 80 where cover_set_id is
used in the GetCoinGroupInfo() call and at line 108 where it is used in the
GetCoinSetForSpend() call, validate that the uint64_t value falls within the
valid range for int (typically 0 to INT_MAX) before performing the conversion.
If the value is out of range, throw an appropriate exception to prevent silent
narrowing that could cause incorrect cover set lookups.
| !isCheckWallet && | ||
| sparkTxInfo && | ||
| !sparkTxInfo->fInfoIsComplete; | ||
| const bool fNeedFullCoverSets = !useBatching; |
Contributor
There was a problem hiding this comment.
Avoid invalidating verified proof-cache entries from the batching path.
When useBatching is true, cover_sets intentionally contains empty vectors, but proofInputHash still includes cover_sets. That makes the hash differ from a prior non-batched verification for the same spend, so Line 853 erases a valid cache entry and forces duplicate batch verification. Use a cache key based on stable proof inputs available in both paths, or skip cache invalidation when full cover sets were not materialized.
Based on PR context, this path is meant to eliminate redundant Spark spend proof verification during deferred/batched validation.
Also applies to: 841-854
🤖 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/spark/state.cpp` at line 761, The batching path sets fNeedFullCoverSets
based on !useBatching (line 761), which causes cover_sets to contain empty
vectors when batching is enabled. However, proofInputHash still includes these
empty cover_sets in its computation, creating a different hash from prior
non-batched verification of the same spend. This causes the cache invalidation
logic in the range 841-854 to incorrectly erase valid cache entries. Fix this by
either computing the proofInputHash using only stable proof inputs that remain
consistent between batched and non-batched paths (excluding cover_sets when they
are not materialized), or by adding a condition in the cache invalidation
section to skip erasing entries when full cover sets were not available (when
useBatching is true).
Comment on lines
+845
to
+879
| try { | ||
| { | ||
| LOCK(cs_checkedSparkSpendTransactions); | ||
| auto it = gCheckedSparkSpendTransactions.find(hashTx); | ||
| if (it != gCheckedSparkSpendTransactions.end()) { | ||
| auto& checkState = it->second; | ||
| if (checkState.proofInputHash != proofInputHash) { | ||
| LogPrintf("CheckSparkSpendTransaction: cached proof inputs changed for tx %s\n", hashTx.ToString()); | ||
| gCheckedSparkSpendTransactions.erase(it); | ||
| } else if (checkState.fChecked) { | ||
| if (!checkState.fResult) | ||
| return state.DoS(100, false, REJECT_INVALID, "CheckSparkSpendTransaction: previously checked and failed"); | ||
|
|
||
| LogPrintf("CheckSparkSpendTransaction: already checked tx %s\n", hashTx.ToString()); | ||
| fChecked = true; | ||
| } | ||
| } | ||
| } | ||
| catch (const std::exception &) { | ||
| passVerify = false; | ||
|
|
||
| if (fChecked) { | ||
| passVerify = true; | ||
| setProofResult(SparkSpendProofVerificationResult::Verified); | ||
| } else if (useBatching) { | ||
| passVerify = true; | ||
| batchProofContainer->add(*spend); | ||
| setProofResult(SparkSpendProofVerificationResult::Batched); | ||
| } else { | ||
| passVerify = spark::SpendTransaction::verify(*spend, cover_sets); | ||
| if (passVerify) | ||
| setProofResult(SparkSpendProofVerificationResult::Verified); | ||
| } | ||
| } | ||
| catch (const std::exception &) { | ||
| passVerify = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not convert internal verifier exceptions into invalid-proof results. Both paths catch every std::exception, so runtime failures from the crypto verifier become passVerify=false / “batch verification failed” instead of surfacing as internal validation errors.
src/spark/state.cpp#L845-L879: let internal/runtime exceptions propagate to the outerCheckSparkTransactioncatch or returnstate.Error(...); only a verifierfalseresult should mean “invalid proof.”src/batchproof_container.cpp#L121-L130: do not collapse verifier exceptions intopassed=false; rethrow internal/runtime failures and reserve theinvalid_argument("Spark batch verification failed...")path for actual proof verification failures.
Based on PR context, low-level Spark/Grootle verification errors are now propagated via exceptions, so callers need to preserve that distinction.
📍 Affects 2 files
src/spark/state.cpp#L845-L879(this comment)src/batchproof_container.cpp#L121-L130
🤖 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/spark/state.cpp` around lines 845 - 879, The exception handling in both
locations is too broad and conflates internal/runtime errors with actual proof
verification failures. In src/spark/state.cpp lines 845-879, the catch block for
std::exception is converting all exceptions to passVerify=false, which masks
internal errors. Replace the bare catch block to let internal exceptions
propagate (either via return state.Error(...) or by not catching them) while
only the verifier result from spark::SpendTransaction::verify should determine
passVerify. Similarly, in src/batchproof_container.cpp lines 121-130, do not
catch and suppress exceptions from the batch verification logic; instead rethrow
internal/runtime failures and reserve the invalid_argument exception for actual
proof verification failures only.
Comment on lines
+2815
to
+2819
| if (batchProofContainer->fCollectProofs) { | ||
| try { | ||
| batchProofContainer->finalize(); | ||
| batchProofContainer->verify(); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid verifying the same Spark batch again after ConnectTip commits.
This new pre-state-update verification is still followed by the existing BatchProofContainer::verify() in ActivateBestChain at Line 3817. Remove or make that later call a no-op after this block succeeds; otherwise Spark batches are verified twice, and any later exception happens after the block has already been connected.
🐛 Proposed direction
- // 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();
+ // Spark batch verification is completed inside ConnectBlock before state updates.🤖 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 2815 - 2819, The Spark batch is being
verified twice: once in the new pre-state-update verification block at lines
2815-2819 where you call batchProofContainer->verify() inside the try block, and
again later in ActivateBestChain at line 3817. Remove the later verification
call at line 3817 in the ActivateBestChain function, or if that call is in a
location that affects other logic, conditionally skip it after the earlier
verification has already succeeded to prevent duplicate verification and avoid
verifying after the block has been connected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch speeds up reindex after Spark activation by removing duplicate Spark spend proof verification and making the remaining Grootle proof verification cheaper. During reindex/import, Spark spend proofs no longer need to be fully verified in the early CheckBlock path and then verified again during ConnectBlock; instead, the expensive proof check is deferred to the ConnectBlock path before Spark state is committed, while mempool, wallet, and VerifyDB proof checks remain enforced. The Grootle verifier is also optimized by avoiding a large intermediate commitment list and feeding the equivalent multi-scalar multiplication terms directly into secp256k1. In the 855k-860k Spark-heavy window, reindex time dropped from 1:26:13 to about 13:12, a 6.5x speedup and 84.7% wall-time reduction.