fix(nitronode): trim blockhash, set it to null in DB

[nksazonov]

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Improved handling of legacy block hash records to properly represent missing data and enhance blockchain reorganization detection accuracy.

[coderabbitai]

📝 Walkthrough

Walkthrough

A Postgres migration makes contract_events.block_hash nullable and converts legacy space-padded empty CHAR(66) values to NULL. Two Go getter functions (GetLatestContractEventBlockHashAndNumber, GetPreviousDistinctBlockHash) now apply strings.TrimSpace to the returned hash. A regression test verifies that padded-empty hashes trim to "" while real hashes are preserved.

Changes

Legacy empty block_hash normalization

Layer / File(s)	Summary
Postgres migration: make block_hash nullable nitronode/config/migrations/postgres/20260618000000_make_block_hash_nullable.sql	Up migration drops the NOT NULL constraint and empty-string default from contract_events.block_hash, then converts rows where TRIM(block_hash) = '' to NULL. Down migration reverses by converting NULL back to '' and restoring the constraint.
Go getter trimming and regression tests nitronode/store/database/contract_event.go, nitronode/store/database/contract_event_test.go	GetLatestContractEventBlockHashAndNumber and GetPreviousDistinctBlockHash now wrap the returned ev.BlockHash in strings.TrimSpace. New test TestGetContractEventBlockHash_TrimsPaddedEmpty inserts space-padded legacy hashes and asserts both getter paths return "" while a real 0x… hash is unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A hash full of spaces? That's really not fair,
🐰 I'll trim all the blanks floating there in the air.
The migration says NULL when the value's all white,
And TrimSpace in Go sets the empty check right.
No phantom reorg shall pass through my gate!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and concisely describes the main change: trimming blockhash values and setting them to null in the database, which matches the core modifications across the migration, Go code, and tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/false-reorg-identif

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

nitronode/store/database/contract_event.go (1)

20-20: ⚠️ Potential issue | 🟠 Major

Handle nullable block_hash at the model boundary.

The migration allows NULL values in block_hash (line 9: DROP NOT NULL, line 10: UPDATE...SET block_hash = NULL), but ContractEvent.BlockHash remains a non-nullable string type. While GORM converts NULL to empty string during scan, this creates an implicit and unclear contract: getters at lines 89 and 107 both call strings.TrimSpace(ev.BlockHash) without explicit null handling, making it ambiguous whether empty string represents actual empty data or a NULL from the database.

To clarify intent and prevent future bugs, change BlockHash to sql.NullString and update both getters to check Valid before accessing the string value.

Proposed fix (nullable model field + explicit normalization)

+import "database/sql"

 type ContractEvent struct {
@@
-	BlockHash       string    `gorm:"column:block_hash"`
+	BlockHash       sql.NullString `gorm:"column:block_hash"`
@@
 }

 func (s *DBStore) StoreContractEvent(ev core.BlockchainEvent) error {
 	contractEvent := &ContractEvent{
@@
-		BlockHash:       ev.BlockHash,
+		BlockHash:       sql.NullString{String: ev.BlockHash, Valid: true},
@@
 	}
@@
 }

 func (s *DBStore) GetLatestContractEventBlockHashAndNumber(contractAddress string, blockchainID uint64) (uint64, string, error) {
@@
-	return ev.BlockNumber, strings.TrimSpace(ev.BlockHash), nil
+	hash := ""
+	if ev.BlockHash.Valid {
+		hash = strings.TrimSpace(ev.BlockHash.String)
+	}
+	return ev.BlockNumber, hash, nil
 }
@@
 func (s *DBStore) GetPreviousDistinctBlockHash(contractAddress string, blockchainID uint64, belowBlockNumber uint64) (uint64, string, error) {
@@
-	return ev.BlockNumber, strings.TrimSpace(ev.BlockHash), nil
+	hash := ""
+	if ev.BlockHash.Valid {
+		hash = strings.TrimSpace(ev.BlockHash.String)
+	}
+	return ev.BlockNumber, hash, nil
 }

Also applies to lines 32–40 (StoreContractEvent), 78–90 (GetLatestContractEventBlockHashAndNumber), and 95–108 (GetPreviousDistinctBlockHash).

🤖 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 `@nitronode/store/database/contract_event.go` at line 20, The BlockHash field
in the ContractEvent struct is defined as a non-nullable string type, but the
database migration allows NULL values, creating an implicit and unclear
contract. Change the BlockHash field from string to sql.NullString to explicitly
represent nullable data. Then update all code that accesses BlockHash (including
the getters around lines 89 and 107, the StoreContractEvent struct around lines
32-40, and the GetLatestContractEventBlockHashAndNumber and
GetPreviousDistinctBlockHash functions) to check the Valid field of the
sql.NullString before accessing the string value instead of calling
strings.TrimSpace without null handling.

🧹 Nitpick comments (1)

nitronode/store/database/contract_event_test.go (1)

93-135: ⚡ Quick win

Add an explicit NULL-hash regression case.

Current coverage validates padded legacy values, but migration writes real NULLs. Add a subtest inserting block_hash = NULL (raw SQL) and assert both getters return "" without error.

🤖 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 `@nitronode/store/database/contract_event_test.go` around lines 93 - 135, Add a
new subtest to the TestGetContractEventBlockHash_TrimsPaddedEmpty function that
validates NULL block_hash values from migrations. Insert a contract event with
block_hash set to NULL using raw SQL, then call both
GetLatestContractEventBlockHashAndNumber and GetPreviousDistinctBlockHash
methods and assert that both return an empty string without error. This ensures
the code handles actual NULL values from database migrations, not just the
padded legacy values already covered by the existing subtests.

🤖 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.

Outside diff comments:
In `@nitronode/store/database/contract_event.go`:
- Line 20: The BlockHash field in the ContractEvent struct is defined as a
non-nullable string type, but the database migration allows NULL values,
creating an implicit and unclear contract. Change the BlockHash field from
string to sql.NullString to explicitly represent nullable data. Then update all
code that accesses BlockHash (including the getters around lines 89 and 107, the
StoreContractEvent struct around lines 32-40, and the
GetLatestContractEventBlockHashAndNumber and GetPreviousDistinctBlockHash
functions) to check the Valid field of the sql.NullString before accessing the
string value instead of calling strings.TrimSpace without null handling.

---

Nitpick comments:
In `@nitronode/store/database/contract_event_test.go`:
- Around line 93-135: Add a new subtest to the
TestGetContractEventBlockHash_TrimsPaddedEmpty function that validates NULL
block_hash values from migrations. Insert a contract event with block_hash set
to NULL using raw SQL, then call both GetLatestContractEventBlockHashAndNumber
and GetPreviousDistinctBlockHash methods and assert that both return an empty
string without error. This ensures the code handles actual NULL values from
database migrations, not just the padded legacy values already covered by the
existing subtests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 88107b0c-df5c-4dd9-a750-51b858987a2a

📥 Commits

Reviewing files that changed from the base of the PR and between cc21377 and 8674e8b.

📒 Files selected for processing (3)

• nitronode/config/migrations/postgres/20260618000000_make_block_hash_nullable.sql
• nitronode/store/database/contract_event.go
• nitronode/store/database/contract_event_test.go