Refactor trx_pool to only check for race condition, not other transaction errors to improve i) computation optimization ii) code readibility

[nczhu]

I'm gonna make the following optimization to improve both i) code readability for beginners and ii) handle transaction logic checking a bit more elegantly.

check_transaction is currently being called in the execute extrinsic and the transaction pools internal validate_trx step.

So this means for every valid transaction, check_transaction actually runs twice, iterating over inputs & outputs twice. Once during trx pool stage, and once during actual extrinsic execution.

This seems inefficient && the code is confusing code for the learner.

Specifically:

Current implementation: Pro: Efficient for cases where trx are invalid, transactions are invalidated at the first trx_pool validate_transaction step before runtime even executes the extrinsic. We only iterate through inputs vec once! But this seems not elegant since we want most of the UTXO validation to happen during the execution of the extrinsic. Con:

• Check_transaction implementation is difficult for beginners to understand
• Inefficient for cases where transactions ARE valid. We iterate through inputs/outputs vec twice!

Refactoring will: Have trx pool check for only missinginput case, refactor it out from check_transaction. Pro: Efficient for valid cases where trx are VALID Con:

• We're catching invalid trx during runtime execution of extrinsic, not during trx_pool.

[JoshOrndorff]

One thing to keep in mind is that the tx_pool is in the outer node. That means an attacker may hack his or her node so that the tx_pool skips these validation checks entirely. With the hacked node the attacker can author blocks that contain transactions that would have been rejected at the transaction pool stage in an un-hacked node. When that attack block is gossiped around and imported by other nodes, they should still reject the block because it contains an invalid transaction.

That means all the checks should still happen in the runtime. Although lightening the checks in the tx_pool would be fine.

[nczhu]

One thing to keep in mind is that the tx_pool is in the outer node. That means an attacker may hack his or her node so that the tx_pool skips these validation checks entirely. With the hacked node the attacker can author blocks that contain transactions that would have been rejected at the transaction pool stage in an un-hacked node. When that attack block is gossiped around and imported by other nodes, they should still reject the block because it contains an invalid transaction.

That means all the checks should still happen in the runtime. Although lightening the checks in the tx_pool would be fine.

For sure. that's why I only want trx pool to check for race conditions, not anything else

[nczhu]

Design changes I'll be incorporating:

• trx pool only checks for race conditions; it shouldn't be responsible for business logic
• removed the rest of check_transaction logic from trx pool, since its redundant and should be done in the runtime pallet itself.
• removed trx priority based on leftover value from trx pool, this is a prioritization step that can be done by indiv nodes, since of course, they are financially incentivised to do it already.

[nczhu]

Reverting this change since we're accepting unsigned transactions now.