Test builder transaction submission via public mempool

[shenkeyao]

Closes #1968. Closes #1969.

This PR:

• Add tests for transaction submissions via public mempool with reserve and fallback builders.
• Combine and refactor tests for public and private mempools.

This PR does not:

• Test complicated scenarios, which will be added later.

Key places to review:

• Whether the retries make sense.
• Whether sleep is needed put in the right places.
• Whether the submission and bundle fetching are properly done.

How to test this PR:

• Run the four test_marketplace_{reserve/fallback}_builder_with_{public/private}_mempool tests.

[QuentinI]

LGTM with minor comments

[shenkeyao]

I'm not sure whether it's correct to return Some without checking if bundle.transactions is empty. The public mempool tests now fail pretty consistently after I changed it to this way.

[Ayiga]

I think one potential cause of this comes from assumping that the next event we get from the Network is a proposal. I think we should be waiting for a Transaction event before we wait for this Quorum Proposal in order to see the transactions.

We might not actually want to wait for the Quorum Proposal either in these cases. As receiving the Transaction event will cause any requests for subsequent Quorum Proposals to return the transaction list until they are included.

[shenkeyao]

Looks like we can't use a Transaction event directly here, because we need the parent commitment to fetch the bundle. But I agree we shouldn't wait for the QuorumProposal. I'll think about what might be a good alternative.

[shenkeyao]

Oh, maybe I misunderstood your point. Do you mean we just do transaction comparison so there's no need to fetch the bundle?

[Ayiga]

Oh... No I don't think that works, because then we wouldn't be testing the Builder.

My point was to point out that we cannot guarantee that the next proposal would be after the Transactions were submitted to consensus. So we'd really need to use the proposal that preceeds the Transactions message. Since the Transactions message is essentially informing the Builder that these transactions need to be included, we can consider them as not being a part of the public mempool until that message is seen.

But even then, this isn't a guarantee that the Builder will have received this message yet. I think if we don't wait for the Transactions message, then the Bundles that the Builder will hand out will be empty Bundles which will not contain the vec of transactions anyway.

We don't have this issue with the private mempool, as the transactions are being handed directly to the Builder rather than coming in through the Events Service.

[Ayiga]

I should add that I tried adding this yesterday, but it didn't seem to result in a pass... instead I received errors about not being able to receive a bundle.

[shenkeyao]

As discussed I moved the quorum proposal handling earlier, so it corresponds to the parent view now, and the tests pass!

[Ayiga]

I think it would be better to rewrite this test to replace the bool in this test with an enum that can add clarity to each of these cases. Since both forks end with an Event stream in either case, it would also be a natural place for the conditional to exist instead of within the body of this function.

[shenkeyao]

Since both forks end with an Event stream in either case, it would also be a natural place for the conditional to exist instead of within the body of this function.

Could you clarify this? Besides replacing the bool with an enum, are you suggesting some other change?

[Ayiga]

I suppose this is more of a nit, and it's likely a personal preference thing. I don't really care for passing a raw bool into a function / method call, as it doesn't convey any context of what it signifies in isolation. It's only with the function signature, which would require manual inspection, that you would get the relevant context.

With the enum you could convey part of the context at the call site.

test_marketplace_fallback_builder(true);

versus

test_marketplace_fallback_builder(Mempool::Public)

The other piece relates to the conditional inspection of the replaced variable within the test_marketplace_fallback_builder itself. With the enum, you could replace the conditional of:

if public_mempool {
  ...
} else {
  ...
}

With a method call, so that this conditional is not required / evaluated within the body of the test itself. It helps the branch logic look cleaner in the test, as we end up just focusing on the result that we are inspecting rather than worrying about what action to perform based on a conditional branch.

mempool_behavior.submit_transactions(txs).await;
let bundle = mempool_behavior.get_next_bundle().await;

[shenkeyao]

All make sense! I replaced bool with enum and reorganized functions. There are some parts that can't be combined or extracted easily, e.g., due to the public and private mempools using different event streams, but I added some helper functions to simplify things.

[shenkeyao]

The latest commit is just to sync with main.

Tests have been fixed.