test: add invalidate fork testcases by sugh01 · Pull Request #399 · bsv-blockchain/teranode

sugh01 · 2026-01-15T11:58:41Z

No description provided.

github-actions · 2026-01-15T11:59:38Z

🤖 Claude Code Review

Status: Complete

This PR adds comprehensive reorg test coverage and fixes critical bugs in fork/reorg handling. The implementation quality is solid with proper context handling, buffered channels, and correct transaction tracking logic.

Key Improvements:

Reorg correctness fix (SubtreeProcessor.go:2268-2339): Properly tracks and unmarks losing conflicting transactions during reorgs
Common ancestor algorithm (BlockAssembler.go:1593-1608): Fixed to use hash set lookup instead of nested loops
Test stability (BlockAssembler_test.go): Added context cancellation, buffered channels, and proper goroutine cleanup
Chain integrity CI (nightly_tests.yaml): Comprehensive 3-node testing with error monitoring and artifact collection

Existing Review Comments:
All 8 inline comments from previous review remain valid and provide useful context about defensive programming patterns, edge cases, and the core correctness fix.

No new issues found.

github-actions · 2026-01-21T09:45:12Z

services/blockassembly/BlockAssembler.go

 func (b *BlockAssembler) handleReorg(ctx context.Context, header *model.BlockHeader, height uint32) error {
+	if err := ctx.Err(); err != nil {
+		return errors.NewProcessingError("context", err)
+	}


Context cancellation check added but reorg work continues

The early context cancellation check at line 1425 is good defensive programming. However, if the context is cancelled during reorg processing (e.g., in the middle of fetching block headers or updating subtrees), the cleanup operations may be left incomplete.

Consider: Does the reorg logic handle partial state correctly if cancelled midway? The error handling returns early, but are there any state mutations that should be rolled back?

Why was this added? Should this not be taken care of the for/select where handleReorg is called?

services/blockassembly/BlockAssembler.go

github-actions · 2026-01-21T09:45:34Z

services/blockassembly/BlockAssembler.go

-					return nil, nil, errors.NewServiceError("error getting common ancestor header", err)
-				}
+	for !currentHash.IsEqual(newHash) {
+		if currentHeight == 0 || newHeight == 0 {


Common ancestor algorithm improvement

This is a significant correctness fix. The old block locator approach was inefficient and could produce incorrect results when the locator step sizes caused it to skip over the actual common ancestor.

The new approach correctly walks both chains backwards in lockstep, which guarantees finding the true common ancestor. The performance is O(depth_difference + reorg_depth) which is optimal for this problem.

Minor suggestion: The loop condition for height 0 with a break is a bit defensive. If both chains are valid and share genesis, this should never trigger. Consider logging if this happens as it may indicate a data inconsistency.

github-actions · 2026-01-21T09:45:46Z

services/blockassembly/subtreeprocessor/SubtreeProcessor.go

+		losingTxSet[hash] = true
+		allLosingTxHashes = append(allLosingTxHashes, hash)
+	}
+


Critical fix: proper tracking of losing conflicting transactions

This is the core fix for the reorg double-spend handling bug. The previous code only marked transactions as on-longest-chain if they were not in the old fork, but it failed to identify and unmark transactions that LOST conflicts.

The new logic correctly:

Builds winningTxSet from all transactions in winning chain

Collects losingTxHashesMap from moveForwardBlock (new return value)

Filters out any losing tx that also appears in winning set (same tx mined in both forks)

Marks all truly-losing transactions as NOT on longest chain

This ensures that when tx2 loses to tx3 (both spending same UTXO), tx2 gets properly unmarked even though it was in the old fork.

Question: Should filteredMarkOnLongestChain also filter against transactions already marked? Or is the UTXO store idempotent for repeated mark operations?

Good question from bot. Could be more efficient if we do not do unnecessary calls.

Not needed as far as I can tell. I think the store is idempotent.

github-actions · 2026-01-21T09:45:52Z

services/blockassembly/BlockAssembler_test.go

 		initPrometheusMetrics()

-		ctx := context.Background()
+		ctx, cancel := context.WithCancel(context.Background())


Test properly uses context cancellation

Good practice: The test now creates a cancellable context and properly defers the cancel to avoid goroutine leaks. The done channel ensures the background goroutine completes before the test exits.

This pattern should be followed in all tests that spawn goroutines.

We should be using t.Context() everywhere. Was added in go 1.24

github-actions · 2026-01-21T09:45:59Z

services/blockassembly/BlockAssembler_test.go

+
+		// Build two competing chains from height 1:
+		// Main chain: 1 -> 2A -> 3A
+		// Fork chain: 1 -> 2B -> 3B (invalidated)


Buffered channel prevents blocking

Excellent fix: The channel is now buffered with size 100, which prevents the test from blocking when multiple subtree notifications are sent rapidly. The previous unbuffered channel could cause deadlocks if notifications arrived faster than the test goroutine could process them.

The buffer size of 100 is reasonable for test scenarios where you may have multiple incomplete subtrees being generated.

github-actions · 2026-01-21T09:46:07Z

services/blockassembly/BlockAssembler_test.go

+
+		require.Len(t, moveForwardBlockHeaders, 2)
+		assert.Equal(t, h2a.Hash(), moveForwardBlockHeaders[0].header.Hash())
+		assert.Equal(t, h3a.Hash(), moveForwardBlockHeaders[1].header.Hash())


New test validates invalidated fork handling

This test case covers an important edge case: when BlockAssembler is tracking an invalidated fork tip and needs to reorg back to the main chain. This scenario validates that the common ancestor algorithm works correctly even when one of the chains has been explicitly invalidated.

The test correctly:

Simulates BlockAssembler being on the wrong fork

Uses InvalidateBlock to mark the fork invalid

Verifies getReorgBlockHeaders returns correct move-back and move-forward blocks

This complements the other reorg tests by focusing on the administrative invalidation case rather than natural chain reorganization.

github-actions · 2026-01-21T09:46:18Z

.github/workflows/nightly_tests.yaml

+
+      - name: Start Teranode nodes with 3 block generators (docker compose up)
+        run: docker compose -f compose/docker-compose-3blasters.yml up -d
+


Comprehensive nightly test workflow

The new workflow adds important long-running integration testing with 3 concurrent block generators. Key features:

Error monitoring: Checks container logs for ERROR messages at 5-second intervals with timestamp tracking

Height consensus: Waits for all 3 nodes to reach height 120+ before declaring success

Chain integrity validation: Uses chainintegrity tool to verify log consistency across nodes

Proper cleanup: Always runs docker compose down and uploads artifacts on failure

Minor suggestions:

Consider making REQUIRED_HEIGHT configurable via workflow input for flexibility

The 10-minute timeout (MAX_ATTEMPTS=120 * SLEEP=5s) may be tight for slower runners - consider documenting expected runtime

Line 66: Empty since_param on first check is clever but could use a comment explaining why

sonarqubecloud · 2026-01-21T12:49:35Z

Quality Gate failed

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

See analysis details on SonarQube Cloud

icellan · 2026-01-21T13:26:05Z

services/blockassembly/BlockAssembler.go

 func (b *BlockAssembler) handleReorg(ctx context.Context, header *model.BlockHeader, height uint32) error {
+	if err := ctx.Err(); err != nil {
+		return errors.NewProcessingError("context", err)
+	}


Why was this added? Should this not be taken care of the for/select where handleReorg is called?

icellan · 2026-01-21T14:05:53Z

services/blockassembly/BlockAssembler_test.go

 		initPrometheusMetrics()

-		ctx := context.Background()
+		ctx, cancel := context.WithCancel(context.Background())


We should be using t.Context() everywhere. Was added in go 1.24

icellan · 2026-01-21T14:07:47Z

services/blockassembly/subtreeprocessor/SubtreeProcessor.go

+		losingTxSet[hash] = true
+		allLosingTxHashes = append(allLosingTxHashes, hash)
+	}
+


Good question from bot. Could be more efficient if we do not do unnecessary calls.

icellan · 2026-01-21T14:08:51Z

services/blockassembly/subtreeprocessor/SubtreeProcessor.go

+					// Ensure reorg move-back does not silently skip nodes as "duplicates".
+					// A tx can have an entry in currentTxMap even if it was removed from the subtrees,
+					// and addNode() uses SetIfNotExists which would otherwise ignore it.
+					stp.currentTxMap.Delete(subtreeNodes[i].Hash)


This would bypass the duplication check, the reason the currentTxMap exists. Is this really correct?

Is that the reason currentTxMap exists? It would bypass duplication check but does need to be deleted? Is there another way to do that?

Why would it needed to be deleted @liam ? It should be reset at the beginning of a reorg and therefore checks for duplications

Where is it reset? Just in the reset() function? Is that called at the beginning of a reorg?

This is in moveBackBlock, which is adding transactions from previous blocks back into the subtreeprocessor. It should never add a transaction that is already there. If you delete here, you circumvent the duplication check in addNode services/blockassembly/subtreeprocessor/SubtreeProcessor.go:1503

// SetIfNotExists returns (value, wasSet) where wasSet is true if the key was newly inserted if _, wasSet := stp.currentTxMap.SetIfNotExists(node.Hash, parents); !wasSet { // Key already existed, this is a duplicate stp.logger.Debugf("[addNode] duplicate transaction ignored %s", node.Hash.String()) return nil }

icellan · 2026-01-21T14:09:07Z

services/blockassembly/subtreeprocessor/SubtreeProcessor.go

 			} else {
 				for i, node := range subtreeNodes {
+					// Ensure reorg move-back does not silently skip nodes as "duplicates".
+					stp.currentTxMap.Delete(node.Hash)


This would bypass the duplication check, the reason the currentTxMap exists. Is this really correct?

freemans13 · 2026-01-22T12:43:21Z

services/blockassembly/subtreeprocessor/SubtreeProcessor.go

 					completeSubtrees = append(completeSubtrees, stp.chainedSubtrees...)

 					// incomplete subtrees ?
-					if chainedCount == 0 && stp.currentSubtree.Load().Length() > 1 {


Taking out chainedCount == 0 doesn't feel right - doesn't this mean it always creates a incomplete subtree copy?

I thought we only use an incomplete subtree copy when we have no full subtrees available.

test: add invalidate fork testcases

7e8dc2c

liam approved these changes Jan 15, 2026

View reviewed changes

sugh01 and others added 6 commits January 15, 2026 13:47

longestchaintest: fixes

b266d24

4384: test invalid fork

fe707c5

e2e: temporarily disable a failing test

f8119a7

4348: fix double spend tx on longest chain test

174eba5

4348: fix double spend tx on longest chain testagain

4760e68

Merge branch 'main' into longtests-1

a62a6c1

liam requested a review from freemans13 January 21, 2026 09:44

github-actions bot reviewed Jan 21, 2026

View reviewed changes

services/blockassembly/BlockAssembler.go Show resolved Hide resolved

github-actions bot reviewed Jan 21, 2026

View reviewed changes

liam added 2 commits January 21, 2026 11:18

4348: add locate reorg selection

56dc403

4348: fix mock

859fe6c

icellan requested changes Jan 21, 2026

View reviewed changes

freemans13 reviewed Jan 22, 2026

View reviewed changes


		- name: Start Teranode nodes with 3 block generators (docker compose up)
		run: docker compose -f compose/docker-compose-3blasters.yml up -d

Conversation

sugh01 commented Jan 15, 2026

Uh oh!

github-actions bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 21, 2026

Quality Gate failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Jan 15, 2026 •

edited

Loading