feat(block-producer): support transaction expiration

[Mirko-von-Leipzig]

This PR adds support for transaction expirations.

(Additionally also removes a dangling file)

Transaction expiration is tracked in a separate struct which bidirectionally maps between transactions and their expiration height.

Closes #508.

[bobbinth]

Thank you! Looks good! I left some comments inline - mostly nits. A couple of more general questions:

First, should we not also account for expiring batches? Basically, if we have a batch that contains a transaction that is now expired, we should probably not try to put it into a block. For this, we may need to add expires_at field to transaction batch and it would be set to the min of all transaction expiration times for this batch.

Also, an alternative approach to handling this would be to move the transaction expiration logic into the transaction graph. Specifically, in TransactionGraph::select_batch() we could check if a selected transaction has expired or not. If it has, we'd discard it and remove all of its dependents, and then move to selecting the next transaction. This approach could have the following benefits:

1. We don't really need to track transaction expirations in a separate struct.
2. The Mempool struct doesn't need to know anything about transaction expiration logic.

The latter could be useful once we decided to optimize transaction selection logic to give some priority to transactions which could expire soon and this logic will be fully encapsulated in the transaction graph.

[bobbinth]

nit: since we are technically removing batches (and not transactions), should we rename this into something like remove_batches_with_transactions()?

[bobbinth]

Should we be a bit more aggressive here? For example, a transaction which expires in the next block is almost certainly not going to make it as we need to put it into the batch first and then the batch needs to make it into the block. Maybe it could be something like:

if tx.expires.at() <= self.chain_tip + 1 {
    ...
}

Instead of hard-coding +1, we could also have a constant for it.

[bobbinth]

Let's add a brief doc comment here as well.

[bobbinth]

nit: should we move the tests into a separate file? (e.g., src/mempool/tests.rs)

[bobbinth]

Let's create an issue to address this in the future (unless we already have one).

[bobbinth]

nit: should we name this variable batch_id to make thins a bit more explicit?

[Mirko-von-Leipzig]

First, should we not also account for expiring batches? Basically, if we have a batch that contains a transaction that is now expired, we should probably not try to put it into a block. For this, we may need to add expires_at field to transaction batch and it would be set to the min of all transaction expiration times for this batch.

This should be handled automatically since the expiration tracking is done for all transactions, even those in batches. For every transaction that expires we:

1. Revert it and all of its descendants, and then
2. Revert all batches associated with these transactions, and then
3. Requeue transactions from (2) that are not in (1)

So (2) handles the batch expiring.

Also, an alternative approach to handling this would be to move the transaction expiration logic into the transaction graph. Specifically, in TransactionGraph::select_batch() we could check if a selected transaction has expired or not. If it has, we'd discard it and remove all of its dependents, and then move to selecting the next transaction. This approach could have the following benefits:

1. We don't really need to track transaction expirations in a separate struct.
2. The Mempool struct doesn't need to know anything about transaction expiration logic.

The latter could be useful once we decided to optimize transaction selection logic to give some priority to transactions which could expire soon and this logic will be fully encapsulated in the transaction graph.

We could do something like this sure. Then we do need separate transaction and batch expiration tracking, and the graphs will have to be aware of the chain tip. The downside is that selecting a batch/block suddenly comes with additional side effects, now you also need to handle reverting things in the mempool.

Additionally, select_batch is probably too late to do this; the transactions technically expire when the block is committed. So there'll be this additional period where you'll accept new dependent transactions building on expired transactions.

Ideally we would keep the expiration check in Mempool::block_commited/failed, and then the checks would remain similar to now, except TransactionExpiration would be handled by the two graph types.

This does improve selection options in the future so maybe its worth trying out. I'll code this up and diff it against the current PR.

Overall I wonder if its enough to abstract the key selection information within each graph separately, or if one day we'll want the selection strategy to have access to the entire mempool's data. As in, batch selection will want to know about the transaction graph data and vice versa.

[bobbinth]

This should be handled automatically since the expiration tracking is done for all transactions, even those in batches. For every transaction that expires we:

1. Revert it and all of its descendants, and then
2. Revert all batches associated with these transactions, and then
3. Requeue transactions from (2) that are not in (1)

Ah! Makes sense - somehow I missed this.

We could do something like this sure. Then we do need separate transaction and batch expiration tracking, and the graphs will have to be aware of the chain tip. The downside is that selecting a batch/block suddenly comes with additional side effects, now you also need to handle reverting things in the mempool.

Do you mean that if we detect an expired transaction, we'd also need to remove the associated batches from the batch graph? If so, it does complicate things, but I wonder if there is a way to keep these graphs disjointed.

Additionally, select_batch is probably too late to do this; the transactions technically expire when the block is committed. So there'll be this additional period where you'll accept new dependent transactions building on expired transactions.

We should be able to address this by "materializing" transaction expiration block in AuthenticatedTransaction. That is, child transactions would "inherit" expiration time of their parents. This way, can can immediately check if a transaction depends on an already expired parent.