Zombienet tests - disputes on finalized blocks

[Overkillus]

Overview:
Adding an extra malus variant focusing on disputing finalized blocks. It will:

• wrap around approval-voting
• listen to OverseerSignal::BlockFinalized and when encountered start a dispute for the dispute_offsetth ancestor
• simply pass through all other messages and signals

Add zombienet tests testing various edgecases:

• disputing freshly finalized blocks
• disputing stale finalized blocks
• disputing eagerly pruned finalized blocks (might be separate PR)

TODO:

• Register new malus variant
• Simple pass through wrapper (approval-voting)
• Simple network definition
• Listen to block finalizations
• Fetch ancestor hash
• Fetch session index
• Fetch candidate
• Construct and send dispute message
• zndsl test 1 checking that disputes on fresh finalizations resolve valid Closes Disputes of recently finalized candidates should conclude in favor #1365
• zndsl test 2 checking that disputes for too old finalized blocks are not possible Closes Disputes of old finalized candidates should not be possible #1364
• zndsl test 3 checking that disputes for candidates with eagerly pruned relay parent state are handled correctly Ensure candidate validation, disputes and approvals work, even if the candidate's relay parent state was pruned. #1359 (deferred to a separate PR)
• Unit tests for new malus variant (testing cli etc)
• Clean/streamline error handling
• Ensure it tests properly on session boundaries

[mrcnski]

Nice! Some nits but nothing blocking. Just a few points:

1. Maybe a dumb question but are the new tests hooked up to CI already?
2. You can put, for example, Closes #1364 in the PR description to automatically close the issue when this is merged.
3. Need to run cargo +nightly fmt --all. You can put this in a git pre-commit hook to make sure it always runs.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4459855

[ordian]

The malus variant looks good, but we should try to minimize the resources allocated to zombienet tests to a minimum, considering they are being run for every commit on every PR, which adds up to the cost.

[ordian]

jFYI, spawn_blocking means that this future will get its own thread (as opposed to being shared on a threadpool), not that this call will block until fully executed. we usually use that for CPU heavy futures, which is not the case here

[mrcnski]

I think I saw at least one other zombienet test that did spawn_blocking, can you check if it's needed there? I guess probably not.

[mrcnski]

zombienet test

I mean malus variant

[Overkillus]

I think I saw at least one other zombienet test that did spawn_blocking

In fact every malus variant use spawn_blocking as it's the implementation of the shared interceptor logic (common.rs) as well as the 2 custom interceptors (the new one I added here and suggest_garbage_candidate). None of them seem super heavy as the common is simply faking validation and dumps precomputed results.

@sandreim do we wanna keep it as the spawn_blocking? Why was it blocking in the first place? example in common.rs

[sandreim]

Yes, we don't need spawn_blocking in any place in malus. Let's switch to spawn

[mrcnski]

we should try to minimize the resources allocated to zombienet tests to a minimum, considering they are being run for every commit on every PR, which adds up to the cost.

Feels like there should be some optimizations possible here. Maybe only run zombienet tests when there is a node label? 🤔 Or if changes in the polkadot/node directory are detected.

[sandreim]

Thanks @Overkillus! Nice work!

[sandreim]

Yes, we don't need spawn_blocking in any place in malus. Let's switch to spawn