Skip to content

Download missing subtree files before block revalidation#454

Open
freemans13 wants to merge 2 commits intobsv-blockchain:mainfrom
freemans13:stu/revalidate-block-download-missing-subtrees
Open

Download missing subtree files before block revalidation#454
freemans13 wants to merge 2 commits intobsv-blockchain:mainfrom
freemans13:stu/revalidate-block-download-missing-subtrees

Conversation

@freemans13
Copy link
Collaborator

@freemans13 freemans13 commented Jan 29, 2026

Problem

When revalidating old invalid blocks via the RevalidateBlock RPC endpoint, the pruner may have already removed the subtree files (FileTypeSubtreeToCheck and FileTypeSubtreeData). This causes revalidation to fail because the validation logic cannot access the required data.

Solution

Call fetchSubtreeDataForBlock() at the start of RevalidateBlock() to download any missing subtree files before proceeding with validation. This method:

  • Checks if each subtree file exists locally
  • Downloads missing files from the original peer's DataHubURL
  • Falls back to alternative peers via GetPeersAtMaxHeight() if the primary peer fails

The fetch is only attempted when p2pClient is available, since peer communication is required to download the files.

Changes

  • services/blockvalidation/Server.go: Added subtree fetch logic in RevalidateBlock() before calling ValidateBlockWithOptions()

Testing

  • Build verified: go build ./services/blockvalidation/...
  • Manual testing: Create an invalid block, let pruner remove subtrees, call RevalidateBlock via gRPC

🤖 Generated with Claude Code

@freemans13 freemans13 self-assigned this Jan 29, 2026
@freemans13 freemans13 changed the title Revalidate block - call fetchSubtreeDataForBlock() Download missing subtree files before block revalidation Jan 29, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

🤖 Claude Code Review

Status: Complete

Current Review:

The PR adds subtree data fetching before block revalidation, which correctly addresses the pruning issue. However, there is one existing concern:

  • Empty baseURL handling (inline comment): When GetPeer() fails at Server.go:1168, an empty baseURL is passed to fetchSubtreeDataForBlock(), causing invalid URLs like /subtree/hash. While fallback logic exists, this creates unnecessary HTTP failures and warning logs. Consider early validation or explicit error handling.

The core logic is sound and the PR description accurately explains the problem and solution.

// This handles the case where pruner has removed subtree data for old invalid blocks.
if u.p2pClient != nil {
var baseURL string
if peer, err := u.p2pClient.GetPeer(ctx, blockHeaderMeta.PeerID); err == nil && peer != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The error from GetPeer is silently ignored. If GetPeer fails, baseURL will be empty, causing fetchSubtreeDataForBlock to construct an invalid URL. While there is fallback logic, this causes unnecessary HTTP failures and warning logs. Consider logging the GetPeer error or handling it explicitly, similar to catchup_get_block_headers.go:438

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant