TxPool fixes

[lukechampine]

In the course of writing a test for full txpool behavior, I found a bug in the index handling: all of the transactions were being assigned an index of len(txpool.txns), instead of sequential ones! This was almost certainly the result of copy-pasting the m.txpool.indices[txid] = len(m.txpool.txns) from AddPoolTransactions; it breaks in revalidatePool because we're appending to filtered, not m.txpool.txns directly.

This is probably the root cause of #256. We had been blaming parentMap for that, so I ripped it out. I think it should stay ripped out, though, since benchmarks show that the operation it's optimizing is not particularly slow in the first place.

I also added an important validation step to updateV2TransactionProofs. This should prevent the panic seen in #254.

[Copilot]

Pull Request Overview

This PR addresses several transaction pool issues by fixing index assignment, removing an outdated parentMap cache, and strengthening proof validation.

• Fixes incorrect index assignment in revalidatePool to use the filtered slice length.
• Removes the parentMap cache and refactors computeParentMap to build a fresh map.
• Adds a validation step in updateV2TransactionProofs and updates tests to reflect new error handling.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
chain/pool_test.go	Adjust error assertion for AddV2PoolTransactions test
chain/manager_test.go	Add TestFullTxPool to validate full pool behavior
chain/manager.go	Fix index handling, remove parentMap cache, add proof validation

Comments suppressed due to low confidence (2)

chain/manager_test.go:369

• This assertion assumes ids has at least one element. If the loop above breaks early, ids may be empty and cause a panic. Add a length check before indexing.

if _, ok := cm.PoolTransaction(ids[0]); !ok {

chain/manager.go:645

• [nitpick] Recomputing the entire parentMap on each call may degrade performance if this method is invoked frequently. Consider restoring a cache or documenting expected call frequency.

parentMap := make(map[types.Hash256]int)

[lukechampine]

[nitpick] Recomputing the entire parentMap on each call may degrade performance if this method is invoked frequently. Consider restoring a cache or documenting expected call frequency.

:omegalol:

[lukechampine]

ok, slight snag here. The RHPv4 code does this in a few places:

- Fund transaction
- Call cm.V2TransactionSet to update the proofs
- Send transaction to host, receive host inputs
- Sign transaction
- Send signed to host

But this fails, because V2TransactionSet now fully validates the transaction, but the transaction is missing inputs and signatures.

I think we could solve this by not updating the proofs renter-side (since the host will update them anyway, after it has all the signatures). But that doesn't address the issue of V2TransactionSet on invalid inputs. So I think we should instead switch to validating only the Merkle proofs in updateV2TransactionProofs, e.g. by adding a ValidateTransactionElements method to consensus.ElementAccumulator.

[ChrisSchinnerl]

Looking pretty good but broke a few tests it seems.

[n8maninger]

ok, slight snag here. The RHPv4 code does this in a few places:

- Fund transaction
- Call cm.V2TransactionSet to update the proofs
- Send transaction to host, receive host inputs
- Sign transaction
- Send signed to host

But this fails, because V2TransactionSet now fully validates the transaction, but the transaction is missing inputs and signatures.

I think we could solve this by not updating the proofs renter-side (since the host will update them anyway, after it has all the signatures). But that doesn't address the issue of V2TransactionSet on invalid inputs. So I think we should instead switch to validating only the Merkle proofs in updateV2TransactionProofs, e.g. by adding a ValidateTransactionElements method to consensus.ElementAccumulator.

We'd need to grab the txn parents some other way, but should be fine. I initially thought that might introduce a race with the contract elements, but it appears we rely on the host to fill it in already.