Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance Commit/Reveal Mechanism to Allow Multiple Pending Commits #598

Open
distributedstatemachine opened this issue Jul 1, 2024 · 1 comment

Comments

@distributedstatemachine
Copy link
Contributor

Description

We need to modify our commit/reveal mechanism to allow validators to submit multiple weight commits without overwriting previous ones. The nonce for each commit will be generated by the Python client rather than on-chain.

Acceptance Criteria

  1. Validators should be able to submit multiple weight commits without overwriting previous pending commits.
  2. Each commit should be identified by a client-generated nonce.
  3. Validators should be able to reveal any of their pending commits within the reveal period.
  4. The system should maintain the existing time constraints for the reveal period.
  5. If a reveal is successful, only the revealed commit should be removed from the pending commits.

Tasks

  1. Modify the storage structure to allow multiple pending commits per validator.
#[pallet::storage]
pub type WeightCommits<T: Config> = StorageDoubleMap<
    _,
    Twox64Concat,
    u16, // netuid
    Twox64Concat,
    T::AccountId,
    BTreeMap<u64, (H256, u64)>, // nonce -> (hash, block_number)
    ValueQuery,
>;
  1. Update the do_commit_weights function to add new commits with client-provided nonces.
pub fn do_commit_weights(
    origin: T::RuntimeOrigin,
    netuid: u16,
    commit_hash: H256,
    nonce: u64,
) -> DispatchResult {
    let who = ensure_signed(origin)?;
    // ... existing checks ...

    WeightCommits::<T>::mutate(netuid, &who, |commits| {
        commits.insert(nonce, (commit_hash, frame_system::Pallet::<T>::block_number()));
    });

    Ok(())
}
  1. Modify the do_reveal_weights function to work with the new multi-commit system, allowing reveal of any valid pending commit.
pub fn do_reveal_weights(
    origin: T::RuntimeOrigin,
    netuid: u16,
    uids: Vec<u16>,
    values: Vec<u16>,
    salt: Vec<u16>,
    version_key: u64,
    nonce: u64,
) -> DispatchResult {
    let who = ensure_signed(origin.clone())?;
    
    WeightCommits::<T>::try_mutate_exists(netuid, &who, |maybe_commits| -> DispatchResult {
        let commits = maybe_commits.as_mut().ok_or(Error::<T>::NoWeightsCommitFound)?;
        
        let (commit_hash, commit_block) = commits.get(&nonce)
            .ok_or(Error::<T>::InvalidCommitNonce)?;

        ensure!(
            Self::is_reveal_block_range(netuid, *commit_block),
            Error::<T>::InvalidRevealCommitTempo
        );

        let provided_hash: H256 = BlakeTwo256::hash_of(&(
            who.clone(),
            netuid,
            uids.clone(),
            values.clone(),
            salt.clone(),
            version_key,
        ));
        ensure!(
            provided_hash == *commit_hash,
            Error::<T>::InvalidRevealCommitHashNotMatch
        );

        // All checks have passed, now we can set the weights
        Self::do_set_weights(origin, netuid, uids, values, version_key)?;

        // After successfully setting weights, remove the revealed commit
        commits.remove(&nonce);
        if commits.is_empty() {
            *maybe_commits = None;
        }

        Ok(())
    })
}

Additional Considerations

  • How will the CLI generate and manage nonces to ensure uniqueness?
  • Should there be a limit on the number of pending commits per validator to prevent potential spam or DoS attacks?
@ppolewicz
Copy link

As for considerations:

  1. How will the CLI generate and manage nonces to ensure uniqueness?

Instead of nonces we should use a pointer to the blockchain, extrinistic id such as #3508432-0001 (see https://x.taostats.io/extrinsic/3508432-0001)

Suggested API on the python side:

my_subnet = await subtensor.get_subnet(12)
commit = await my_subnet.commit_weights(uid_to_weight_dict)
# save the commit object to a database in case the validator restarts or something before it can reveal weights
target_block = commit.block + my_subnet.hyperparameters.min_reveal_delay
await subtensor.wait_untiL_block(target_block)
while subtensor.get_current_block() < commit.block+my_subnet.hyperparameters.max_reveal_delay
    return await commit.reveal_weights()
    await asyncio.sleep(5)
return False

then a coroutine which runs after start should see if there are any weights in the db that need to be revealed

  1. Should there be a limit on the number of pending commits per validator to prevent potential spam or DoS attacks?
    max_reveal_delay should be set to a couple days, that should be enough to secure it. If subnets will abuse it, we'll see it, though with the O(1) cost of fetching the hash this shouldn't be a big problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants