vms/platformvm: Remove MempoolTxVerifier

[dhrubabasu]

Why this should be merged

MempoolTxVerifier is a thin wrapper on StandardTxVerifier. It is only used in the P-Chain block executor manager VerifyTx function. I found this pattern confusing when trying to figure out how mempool txs were being verified. This is an attempt at making this process easier to understand.

How this works

In-lines MempoolTxVerifier logic to improve readability

How this was tested

CI

[dhrubabasu]

We run all the txs through the StandardTxExecutor already which the MempoolTxVerifier called internally anyway. This can be removed outside of this PR.

[abi87]

I think this is a step in the wrong direction:

1. I am fine having small structs that do one clear thing, even if a small one. To me MempoolTxVerifier says clearly that mempool txs are validated not on chain tip state (as you may naively assume) but on a state version built on top of it. MempoolTxVerifier does call then the StandardTxExecutor, which tells us that, apart from the state, we reuse the very same code to validator blocks and mempool txs. This seems good engineering to me
2. Block manager is already too big and it's doing many so many stuff that should be break up. The way I see it currently:
   • Support blocks verification, acceptance and rejection
   • Holds chain state at different heights (probably it's main function)
   • Verify mempool txs, which does not make sense at all to me, but that I accepted as a temporary solution to move forward with the mempool cleanup.

I think if we want to keep refactoring code, we should focus on breaking block Manager into smaller chunks, rather than extending its scope.

[dhrubabasu]

In-lining the struct can save us one alloc/op (and is marginally faster). Since this is run for each tx received from AppGossip, I think we should consider in-lining it. The manager already has the VerifyTx function so this PR does not make the interface any bigger

// dev branch
Benchmark/manager.VerifyTx-10                  1537694              7424 ns/op            4744 B/op          62 allocs/op

// this PR
Benchmark/manager.VerifyTx-10                  1678203              7138 ns/op            4680 B/op          61 allocs/op

Unfortunately we can't re-use the MempoolTxVerifier logic when we're verifying the mempool txs before putting them into a block so this logic is only used here (#2359)

[abi87]

just a question about naming. Other than that LGTM

[abi87]

q: since we're dropping the mempool verifier, should we rename this VerifyTx method as VerifyMempoolTx?

[dhrubabasu]

I think we already use VerifyTx exclusively for mempool verification (including in the network pkg). Since we're not modifying the function usage, I think the same name is okay and keeps the diff minimal