Mempool Simplification

[l-monninger]

Summary

• RFCs: $\emptyset$.
• Categories: protocol-units

Resolves #1058 by using the Aptos Core Mempool to construct batches of transactions.

This may also still resolve #490, however, it is as of yet unclear.

• An evaluation of this revised means of transaction ingress is provided here. Note: there may still be outstanding issues with Gas DoS attacks. These will need to be validated.
• The most important test for validating this resolves sequence_number_too_old error #1058 is the sequencer-number-ooo
• This test can be run with the corresponding overlay just movement-full-node native build.setup.eth-local.celestia-local.test-sequence-number-ooo --keep-project

Changelog

Testing

just movement-full-node native build.setup.eth-local.celestia-local.test-sequence-number-ooo --keep-project
just movement-full-node native build.setup.eth-local.celestia-local.test-gas-dos --keep-project

Outstanding issues

1. movement/protocol-units/execution/maptos/opt-executor/src/background/transaction-pipe.md

   Line 9 in 63e370d

   Aptos will throw a `SEQUENCE_NUMBER_TOO_NEW` error if the sequence number is validated by the VM as too new. This is a more serious error, as it indicates that the transaction is not yet valid. `reject_transaction` does not remove the transaction from the mempool as the user must be required to pay gas for this transaction at some point. If it were not the case, then the user could load up the mempool with transactions that would never be executed. At the time of writing, it would seem this is still possible using the raw Aptos Mempool.

2. If we want to match the usage of mempool by aptos-core itself, we would want to move the self.core_mempool.commit_transasction calls back to occur in the execution loop, i.e., after a block has been executed. This would in turn require reintroducing a synchronization primitive around core_mempool.
3. See comment about try_next and tokio::select being buggy. That tick needs to time out, but isn't right now.

[l-monninger]

The concern is essentially that, without the tolerance, I think users can submit way out into the future, so far that they might never realize gas fees.

This reintroduces the attack the tolerance amended.

And, I don't see anything directly in the mempool which actually prevents this.

If anything it's a side-effect of something in the VM

[l-monninger]

This solves #490 insofar as we it doesn't allow the submission of duplicate transactions by sequence number. But, it's not yet clear to me that a user cannot submit transactions roughly arbitrarily out into the future that would abort on the prologue and not pay gas.

[l-monninger]

@musitdev @0xmovses do you guys understand this PR?

[0xmovses]

I haven't had the time to dig into it. I've perused over it a few times but I need to spend more time on it. After burn cap upgrade test for me.

[0xmovses]

Immediate question is why we didn't see this in previous testnets or bardock. Or did we? Was it reported? If it wasn't it would suggest some regression entered the system that was not caught.

[musitdev]

Yes I think. I've done some test and reproduce some issues.
For the first one, I'm not sure of the expected behavior.
The current behavior is:
If one account send 2 tx at the same time with 2 consecutive seq number 2 cases can arrive:

• the Tx are executed in order correctly.
• The tx arrive in wrong order and one is rejected with SEQUENCE_NUMBER_TOO_NEW error. Still not sure with one is rejected; I've to add more logs.
  Currently, this error is not reported to the sequencer mempool so the test fail with errortiming out waiting for the transaction . I can solve it by reintroducing a synchronization primitive like you say.
  My question:
  If several Tx from an account is received, they are not ordered, so Tx in the wrong order are rejected like the current behavior or do they need to be executed in the right order?