feat(block-producer): multiple inflight account txs

[Mirko-von-Leipzig]

Support multiple inflight transactions affecting the same account by tracking the latest inflight state of modified accounts. This is then checked against new txns.

This is likely more a progress check than a final PR - I suspect there are more indirect changes that must be made?

Probably missing: tests ensuring that secondary inflight transactions are rejected if they have the wrong initial state.

Closes #186

[bobbinth]

Thank you! Looks good! I left a few comments inline.

We also need to make sure that the processes of combining transactions into a batch and combining batches into a block work correctly (e.g., see here and here).

[Mirko-von-Leipzig]

I've added the account state tracker; there are still some tests that need updating/fixing.

I'll draft this PR for now.

[Mirko-von-Leipzig]

I've made the suggested changes, and also added support for multiple transactions in the transaction batcher (missing the detail merging as mentioned here).

I looked into any required BlockWitness changes, but I think those might already be covered by my batcher changes which didn't change the API? Might be wrong - still wrapping my head around things.

[bobbinth]

Looks good! Thank you! I left some more comments inline.

I looked into any required BlockWitness changes, but I think those might already be covered by my batcher changes which didn't change the API?

I think the biggest thing we need to make sure is happening is when we have 2 batches with updates to the same account, we'd need to merge their BlockAccountUpdate's. If this isn't happening already, this could be a fairly involved change, and I'm thinking we can create a separate issue for this and tackle it in a future PR.

[Mirko-von-Leipzig]

I think this PR could do with another review :)

I also took a stab at merge support in the block witness in the last commit. I think this is done correctly, though of course there are no tests. I'll add those tomorrow -- just wanted input on my implementation first? No point in testing the wrong assumption :D

[bobbinth]

Looks good! Thank you! I left just a couple of small comments inline.

Also agreed - would be good to add some tests.

[Mirko-von-Leipzig]

Block witness and producer have been updated to support multiple txs as well. I implemented this in the least intrusive way I could find; however the result is a bit ugly imo.

The witness now outputs the block deltas since it is responsible for merging batches. My initial attempts at moving this out met with resistance from the "code shape" being wrong in several locations. A better flow can probably be found by taking a higher level view but that's a large diff left best left for another PR.

I'm leaving the merge conflict until the changes are accepted so that the reviews don't start from a completely fresh rebase.

[Mirko-von-Leipzig]

The validate function became a bit of a liability - it can no longer be performed at the start but instead needs to be done in parts since account state can only be checked after we've merged the batch updates.

[Mirko-von-Leipzig]

I dislike the return type, but this was the smallest delta I could find.

[bobbinth]

Looks good! Thank you! I left just a few small comments inline.

Overall though, I wonder if we should simplify the whole approach somehow. I think things are relatively clean at the batch level, but when we get to the block level, some of the inter-dependencies we need to check worry me a bit.

A good time to think through and potentially refactor this logic would be when we migrate batch/block construction logic to miden-base. Basically, I think miden-base should contain the following logic:

1. Given a list of proven transactions and some additional data (e.g., inclusion paths for unauthenticated note), output a new TransactionBatch.
2. Given a list of transaction batches and some additional data (i.e., what is now called BlockInputs), output a new Block.

On my TODO list is to write up issues about these.

---

Separately, cc @igamigo - we should probably add integration tests in the client to try to test multiple in-flight transactions for the same account.

[Mirko-von-Leipzig]

Overall though, I wonder if we should simplify the whole approach somehow. I think things are relatively clean at the batch level, but when we get to the block level, some of the inter-dependencies we need to check worry me a bit.

A good time to think through and potentially refactor this logic would be when we migrate batch/block construction logic to miden-base. Basically, I think miden-base should contain the following logic:

1. Given a list of proven transactions and some additional data (e.g., inclusion paths for unauthenticated note), output a new `TransactionBatch`.

2. Given a list of transaction batches and some additional data (i.e., what is now called `BlockInputs`), output a new `Block`.

Definitely agree this could be simplified. I like approaching this from the testing point of view. If its difficult to fully test all the variations of the function/type then its probably doing too many things.

If we can pull the store access out then creating tests also becomes easier.

let mut block = BlockBuilder::new();

for (id, batch) in batches.enumerate() {
    block.merge(batch).context("Merging batch {id})?;
}

let store_state = self.store.get_inputs(block.account_list())?;

let block = block
    .create_witness(store_state)?
    .finalize();