Remove parentMap

[ChrisSchinnerl]

I'm proposing to remove the parentMap cache. I think Luke is right in the linked issue that the panic stems from an inconsistency between the cache and pool state. However, if we invalidate the cache every time we call revalidatePool, which we call every time we call UnconfirmedParents, there is no point in keeping the map anymore. Since it will be invalidated before every time we use it.

Fixes #256

[Copilot]

Pull Request Overview

This PR removes the parentMap cache from the txpool to eliminate inconsistencies between the cache and pool state by computing a fresh parent map every time it is needed. Key changes include:

• Removing the parentMap field from the Manager struct.
• Updating computeParentMap to compute a new map on each call.
• Removing parentMap invalidation assignments in reorgTo, AddPoolTransactions, and AddV2PoolTransactions.

Comments suppressed due to low confidence (4)

chain/manager.go:644

• The computeParentMap function no longer uses a cached value and instead computes a fresh map on each call. Please confirm that all callers are intended to operate with a newly computed map rather than relying on a persistent cache.

func (m *Manager) computeParentMap() map[types.Hash256]int {

chain/manager.go:415

• The removal of the parentMap cache invalidation in reorgTo is consistent with the updated design. Verify that downstream code no longer expects a cached parentMap value.

m.txpool.parentMap = nil

chain/manager.go:1215

• Removing the parentMap invalidation in AddPoolTransactions aligns with the new approach. Ensure that this change doesn’t impact any logic that previously relied on the cached state.

m.txpool.parentMap = nil

chain/manager.go:1295

• The deletion of the parentMap invalidation in AddV2PoolTransactions is consistent with the overall removal of the cache. Confirm that no other parts of the system are affected by this removal.

m.txpool.parentMap = nil

[lukechampine]

tbc, I meant we should invalidate the cache if the pool is changed by revalidatePool. If you're calling UnconfirmedParents many times between txpool changes, the cache is still helpful.

That said, I never actually benchmarked how long it takes to rebuild the parentMap. If the answer is <100ms, then yeah I agree for simplicity we should just remove it.

[lukechampine]

Ok, I benchmarked it: with 1000 txns in the pool, each spending 1 input and creating 1 output, it takes <1 ms. With 100 txns, each spending 1 input and creating 100 outputs, it takes 87ms. So things start to get worryingly slow around 10,000 total outputs across txns in the pool.

However, that's for v1 transactions, where computing an output ID requires hashing the full transaction. When I rerun the benchmark with v2 transactions, I see a ~20x speedup: 4ms for 10k outputs. (And about the same <1ms for 1000 single-output txns, as expected.) Given that most of the network has already transitioned to using only v2 transactions, I think we can safely say that we have enough headroom to ditch the map. The only reason to keep it around would be if someone out there needs to call V2TransactionSet in a hot loop, which seems unlikely.

[lukechampine]

Closing in favor of #260