Cosign tests

[rafael-xmr]

Changes:

common/task/src/lib.rs

1) [bug] wrong usage of .max():

This line:

// Set a limit of sleeping for two minutes
*current_sleep_before_next_task = new_sleep.max(Self::MAX_DELAY_BETWEEN_ITERATIONS);

Had a bug. As the .max() typedocs define:

Compares and returns the maximum of two values.

Returns the second argument if the comparison determines them to be equal.

Examples

assert_eq!(1.max(2), 2);
assert_eq!(2.max(2), 2);

The intended use of .max() here was to, given a new_sleep time, and a maximum constant value in Self::MAX_DELAY_BETWEEN_ITERATIONS, the new_sleep time should never be greater than the constant and if greater, always default the sleeping time to the constant's time instead. Otherwise, a forever increasing new_sleep could occur and hold tasks indefinitely.

2) [feat] Made the addition of test-helpers behind a cargo flag, to help with the testing on the running of tasks.

coordinator/cosign/src/intend.rs

1) [bug] Zero-stake Event::SetKeys would panic

Identified a bug would panic every time on a Notable Event::SetKeys event where the total stake amounts to 0, and would always come back to panic preventing the cosign task from ever continuing. A 0 total stake notable event scenario is a possibility and this should be handled instead, for example at network's launch as there is a genesis period where liquidity is provided before any coins and any stake yet exist. The solution being adding the following:

// Handle declarations of the latest set
for event in vset_events.set_keys_events() {
  let Event::SetKeys { set, key_pair } = event else {
    unreachable!("event from `set_keys_events` wasn't `Event::SetKeys`")
  };

  let validators = Validators::take(&mut txn, *set)
    // critical panic:
    // this is a critical issue and will not be solved after re-tries,
    // missing Validators from previous blocks will remain missing until re-indexed
    // if encountered halt the process
    .expect("set which wasn't decided set keys");

  let stake: u64 = validators
    .iter()
    .map(|v| Stakes::get(&txn, set.network, *v).unwrap_or(Amount(0)).0)
    .sum();

  // Sets with 0 stake should be skipped and not considered w.r.t. cosigning
  // for no set with stake then has_events will remain HasEvents::No for this block and ignored
  if stake > 0 {
    has_events = HasEvents::Notable;
    LatestSet::set(
      &mut txn,
      set.network,
      &Set { session: set.session, key: key_pair.0, stake: Amount(stake) },
    );
  } else {
    serai_env::trace!(
      "{block_number}: skipped session {:?} of {:?} with 0 stake from being selected for cosigns",
      set.session,
      set.network
    );
  }
}

Where not only the if stake > 0 { was added to guard the addition of a LatestSet but also has_events = HasEvents::Notable; is only set as a Notable event type there to guard against the later condition that initiates a new global session for this event. Events with 0 stake don't need to be considered as Notable and can be skipped w.r.t. the cosigning protocol.

2) [feat] Empty validator set from Event::SetDecided

Added a sanity check:

if validators.is_empty() {
    panic!("validator set from Event::SetDecided was empty");
}

3) [bug] Block indexing start

The current implementation begins indexing by block 1, changed it to:

let start_block_number = ScanCosignFrom::get(&self.db).unwrap_or(0);

Otherwise gets the task permanently stuck with error node's block #1 doesn't build upon the block #0 prior indexed if genesis was not indexed.

coordinator/cosign/src/evaluator.rs

1) [bug] added initial check for a global session because it is not possible to evaluate cosigns for a non-existing global session, so skip.

Now the evaluator initiates a new BlockEvent with:

// If no session is being evaluated yet, check if this block can be processed
if currently_evaluated_global_session(&txn).is_none() {
  match GlobalSessionsChannel::peek(&txn) {
    // No global session declared yet: this block predates all sessions, skip it
    // this means only HasEvents:No blocks have been consumed so far
    None => {
      commit_evaluated_block(txn, block_number, false);
      made_progress = true;
      continue;
    }
    // Session queued but starts after this block, skip it
    Some(next) if next.1.start_block_number > block_number => {
      commit_evaluated_block(txn, block_number, false);
      made_progress = true;
      continue;
    }
    // Session covers this block: proceed normally
    _ => {}
  }
}

2) [feat] added has_events to CosignedBlocks so delay can skip no event blocks

Now the db entry looks like:

db_channel!(
  SubstrateCosignEvaluatorChannels {
    // (cosigned block, time cosign was evaluated, has_events)
    CosignedBlocks: () -> (u64, u64, bool),
  }
);

and the addition of the bool for has_events allows the delay task to not need to sleep on HasEvents::No blocks

coordinator/cosign/src/delay.rs

1) [feat] sanity check to avoid index regression

Simply made it consider skipping later blocks if already indexed, following a rule to not regress indexing.

if block_number <= latest_cosigned_block_number {...}

2) [feat] skip no event blocks from delay

as explained above with the evaluator, this was added:

// No events means no cosigns to wait for, mark as cosigned immediately
if !has_events {
  LatestCosignedBlockNumber::set(&mut txn, &block_number);
  txn.commit();
  made_progress = true;
  continue;
}

3) [bug] fixed the wrong calculation being used for the time to sleep

Now calculates as

let now_timestamp = now_timestamp().as_secs();
let time_valid_timestamp = time_evaluated + ACKNOWLEDGEMENT_DELAY.as_secs();

coordinator/cosign/src/lib.rs

1) [bug] fixed cosign task handles not being held and being dropped

Now uses a:

// Forget the intend task handle, as dropping the handle would stop the task
// keeps all cosign tasks running in the background
core::mem::forget(intend_task_handle);

2) [bug] fixed Cosigning::latest_cosigned_block_number() always defaulting to 0, as if block 0 was already cosigned

Returns a Result<Option<...>> instead:

/// The latest cosigned block number.
pub fn latest_cosigned_block_number(getter: &impl Get) -> Result<Option<u64>, Faulted> {
    if FaultedSession::get(getter).is_some() {
      Err(Faulted)?;
    }

    Ok(LatestCosignedBlockNumber::get(getter))
}

And is more clear when a block has actually been indexed/cosigned or not

Misc

• Cargo.toml: unexpected_cfgs = { level = "warn", check-cfg = ['cfg(coverage)'] } hides spammy warnings when running tests.
• coordinator/cosign/README.md: re-worded a bit the phrasing in an attempt to make it clearer at least to myself when re-reading

---

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

No output from a Large Language Model (LLM) was included within any of the
contribution.

[kayabaNerve]

This is all exceptionally clear and straighforward tests (and bug fixes) for the cosign module I can say I'm incredibly happy with. While I've left a littany of comments, they're largely just review nits and style as we work together moving forward. The dedicated shim RPC really seems to be the way to go for testing re: the Serai node's behavior, even if I'm not entirely sold on all of the designs of fixtures/abstractions here. Thankfully, because they're test files, I can accept they work and be happy with them in that regard. The usage of coverage also really seems to have been great at clearly establishing all the test cases, and I appreciated the fuzz test re: intend.

[kayabaNerve]

No, actually, but I see why you did this.

If a validator has never staked, they will have 0 allocated as stake.

If they then try to deallocate 0 stake, the amount they're deallocating is less than or equal to the amount allocated, so the system allows it.

It's silly, but it's something I've learned to do over the years. Specifically, it descends from rejecting transferring 0, which is pointless. The reason not to reject transferring 0 is because sometimes, you want to always issue a transfer on a regular basis (one a week), and some weeks, there may not be any value to transfer. When the regular operation occurs, it still occurs, it just transfers 0. If it errored, it may bork the regular operation and screw up the caller.

With that mind, as my personal practice, I write code to never bork on any legitimate amount, even if a silly amount.

[rafael-xmr]

Yes but here it is checking for a previous indexed Stakes object, so it's just a sanity check in the case the task did not succesfully index a stake any size it is, but I added tests here:

https://github.com/rafael-xmr/serai/blob/108754e16a950bf7aa1372e296d0072502ed2cd9/coordinator/cosign/src/tests/intend.rs#L608-L644

To verify that given a stake event, and the existing Stake index, deallocate always works even if 0-denominated, or either stake and deallocate are 0.

But given you say

If a validator has never staked, they will have 0 allocated as stake.

should the stake just be considered 0,when hitting this case on deallocate, even if not indexed in a db, and pass, or should it be indexed as 0, for every existing validator beforehand to then also allow this to pass?

[kayabaNerve]

Per https://github.com/serai-dex/serai/blob/next-polkadot-sdk/LICENSE.md (which this caused me to add), please update the LICENSE files for the new crates and attach the requested DCO :)

Also, should the utilities to test a task simply be part of serai-task? Just a stray thought I had.

[kayabaNerve]

Ugh. When I merged next in, I merged my local next and not the upstream's next. That butchered the history a bit. I've corrected it by manually squash merging 95ea47d into next and force pushing to next, but a bit of weirdness for anyone reviewing the exact Git history there...