Skip to content

fix: Race Condition in Block Height and Median Time Validation#96

Merged
icellan merged 2 commits into
bsv-blockchain:mainfrom
gokutheengineer:feat/atomic-snapshot
Nov 3, 2025
Merged

fix: Race Condition in Block Height and Median Time Validation#96
icellan merged 2 commits into
bsv-blockchain:mainfrom
gokutheengineer:feat/atomic-snapshot

Conversation

@gokutheengineer

@gokutheengineer gokutheengineer commented Nov 1, 2025

Copy link
Copy Markdown
Collaborator

Add Atomic BlockState Snapshot to Prevent Race Conditions

Summary

This PR introduces an atomic BlockState snapshot mechanism in the UTXO store to prevent race conditions between block height and median block time reads during transaction validation.

Problem

Previously, GetBlockHeight() and GetMedianBlockTime() were called separately during validation. This created a race condition where:

  • Block height could be read
  • Block state updates in between
  • Median block time read reflects a different state
  • This inconsistency could cause validation errors or incorrect finality checks

Solution

Introduced GetBlockState() method that returns an atomic snapshot containing both:

type BlockState struct {
    Height     uint32 // Current block height
    MedianTime uint32 // Median time of recent blocks
}

Changes

  • stores/utxo/Interface.go: Added BlockState struct and GetBlockState() interface method
  • stores/utxo/mock.go: Implemented mock for GetBlockState()
  • stores/utxo/aerospike|sql|nullstore: Implemented GetBlockState() in all store implementations
  • services/validator/Validator.go: Updated validation logic to use atomic GetBlockState() instead of separate calls
  • All test files: Updated mocks to use GetBlockState() instead of deprecated separate calls

Benefits

  • ✅ Eliminates race conditions during concurrent block state updates
  • ✅ Ensures consistency between height and median time during validation
  • ✅ Improves reliability of transaction finality checks (BIP113)
  • ✅ Cleaner API with single atomic operation

Testing

  • Updated all existing tests to use new GetBlockState() mock
  • All validator tests pass with the new atomic snapshot approach

@github-actions

github-actions Bot commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Status: Complete


Critical Issue Found:

The new GetBlockState() implementation does not actually provide atomic snapshot guarantees. In both Aerospike and SQL implementations, the method calls .Load() on two separate atomic variables.

See inline comment at stores/utxo/aerospike/aerospike.go:354 for details.

Between the first Load() and second Load(), another goroutine could call SetBlockHeight() or SetMedianBlockTime(), causing the returned snapshot to contain mismatched values from different states. This defeats the purpose of the PR.

Solution:

To achieve true atomicity, consider one of these approaches:

  1. RWMutex approach - Use a single lock protecting both values
  2. Single atomic.Value - Store the entire BlockState struct in one atomic variable
  3. atomic.Uint64 - Pack both uint32 values into a single uint64 for atomic read/write

The rest of the PR structure is solid - good test coverage, proper interface design, and consistent implementation across all store types.

@sonarqubecloud

sonarqubecloud Bot commented Nov 3, 2025

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
15.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Comment thread stores/utxo/aerospike/aerospike.go
@icellan icellan merged commit 275ce97 into bsv-blockchain:main Nov 3, 2025
8 of 9 checks passed
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.

2 participants