fix: mempool occasional panics when store is ahead by a block

[Mirko-von-Leipzig]

This PR fixes an occasional panic when submitting a transaction or batch to the mempool.

The panic is handled correctly, that is, the mempool is not corrupted and continues to function. However, on v0.14 I've observed the panic unwinding holding onto the mempool lock for several seconds, making this more of a priority performance fix.

The bug occurs due to a race condition between a block being committed in the store and the mempool. A block first lands in the store, which means incoming transactions can get authenticated against store.chain_tip, while mempool.chain_tip = store.chain_tip - 1. The mempool then panics since its being given "bad" data from a trusted source.

This PR fixes this issue by having the mempool consider the current pending block as its chain tip, instead of only the latest committed block. This fixes the above issue, and also more correctly models things imo.

Fixes #1856

---

On a separate note, this does highlight that we should consider alternatives to a global mempool mutex.

[igamigo]

LGTM! Left just a couple of minor comments. I think this does change the semantics of add_transaction and add_user_batch minimally, so I wonder if it's worth updating the comments/docs. For instance:

// Submits proven transaction to the Miden network. Returns the node's current block height.
rpc SubmitProvenTransaction(transaction.ProvenTransaction) returns (blockchain.BlockNumber) {}

now may return a block number that has not been committed yet (and may be reverted, etc.). The alternative is to roll back the return values in those functions, but I think in any case the returned value is more useful now (originally it was meant to signal, e.g. after which block number your note may be created).

this does highlight that we should consider alternatives to a global mempool mutex.

Agreed

[igamigo]

I think we may want to rename this. Reading the code, some places still use self.chain_tip and some others have moved to self.chain_tip(), so the difference is not immediately obvious to the reader. Or maybe the field itself could be renamed (committed_chain_tip?)

[Mirko-von-Leipzig]

Ah. I think renaming the field is a great idea, thank you!

[Mirko-von-Leipzig]

@igamigo on the point of the RPC now returning the pending block height.. are you saying that's okay? This was an unintended side effect for me which I am going to revert.

But it sounds like you're in favor of it, or okay with it? My main concern would be that its possible this block does not become committed.

[igamigo]

Yeah, I went back and forth, but I think we want to remove this as well. As I said, that return value is supposed to be a hint for clients to know after which block their transaction would be committed (and so, when output notes would be created in the chain). Since that block number may not be committed, it may not be always accurate (ie we may return block 5 but it may turn out that the transaction we are submitting is committed in block 5).

[bobbinth]

now may return a block number that has not been committed yet (and may be reverted, etc.). The alternative is to roll back the return values in those functions, but I think in any case the returned value is more useful now (originally it was meant to signal, e.g. after which block number your note may be created).

I would keep returning the latest committed block number - this way, there should be no ambiguity about whether we get latest committed or the upcoming block number. The client can then make adjustments (if any needed) to estimate which block the transaction is going to land in.

[Mirko-von-Leipzig]

now may return a block number that has not been committed yet (and may be reverted, etc.). The alternative is to roll back the return values in those functions, but I think in any case the returned value is more useful now (originally it was meant to signal, e.g. after which block number your note may be created).

I would keep returning the latest committed block number - this way, there should be no ambiguity about whether we get latest committed or the upcoming block number. The client can then make adjustments (if any needed) to estimate which block the transaction is going to land in.

I've reverted to this behaviour.