refactor: staged writes in tagging stores

[mverzilli]

Third part of the series started with #19445.

This makes the stores related to tagging synchronization work based on staged writes.

[nventuro]

It looks like the capsule changes got here. Can you fix the branch?

[benesjan]

Most of it are nits and would approve if it were not for the sender_tagging_store test. Those seems to be quite messy.

All the naming suggestions are up for a debate. Just felt like plenty of it could be cuter.

BTW really like the whole staging approach you came up with. It's very elegant. Great job!

[mverzilli]

It looks like the capsule changes got here. Can you fix the branch?

Probably a glitch while the capsule changes were squashed by CI, should be fine now

[AztecBot]

Flakey Tests

🤖 says: This CI run detected 3 tests that failed, but were tolerated due to a .test_patterns.yml entry.

\033FLAKED\033 (8;;http://ci.aztec-labs.com/890cc6125eca9baa�890cc6125eca9baa8;;�):  yarn-project/end-to-end/scripts/run_test.sh simple src/e2e_epochs/epochs_invalidate_block.parallel.test.ts "committee member invalidates a block if proposer does not come through" (93s) (code: 1) group:e2e-p2p-epoch-flakes (\033mverzilli\033: code review sender_tagging_store)
\033FLAKED\033 (8;;http://ci.aztec-labs.com/da4405622c3ed5bd�da4405622c3ed5bd8;;�): yarn-project/end-to-end/scripts/run_test.sh web3signer src/composed/web3signer/e2e_multi_validator_node_key_store.test.ts (34s) (code: 1) (\033mverzilli\033: code review sender_tagging_store)
\033FLAKED\033 (8;;http://ci.aztec-labs.com/f40ebadcebf2fa3e�f40ebadcebf2fa3e8;;�):  yarn-project/end-to-end/scripts/run_test.sh simple src/e2e_p2p/broadcasted_invalid_block_proposal_slash.test.ts (202s) (code: 1) group:e2e-p2p-epoch-flakes (\033mverzilli\033: code review sender_tagging_store)

[benesjan]

It's all very cute now. Thanks

I think we might just be missing a few test cases but that's not a sufficient reason to block this so LGTM

[benesjan]

We are not using the same job below though so this seems out of place?

[benesjan]

Suggested change

	expect(await taggingStore.getHighestAgedIndex(secret1, 'job1')).toBe(10);
	// Now we verify that we don't receive the data originally staged in job1
	expect(await taggingStore.getHighestAgedIndex(secret1, 'job1')).toBe(10);

Took me a bit to figure out how exactly this is testing the staged gets cleared.

[benesjan]

I know it's annoying but given that aged and finalized indexes are stored completely separately we should have this test case (and some of the following) duplicated for both type of indexes.

[benesjan]

This one also should have the finalized index counterpart.

[benesjan]

We have 3 write operations on the public interface of the store:

• storePendingIndexes
• dropPendingIndexes
• finalizePendingIndexes

I think for each of these we want to test that:

• they are isolated between jobs,
• discarding works as expected,
• committing works as expected

So we seem to miss some test cases here. Or would you say it's unnecessery?

I mean we use the same private functions under the hood (e.g. #writePendingIndexes) so IDK it might be unnecessary.

Maybe the private functions should be on a separate class such that the higher level functionality could access only the functions that already handle the staging etc.?

That way we could test the staging directly and we would not deduce it works correctly from a higher level behavior.

But anyway think it's fine to merge this and do that in a followup PR if you agree.