coordinator/tributary tests (#783)

* feat: add task delay tests

* feat: add intend task tests

* feat(cosign): improve delay iteration txn use, sanity check timestamp, avoid overflow panic

* feat(cosign): refactor intend tests, add better test cases

* fix: the txn commit behavior on delay and even better tests

* feat(cosign): more delay improvements & refactor test into traits

* refactor(cosign): more refactors

* refactor(cosign): minor refactors

* feat(cosign): intend changes, add evaluator

* fix: test failures

* feat(cosign): more tests

* refactor(cosign): simpliy & cleanup delay tests

* feat(cosign): types tests

* refactor(cosign): simpliy, cleanup & get tests ready

* chore(cosign): merge issues

* misc

* Revert "misc"

This reverts commit 4a1cf91.

* misc

* misc2

* feat: adding rpc harness

* feat(cosign): skip 0 stake vsets

* feat(cosign): remove unwanted addition

* chore(log): wrong license

* feat(cosign): misc log & comments

* feat: add test libs & end clean up cosign tests

* feat(tributary): begin tests based off cosign

* feat(tributary): finish transaction.rs tests and add db.rs tests

* feat(tributary): add more db.rs tests

* feat(tributary): add initial scan_block.rs tests

Identified bugs:
- panic in Accumulated::<D>::get
- panic in assert!(slash_report.len() <= f);

* refactor(tributary): move utils to mod.rs

* feat(tributary): refactor db tests, improve handling of overflow attempts and add test cases

* misc

* feat(coordinator/cosign): adding full-stack tests

* feat(env): move serai_log into serai_env

* feat(tests/shim-rpc): add random failure rate

* feat(coordinator/cosign): more touch ups & fix evaluate -> delay flow for no events

* feat(coordinator/cosign): full_stack.rs tests

* feat(coordinator/cosign): initial review changes

* feat(coordinator/cosign): more review comment changes

* refactor(common/task): move test helpers from external crate

* refactor(coordinator/cosign): self-reviewing & cleaning up

* some misc

* refactor(coordinator/cosign): self-reviewing & cleaning up tests

* feat(cosign/tests/full_stack): add dos offline cosigns stall scenario & re-word readme.md

* refactor(coordinator/tributary): bringing in updates from cosign work, refactoring & improving some tests

* feat(coordinator/tributary): adding changes and new test cases

* feat(coordinator/tributary): finish the coverage adding scanning block and scanning tributary tests

* feat(substrate/test_helpers): missing test utility

* chore(coordinator/tributary): initial self-review by comparing changes

* chore(coordinator/tributary): missing misc

* chore: ci alerts

* chore(coordinator/tributary): fix match same arms

* chore: lock

* chore(coordinator/tributary): clippy errors

* chore(coordinator/tributary): clippy errors

* chore(coordinator/tributary): fix panics

* chore(coordinator/tributary): clippy errors

* chore(coordinator/tributary): clippy errors

* chore(coordinator/tributary): fmt errors

* Checkout the `Cargo.lock` from the latest commit from `next` this descends from

This reverts a myriad of `cargo update`s.

* Use a `u64` for attempts on the Tributary

A `u32`, when incremented by one, is technically feasible to overflow. This PR
changed it to return `None` for the next topic if it did overflow, but that'd
technically stop creating topics for ongoing protocols. This transformed a
panic (which would crash the entire `coordinator`) into a liveness failure for
this specific Tributary due to inaccurately claiming there was no next topic
(an unsound definition which would become a more localized liveness issue).

This uses a `u64` to ensure this state is unreachable when started from zero,
and incremented by one, unless so many years pass this is definitively
irrelevant.

* Correct the fix for the slash report's length exceeding `f`

Only pushing some elements meant that this vector no longer corresponded to the
list of validators. Since each element wasn't paired with a validator's ID, it
became meaningless. The `assert` which triggered was the underlying fault, and
it's been adjusted to only check the amount of non-zero elements is now less
than or equal to `f`.

* Restore `Topic::required_participation`

While it currently _can_ be collapsed to the function `required_participation`,
the ability for per-topic thresholds is desired.

The calculation is also updated to be infallible.

* Restore `to_signed` taking a nonce

Moving to `SigningProtocolRound` would be nice if all such transactions fit
that model. As shown, not all transactions do. Kludging them into
`SigningProtocolRound` isn't preferred, and it appears to have been improperly
done for `SigningProtocolRound` (causing a `clippy` lint which was allowed).

* Ban `borsh::from_reader` for reasons identified by @rafael-xmr

* Tidy the test helpers

Some are made more flexible, removing the need for other approximate variants.
Test helpers which encoded a non-syntactic semantic context via their name have
been removed outright, especially as `substrate/primitives` shouldn't have
helpers for Coordinator-specific definitions which may not be universal or
applicable to the Substrate context.

Similarly, these should likely be moved to `tests/helpers` where we have more
flexibility. We want to publish `substrate/primitives` but I couldn't care less
about the idea of publishing these test helpers and committing to them under
our API in any way. While the `Cargo.toml` already has a comment on the
`test-helpers` feature, their own crate entirely avoid any contamination here.

* Remove the `Preprocess`, `Share`, `GenericSignPayload`, `RoundPayloads` type aliases

I understand the intent. Unfortunately, these definitions were incorrect.

For a FROST protocol over Ristretto, the preprocess will be 64 bytes and the
share 32 bytes. This is used for the DKG confirmation, batches, where the
former hard-coded such a definition. For batches, the process is routed via
`VariantSignId` where the same processing occurs for transactions. Signing
transactions on external networks has a preprocess, share, of length variable
to the external network and its signing protocol. For Ethereum, it'd be a
66-byte preprocess and 32-byte share. For Bitcoin, it's a 64-byte preprocess
and 32-byte share _per input_ (concatenated into a single byte blob). For
Monero, it's a 160-byte preprocess and 32-byte share _per input_ (again so
concatenated).

* Redefine how Tributary genesis values are decided

The tests in `coordinator/tributary/src/tests/scan_tributary.rs` demonstrated
creating a tributary with one `NewSetInformation`, before reading from it with
another, which should be obviously invalid? `NewSetInformation::genesis` has
been added to make the genesis deterministic and binding to the
`NewSetInformation`, allowing the `ScanTributaryTask` to check its initialized
with values consistent to how the Tributary itself was initialized.

This updates the cited tests accordingly.

* Tweak some `scan_block` tests

One has an unclear story. Others have a pattern of checking a single message
(less than the threshold) causes no events, but doesn't check all messages less
than the threshold cause no events.

* Note other incongruent test

* Nits

Some of these fix behavior which was updated with prior commits, but for which
all the tests/cases had yet to be updated.

---------

Co-authored-by: Luke Parker <lukeparker5132@gmail.com>

Author: rafael-xmr

Cargo.lock

@@ -5,6 +5,10 @@ path = "Option::map_or"
 [[disallowed-methods]]
 path = "Option::map_or_else"
 
+[[disallowed-methods]]
+path = "borsh::from_reader"
+reason = "`borsh::from_reader` errors if there's any bytes following the read item, which isn't documented behavior"
+
 # TODO: https://github.com/rust-lang/rust-clippy/pull/12194
 [[disallowed-methods]]
 path = "<ruint::Uint as core::ops::Add>::add"

clippy.toml

@@ -21,7 +21,6 @@ zeroize = { version = "^1.5", default-features = false, features = ["std"] }
 bitvec = { version = "1", default-features = false, features = ["std"] }
 rand_core = { version = "0.6", default-features = false, features = ["std"] }
 
-blake2 = { version = "0.11.0-rc.5", default-features = false, features = ["alloc"] }
 schnorrkel = { version = "0.11", default-features = false, features = ["std"] }
 
 dalek-ff-group = { path = "../crypto/dalek-ff-group", default-features = false, features = ["std"] }

coordinator/Cargo.toml

@@ -131,7 +131,7 @@ fn signed_cosign(
       block_hash: random_block_hash(&mut OsRng),
       cosigner,
     },
-    signature: random_bytes_64(&mut OsRng),
+    signature: random_bytes(&mut OsRng),
   }
 }
 

coordinator/cosign/src/tests/evaluator.rs

@@ -21,7 +21,7 @@ pub fn random_external_network_id(rng: &mut (impl RngCore + CryptoRng)) -> Exter
 
 /// Generate a random global session ID (`[u8; 32]`).
 pub fn random_global_session<R: RngCore + CryptoRng>(rng: &mut R) -> [u8; 32] {
-  serai_primitives::test_helpers::random_bytes_32(rng)
+  serai_primitives::test_helpers::random_bytes(rng)
 }
 
 /// Generate a random [`Cosign`] for testing.

coordinator/cosign/types/src/tests/mod.rs

@@ -108,9 +108,9 @@ fn handle_frost_error<T>(result: Result<T, FrostError>) -> Result<T, Participant
 
 #[rustfmt::skip]
 enum Signer {
-  Preprocess { attempt: u32, seed: CachedPreprocess, preprocess: [u8; 64] },
+  Preprocess { attempt: u64, seed: CachedPreprocess, preprocess: [u8; 64] },
   Share {
-    attempt: u32,
+    attempt: u64,
     musig_validators: Vec<SeraiAddress>,
     share: [u8; 32],
     machine: Box<AlgorithmSignatureMachine<Ristretto, Schnorrkel>>,
@@ -151,7 +151,7 @@ impl<CD: DbTrait, TD: DbTrait> ConfirmDkgTask<CD, TD> {
   fn preprocess(
     db: &mut CD,
     set: ExternalValidatorSet,
-    attempt: u32,
+    attempt: u64,
     key: Zeroizing<<Ristretto as WrappedGroup>::F>,
     signer: &mut Option<Signer>,
   ) {

coordinator/src/dkg_confirmation.rs

@@ -3,7 +3,6 @@ use std::sync::Arc;
 
 use zeroize::Zeroizing;
 use rand_core::OsRng;
-use blake2::{digest::typenum::U32, Digest as _, Blake2s};
 use ciphersuite::*;
 use dalek_ff_group::Ristretto;
 
@@ -482,8 +481,7 @@ pub(crate) async fn spawn_tributary<P: P2p>(
     return;
   }
 
-  let genesis =
-    <[u8; 32]>::from(Blake2s::<U32>::digest(borsh::to_vec(&(set.serai_block, set.set)).unwrap()));
+  let genesis = set.tributary_genesis();
 
   // Since the Serai block will be finalized, then cosigned, before we handle this, this time will
   // be a couple of minutes stale. While the Tributary will still function with a start time in the

coordinator/src/tributary.rs

@@ -19,6 +19,7 @@ workspace = true
 [dependencies]
 borsh = { version = "1", default-features = false, features = ["std", "derive", "de_strict_order"] }
 
+blake2 = { version = "0.11.0-rc.5", default-features = false, features = ["alloc"] }
 dkg = { path = "../../crypto/dkg", default-features = false, features = ["std"] }
 
 serai-client-serai = { path = "../../substrate/client/serai", default-features = false }

coordinator/substrate/Cargo.toml

@@ -7,6 +7,10 @@ use std::collections::HashMap;
 
 use borsh::{BorshSerialize, BorshDeserialize};
 
+use blake2::{
+  digest::{typenum::U32, Digest as _},
+  Blake2b,
+};
 use dkg::Participant;
 
 use serai_client_serai::abi::{
@@ -80,6 +84,37 @@ impl NewSetInformation {
       self.participant_indexes.insert(*validator, these_is);
     }
   }
+
+  /// Create a new [`NewSetInformation`].
+  pub fn new(
+    set: ExternalValidatorSet,
+    serai_block: [u8; 32],
+    declaration_time: u64,
+    threshold: u16,
+    validators: Vec<(SeraiAddress, u16)>,
+    evrf_public_keys: Vec<([u8; 32], Vec<u8>)>,
+  ) -> Self {
+    let mut result = Self {
+      set,
+      serai_block,
+      declaration_time,
+      threshold,
+      validators,
+      evrf_public_keys,
+      participant_indexes: Default::default(),
+      participant_indexes_reverse_lookup: Default::default(),
+    };
+    result.init_participant_indexes();
+    result
+  }
+}
+
+impl NewSetInformation {
+  /// The hash to use for the genesis of the corresponding Tributary.
+  pub fn tributary_genesis(&self) -> [u8; 32] {
+    // This MUST only hash data completely deterministic to the Substrate blockchain.
+    Blake2b::<U32>::digest(borsh::to_vec(self).unwrap()).into()
+  }
 }
 
 mod _public_db {

coordinator/substrate/src/lib.rs

@@ -28,7 +28,7 @@ dalek-ff-group = { path = "../../crypto/dalek-ff-group", default-features = fals
 dkg = { path = "../../crypto/dkg", default-features = false, features = ["std"] }
 schnorr = { package = "schnorr-signatures", path = "../../crypto/schnorr", default-features = false, features = ["std"] }
 
-serai-primitives = { path = "../../substrate/primitives", default-features = false, features = ["std"] }
+serai-primitives = { path = "../../substrate/primitives", default-features = false, features = ["std", "test-helpers"] }
 
 serai-db = { path = "../../common/db" }
 serai-task = { path = "../../common/task", version = "0.1" }
@@ -40,7 +40,19 @@ serai-coordinator-substrate = { path = "../substrate" }
 
 messages = { package = "serai-processor-messages", path = "../../processor/messages" }
 
-log = { version = "0.4", default-features = false, features = ["std"] }
+serai-env = { path = "../../common/env", version = "0.1.0" }
+
+[dev-dependencies]
+rand = { version = "0.8", default-features = false, features = ["std"] }
+rand_chacha = { version = "0.3", default-features = false, features = ["std"] }
+
+tendermint = { package = "tendermint-machine", path = "../tributary-sdk/tendermint" }
+tributary-sdk = { path = "../tributary-sdk", features = ["tests"] }
+
+tokio = { version = "1", default-features = false, features = ["rt", "time", "macros", "rt-multi-thread"] }
+
+serai-task = { path = "../../common/task", features = ["test-helpers"] }
+serai-substrate-tests = { path = "../../tests/substrate" }
 
 [features]
 longer-reattempts = []