[A13xB0]

Problem

nodeHashes is the set behind byNode's de-dup: for every node, the transmissions already listed under it. It was keyed by each transmission's 16-character hash string. On a week of a busy network that's about 650k entries and 21.5 MB, four times the size of byNode itself.

Change

• The sets are keyed by transmission ID. That's the same membership, since a store holds one transmission per hash.
• Eviction deletes by ID.
• The health endpoint's region filter matches regional observations by transmission ID.
• newNodeSets and nodeHasTx wrap the map for callers and tests.

Measured

	before	after
node sets' live heap (copy of a busy week)	21.5 MB	13.2 MB
fixture, 500k (node, packet) pairs	25.8 MB	18.0 MB

Tests

• TestNodeSetRetainedHeap: red commit first.
• TestAddToByNodeDedup.
• The existing eviction, routes and resolved-index tests updated to the ID-keyed sets.
• go test ./cmd/server passes.

No config or UI changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KaR4xu8gVykQBkH8yG8m6H

[efiten]

One test before merge; the change itself is right.

What I checked

• Keying.
  • Hash and ID map one to one: transmissions has id INTEGER PRIMARY KEY AUTOINCREMENT and hash TEXT NOT NULL UNIQUE, and AUTOINCREMENT never reuses an ID.
  • Every write (addToByNode) and both eviction loops switched keys together, so no set mixes hash and ID keys.
  • Every addToByNode caller passes a StoreTx built from a DB row, so tx.ID is set. A duplicate object for a hash that is already loaded carries the same ID.
• Unchanged: locking stays the same, and eviction still drops empty sets.
• Heap, same fixture as yours (500k pairs, 5 runs each): 26.12-26.15 MB on base, 18.30-18.34 MB on head. This matches your 25.8 to 18.0 MB. Your TestNodeSetRetainedHeap is red on base (26.1 MB) and green on head.
• Throwaway benchmark (500k adds plus 500k repeat adds, -benchmem -count=6): B/op 63.83 MB to 47.70 MB, allocation count unchanged, time within noise.
• Suite and interaction with other PRs:
  • go vet is clean.
  • go test ./cmd/server passes on the head merged into current master.
  • -race on the ByNode/Evict/BulkHealth/Chunk tests passes.
  • None of the perf(store) PRs merged since your base touches nodeHashes.

Requested change

1. The rewritten region branch of GetBulkHealth (store.go:9806-9826) has no assertion. The two tests that reach it check nothing about the result:

   • coverage_test.go:2232 does _ = results;
   • coverage_test.go:2662 checks only the HTTP status.

   If regionalTxIDs were keyed by the wrong ID (an observation ID instead of the transmission ID, say), the suite would still pass. Please add a test that asserts which pubkeys come back for a region whose observer saw some nodes' transmissions and not others'.

Notes, not blocking

1. nodeHashes (store.go:200) and the eviction locals hashes now hold transmission IDs. The comment on the field is right, the names are not. Renaming to nodeTxIDs would fix that, or leave it to avoid churn.
2. nodeHasTx (store.go:1922-1926) is only used by tests.
3. The comments at store.go:1917-1919 and in the header of node_set_memory_test.go carry measured numbers ("~650k entries", "about 30% less", "21.5MB"). Those will go stale; the PR description is a better home for them.

[A13xB0]

Review feedback addressed (commits 77fee316, 41f6912b).

1. 77fee316: TestGetBulkHealthRegionKeepsNodesHeardThere asserts exactly which pubkeys GetBulkHealth returns per region, on a fixture of its own: two observers (EDI, GLA) and three repeaters, one heard only in EDI, one only in GLA, and one that adverts in GLA and also relays a packet heard in EDI. EDI must give {a, c} and GLA {b, c}. Observation IDs (1-4) never equal transmission IDs (10-40), so a filter keyed by the wrong ID fails: switching regionalTxIDs[o.TransmissionID] to o.ID makes both regions come back empty, and the test fails on that.
2. Notes:
   • Note 3, done (41f6912b): the measured sizes are out of the comments in store.go and the header of node_set_memory_test.go; they stay in the description.
   • Note 1, left as is: I kept the nodeHashes name to avoid churn across the files that touch it; the field comment says what it holds now.
   • Note 2, left as is: nodeHasTx is test-only on purpose, so the tests don't depend on how the sets are keyed.

go vet ./... is clean, go test ./... passes in cmd/server, and -race -count=2 passes on the ByNode, Evict, BulkHealth, Chunk and NodeSet tests. The branch still merges cleanly with current master.

🤖 Generated with Claude Code

[A13xB0]

Bump

[efiten]

Thanks, the region test is what I asked for. Approving.

What I checked at 41f6912b

• TestGetBulkHealthRegionKeepsNodesHeardThere fails when regionalTxIDs[o.TransmissionID] (store.go:9813) becomes o.ID: both regions come back empty. It also fails with the region filter bypassed (both regions return a, b and c) and with relay indexing dropped in indexResolvedPathHops (EDI loses c), so the relay path is covered too.
• Every non-test use of the sets keys by transmission ID: addToByNode, both eviction paths, and the GetBulkHealth region filter. All four addToByNode callers pass a *StoreTx.
• go vet is clean, go test -count=1 ./... in cmd/server passes, and -race on the ByNode, Evict, BulkHealth and NodeSet tests passes. The branch merges cleanly with current master, and the merged tree passes go test too.
• Keeping the nodeHashes name and the test-only nodeHasTx is fine with me, for the reasons you gave.

Small points, not blocking

1. eviction_test.go:313 prints tx.Hash in its failure message, but the set holds transmission IDs now; tx.ID with %d would match.
2. The description's Tests section does not list the new region test yet.

I did not re-run the heap benchmark: the changes since fcc74512 are a test and comments.