Skip to content

Fix multi node test#572

Open
sugh01 wants to merge 4 commits intobsv-blockchain:mainfrom
sugh01:fix-multi-node-test
Open

Fix multi node test#572
sugh01 wants to merge 4 commits intobsv-blockchain:mainfrom
sugh01:fix-multi-node-test

Conversation

@sugh01
Copy link
Copy Markdown
Collaborator

@sugh01 sugh01 commented Mar 10, 2026

No description provided.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 10, 2026

🤖 Claude Code Review

Status: Complete


Critical Issues Found:

  1. daemon/daemon_stores.go:34 - Using url.URL as map key will cause equality issues. URLs contain pointers/slices internally, so identical URL strings may not match as map keys, leading to cache misses and multiple store instances.

  2. test/sequentialtest/double_spend/10_fork_pruning_double_spend_test.go:140 - Logic error: Comment says "Delete txB" but code deletes txA. This changes the test behavior - appears txB should be deleted since it is on the losing chain.

Design Concerns:

  1. daemon/test_daemon.go:258 - Creating temporary settings.Settings to extract P2P port is fragile. The override function may expect a fully initialized object or have side effects.

  2. daemon/test_daemon.go:168 - Commented-out code for unique test contexts may impact test isolation. Without unique contexts, concurrent TestDaemons could share cached listeners causing port conflicts.

Other Changes:

  • Settings configuration refactored to use KAFKA_SUFFIX pattern (cleaner)
  • Blockchain store now supports multiple URLs via caching (once map key issue is fixed)
  • New fork pruning tests added for postgres/aerospike
  • Various test helper function names capitalized for export

mainP2PClient p2p.ClientI
mainSubtreeStore blob.Store
mainBlockchainStore blockchainstore.Store
mainBlockchainStoreCache map[url.URL]blockchainstore.Store
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using url.URL as a map key is problematic. URLs are structs with pointers and slices internally, which means two URLs with identical string representations may not be equal as map keys. This could lead to cache misses where the same URL creates multiple store instances.

Consider using the URL string representation as the map key instead:

mainBlockchainStoreCache map[string]blockchainstore.Store
// Then use: blockchainStoreURL.String() as the key

// P2P gRPC
// Just set the port from the settings override function
if opts.EnableP2P {
var tmpSettings settings.Settings
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code creates a temporary settings.Settings struct just to extract the P2P port from the override function. This is fragile because:

  1. The temporary settings will not have the full configuration context
  2. The override function may expect a properly initialized settings object
  3. If the override function has side effects or checks other fields, this could fail silently

Consider either:

  • Calling the override function after all port assignments
  • Adding the P2P port to TestOptions as an explicit field
  • Documenting that SettingsOverrideFunc should set P2P.Port and reading it after the override is applied to the real appSettings


// ========== Simulate pruning by deleting txB from UTXO store ==========
// This simulates the scenario where:
// - txB UTXOs reach DeleteAtHeight and are pruned
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment on line 30 says "Delete txB" but the code deletes txA. This appears to be a critical logic error.

At this point in the test:

  • txA is on the winning chain (4a)
  • txB is on the losing chain (4b)
  • The test comment says "Delete txB from UTXO store"

But the code deletes txA instead. This fundamentally changes what the test validates. Based on the scenario description (simulating pruning before a reorg), it seems txB should be deleted, not txA.

// The counter ensures uniqueness even when tests run in quick succession.
uniqueID := atomic.AddUint64(&testDaemonCounter, 1)
appSettings.Context = fmt.Sprintf("%s-testdaemon-%d", appSettings.Context, uniqueID)
// uniqueID := atomic.AddUint64(&testDaemonCounter, 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These commented-out lines that modify appSettings.Context appear to be important for test isolation. The comment explains they ensure unique listeners for each TestDaemon instance.

Without this code, multiple TestDaemons running concurrently might share cached listeners from util.GetListener, potentially causing port conflicts or cross-test interference.

Was this intentionally removed? If so, how is test isolation now ensured?

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