Fix/issue 259 globalindex reorg algsoch

[algsoch]

Summary

Fixes Issue #259: Blockchain reorganization bug where global_index becomes inconsistent with chronological ordering after reorgs.

⚠️ DRAFT PR - Implementation complete, tests pending. Seeking feedback on approach before adding test suite.

Problem: Only main_chain flag updates, global_index stays wrong → wrong transaction order.

Solution: Recalculate global_index using window function with explicit locking.

---

Implementation

1. TransactionQuerySet.scala

Added recalculateGlobalIndexFromHeight() method:

def recalculateGlobalIndexFromHeight(height: Int)(implicit lh: LogHandler): Update0 =
  sql"""
       |WITH base_index AS (
       |  SELECT COALESCE(MAX(global_index), -1) AS last_index
       |  FROM node_transactions
       |  WHERE inclusion_height < $height AND main_chain = true
       |),
       |ordered_txs AS (
       |  SELECT 
       |    t.id, t.header_id,
       |    (SELECT last_index FROM base_index) + 
       |      ROW_NUMBER() OVER (
       |        ORDER BY t.inclusion_height ASC, 
       |                 t.timestamp ASC, 
       |                 t.index ASC
       |      ) AS new_global_index
       |  FROM node_transactions t
       |  WHERE t.inclusion_height >= $height AND t.main_chain = true
       |  FOR UPDATE
       |)
       |UPDATE node_transactions t
       |SET global_index = o.new_global_index
       |FROM ordered_txs o
       |WHERE t.id = o.id AND t.header_id = o.header_id
       |""".stripMargin.update

2. TransactionRepo.scala

Added method to trait and implementation:

// Trait
def recalculateGlobalIndexFromHeight(height: Int): D[Unit]

// Implementation
def recalculateGlobalIndexFromHeight(height: Int): D[Unit] =
  QS.recalculateGlobalIndexFromHeight(height).run.void.liftConnectionIO

3. ChainIndexer.scala

Modified updateChainStatus() to trigger recalculation:

private def updateChainStatus(blockId: BlockId, mainChain: Boolean): D[Unit] =
  for {
    _ <- repos.headers.updateChainStatusById(blockId, mainChain)
    _ <- if (settings.indexes.blockStats) 
           repos.blocksInfo.updateChainStatusByHeaderId(blockId, mainChain)
         else unit[D]
    _ <- repos.txs.updateChainStatusByHeaderId(blockId, mainChain)
    _ <- repos.outputs.updateChainStatusByHeaderId(blockId, mainChain)
    _ <- repos.inputs.updateChainStatusByHeaderId(blockId, mainChain)
    _ <- repos.dataInputs.updateChainStatusByHeaderId(blockId, mainChain)
    
    headerOpt <- repos.headers.get(blockId)
    _ <- headerOpt match {
      case Some(header) if mainChain =>
        repos.txs.recalculateGlobalIndexFromHeight(header.height)
      case _ =>
        unit[D]
    }
  } yield ()

---

Why Different from PR #266?

PR #266 provided test infrastructure only.

This implementation:

• Uses window function instead of recursive CTE
• Explicit FOR UPDATE locking
• Simpler and easier to maintain

---

[Copilot]

Pull request overview

This PR addresses Issue #259 by implementing a solution to recalculate the global_index field during blockchain reorganizations. The implementation adds a SQL query that uses window functions to recompute global indexes and integrates this into the chain indexer process.

Key changes:

• Adds recalculateGlobalIndexFromHeight() method using PostgreSQL window functions
• Integrates recalculation into updateChainStatus() to trigger during reorganizations
• Uses FOR UPDATE locking strategy for concurrent safety

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TransactionQuerySet.scala	Adds SQL query using ROW_NUMBER() window function to recalculate global_index from a given height with explicit row locking
ChainIndexer.scala	Modifies updateChainStatus() to fetch header and trigger recalculation when blocks become main chain
TransactionRepo.scala	Adds trait method and implementation wrapper for the new recalculation functionality
PR_DESCRIPTION_ISSUE_259_ALGSOCH.md	Documentation file describing the implementation approach and comparison with PR #266
IMPLEMENTATION_COMPLETE.md	Documentation file providing implementation summary and deployment details
CREATE_PR_NOW.md	Documentation file with PR submission instructions

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Potential type mismatch in the calculation. The ROW_NUMBER() function returns a BIGINT, and last_index is derived from MAX(global_index) which is a BIGINT (since globalIndex is defined as Long in the Transaction model). However, when ROW_NUMBER() is added to last_index, the result should be cast to ensure type consistency. While PostgreSQL handles this automatically in most cases, explicitly casting the result would make the query more robust and self-documenting:

CAST((SELECT last_index FROM base_index) + ROW_NUMBER() OVER (...) AS BIGINT)

This ensures the new_global_index matches the expected type of the global_index column.

Suggested change

	| (SELECT last_index FROM base_index) +
	| ROW_NUMBER() OVER (
	| ORDER BY t.inclusion_height ASC,
	| t.timestamp ASC,
	| t.index ASC
	| ) AS new_global_index
	| CAST(
	| (SELECT last_index FROM base_index) +
	| ROW_NUMBER() OVER (
	| ORDER BY t.inclusion_height ASC,
	| t.timestamp ASC,
	| t.index ASC
	| ) AS BIGINT
	| ) AS new_global_index

[Copilot]

The FOR UPDATE clause is placed incorrectly in this CTE. In PostgreSQL, when using CTEs with UPDATE statements, FOR UPDATE should either be:

1. Removed entirely (the UPDATE statement itself provides locking), or
2. Applied to the SELECT in the UPDATE's FROM clause

As written, this FOR UPDATE locks rows during the CTE evaluation but these locks may not protect the final UPDATE operation correctly. The UPDATE statement's WHERE clause already provides row-level locking. Consider removing the FOR UPDATE clause as it's redundant and may cause performance issues.

Suggested change

	| FOR UPDATE -- Explicit locking for concurrent safety
	|

[Copilot]

Race condition in reorganization handling. Looking at line 205 in commitChainUpdates, the code does: updateChainStatus(id, mainChain = true) >> nonBest.traverse(updateChainStatus(_, mainChain = false)). This means:

1. updateChainStatus is called with mainChain=true for the new best block at height H
2. Lines 243-248 trigger recalculateGlobalIndexFromHeight(H)
3. The recalculation query (line 222-223 in TransactionQuerySet) selects all transactions where inclusion_height >= H AND main_chain = true
4. At this moment, the old competing block's transactions at height H still have main_chain=true (they haven't been updated yet)
5. This causes BOTH the new block's transactions AND the old block's transactions to be included in the recalculation

The recalculation should happen AFTER all chain status updates are complete for a given height, not during the first block's update. Consider moving the recalculation outside of updateChainStatus and calling it once after all blocks at a height have been processed in commitChainUpdates.

Suggested change

	// FIX FOR ISSUE #259: Recalculate globalIndex after chain reorganization
	// When a block's main_chain status changes, we need to ensure globalIndex
	// remains consistent with chronological ordering (height -> timestamp -> tx_index)
	//
	// This only triggers when mainChain = true (block becoming part of main chain)
	// to avoid unnecessary recalculations when blocks are removed from main chain.
	headerOpt <- repos.headers.get(blockId)
	_ <- headerOpt match {
	case Some(header) if mainChain =>
	// Recalculate globalIndex for all transactions from this height onwards
	// This ensures ORDER BY timestamp = ORDER BY globalIndex invariant
	repos.txs.recalculateGlobalIndexFromHeight(header.height)
	case _ =>
	// No recalculation needed if block is being marked as non-main-chain
	// or if header not found (shouldn't happen, but defensive programming)
	unit[D]
	}