Feat/multiple commits #599

distributedstatemachine · 2024-07-01T09:19:35Z

Description

Motivation for change

The current weight commitment system lacks flexibility and doesn't allow validators to queue multiple weight updates. This limitation can lead to inefficiencies in the network, especially during periods of high activity or when validators need to make rapid adjustments to their weights.

Behaviour pre-change

Validators could only commit and reveal one set of weights at a time.
There was no mechanism to handle multiple commits or to enforce the order of processing.
Old commits were not automatically removed, potentially leading to storage bloat.

Behaviour post-change

Validators can now commit multiple weight updates, which are processed in a First-In-First-Out (FIFO) order.
A nonce system has been introduced to ensure monotonicity and prevent replay attacks.
The system now enforces a limit of 10 pending commits per validator, automatically removing the oldest commit when this limit is reached.
Old commits are now automatically removed after successful processing, improving storage efficiency.
The do_commit_weights function now checks for nonce monotonicity and prevents duplicate nonces within the current interval.

Alternatives considered

Batch commits: We considered allowing validators to submit multiple weight updates in a single transaction. This was rejected due to increased complexity in processing and potential issues with transaction size limits.
Time-based expiry: An alternative to the FIFO system was to expire commits based on time. This was not implemented as it could lead to issues with network congestion and block time variations.
Unlimited commits: We considered allowing unlimited pending commits per validator. This was rejected to prevent potential DoS attacks and to limit storage usage.

Related Issue(s)

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

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Other (please describe):

##Breaking Change

If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have run cargo fmt and cargo clippy to ensure my code is formatted and linted correctly
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Screenshots (if applicable)

Please include any relevant screenshots or GIFs that demonstrate the changes made.

Additional Notes

Please provide any additional information or context that may be helpful for reviewers.

pallets/subtensor/src/weights.rs

pallets/subtensor/src/benchmarks.rs

pallets/subtensor/src/errors.rs

orriin · 2024-07-01T13:18:14Z

pallets/subtensor/src/lib.rs

    pub type WeightCommits<T: Config> = StorageDoubleMap<
        _,
        Twox64Concat,
-        u16,
+        u16, // netuid
        Twox64Concat,
        T::AccountId,
-        (H256, u64),
-        OptionQuery,
+        BTreeMap<u64, (H256, u64)>, // nonce -> (hash, block_number)
+        ValueQuery,
    >;


Does this need a migration?

done. Can you please review the implementation and test

Looks good, you should also

put the migration in the OnRuntimeUpgrade trait

add the migration to the runtime

set the in-code pallet storage version in the pallet (see https://github.com/paritytech/polkadot-sdk/blob/a466a490fb20945b834e6c1983cf37daba6fb3e4/substrate/frame/balances/src/lib.rs#L319)

Co-authored-by: orriin <[email protected]>

pallets/subtensor/src/lib.rs

pallets/subtensor/src/weights.rs

camfairchild · 2024-07-12T00:59:17Z

pallets/subtensor/tests/weights.rs

            ),
            Error::<Test>::InvalidRevealCommitHashNotMatch
        );
    });
 }

+#[test]
+fn test_multiple_commit_reveal() {


Also test for out-of-order reveal.

pallets/subtensor/src/migration.rs

pallets/subtensor/src/errors.rs

pallets/subtensor/src/migration.rs

orriin · 2024-07-12T12:51:08Z

pallets/subtensor/src/lib.rs

    pub type WeightCommits<T: Config> = StorageDoubleMap<
        _,
        Twox64Concat,
-        u16,
+        u16, // netuid
        Twox64Concat,
        T::AccountId,
-        (H256, u64),
-        OptionQuery,
+        BTreeMap<u64, (H256, u64)>, // nonce -> (hash, block_number)
+        ValueQuery,
    >;


Looks good, you should also

put the migration in the OnRuntimeUpgrade trait

add the migration to the runtime

set the in-code pallet storage version in the pallet (see https://github.com/paritytech/polkadot-sdk/blob/a466a490fb20945b834e6c1983cf37daba6fb3e4/substrate/frame/balances/src/lib.rs#L319)

pallets/subtensor/src/lib.rs

pallets/subtensor/src/migration.rs

orriin · 2024-07-12T12:54:28Z

pallets/subtensor/src/weights.rs

+        let last_processed_nonce = Self::get_last_processed_nonce(netuid, &who);
+        ensure!(nonce > last_processed_nonce, Error::<T>::NonMonotonicNonce);


Could you brick your subnet by using a nonce that's u64 max? Should we enforce cur nonce + 1 instead?

You couldnt brick your subnet by doing this , what would happen is that you would brick your validator , as the nonces are per validator, not subnet

pallets/subtensor/src/weights.rs

orriin · 2024-07-12T12:56:25Z

pallets/subtensor/src/weights.rs

+    /// Get the last processed nonce for a given netuid and account
+    ///
+    /// # Arguments
+    ///
+    /// * `netuid` - The network ID
+    /// * `account` - The account ID
+    ///
+    /// # Returns
+    ///
+    /// Returns the last processed nonce for the given netuid and account, or 0 if not found
+    ///
+    /// # Note
+    ///
+    /// This function is used to retrieve the last nonce that was processed for a specific account
+    /// in a specific subnet. It's crucial for maintaining the order of transactions and
+    /// preventing replay attacks.
+    pub fn get_last_processed_nonce(netuid: u16, account: &T::AccountId) -> u64 {
+        LastProcessedNonce::<T>::get(netuid, account)
+        // ^ If no nonce is found, we return 0 as the default value
+    }


I think we should just use LastProcessedNonce::<T>::get(netuid, account) directly in-line in our code, instead of adding all this boilerplate

We have not decided on whether or not to start accessing storage values directly in code , and this follows the current pattern of creating getters and setters for storage values.

orriin · 2024-07-12T12:56:41Z

pallets/subtensor/src/weights.rs

+    /// Set the last processed nonce for a given netuid and account
+    ///
+    /// # Arguments
+    ///
+    /// * `netuid` - The network ID
+    /// * `account` - The account ID
+    /// * `nonce` - The nonce to set
+    ///
+    /// # Note
+    ///
+    /// This function updates the last processed nonce for a specific account in a specific subnet.
+    /// It's important for maintaining the state of processed transactions and ensuring
+    /// that each nonce is only processed once.
+    ///
+    /// # TODO
+    ///
+    /// Consider adding error handling or logging for cases where the nonce update fails.
+    pub fn set_last_processed_nonce(netuid: u16, account: &T::AccountId, nonce: u64) {
+        LastProcessedNonce::<T>::insert(netuid, account, nonce);
+        // ^ This operation overwrites any existing nonce for the given netuid and account
+    }


I think we should just use LastProcessedNonce::<T>::insert(netuid, account, nonce) directly in-line in our code, instead of adding all this boilerplate

same as above

Co-authored-by: orriin <[email protected]>

sam0x17

everything looks solid to me

sam0x17 · 2024-09-04T13:32:21Z

pallets/subtensor/src/weights.rs

+            ensure!(!commits.contains_key(&nonce), Error::<T>::DuplicateNonce);
+
+            // Check if there are already 10 commits
+            if commits.len() >= 10 {


do we want to maybe make this configurable and/or are we happy with 10?

The commit-reveal interval duration is configurable per subnet so I think the maximum commits within that interval should be customizable as well.

sam0x17 · 2024-09-05T13:11:40Z

bunch of merge conflicts now btw

The base branch was changed.

…_commits

distributedstatemachine requested review from JohnReedV, orriin, sam0x17, camfairchild, keithtensor, open-junius and gztensor July 1, 2024 09:19

open-junius reviewed Jul 1, 2024

View reviewed changes

pallets/subtensor/src/weights.rs Outdated Show resolved Hide resolved

open-junius reviewed Jul 1, 2024

View reviewed changes

pallets/subtensor/src/weights.rs Outdated Show resolved Hide resolved

open-junius reviewed Jul 1, 2024

View reviewed changes

pallets/subtensor/src/weights.rs Outdated Show resolved Hide resolved

open-junius reviewed Jul 1, 2024

View reviewed changes

pallets/subtensor/src/weights.rs Outdated Show resolved Hide resolved

distributedstatemachine requested a review from open-junius July 1, 2024 12:06

orriin reviewed Jul 1, 2024

View reviewed changes

distributedstatemachine force-pushed the feat/multiple_commits branch from 0b73bd9 to 2da8c95 Compare July 2, 2024 12:13

Samuel Dare and others added 11 commits July 2, 2024 16:17

feat: multiple commits

e19e6c9

chore: lints

48c4762

chore: pr comments , fix benchmarks , lints

44856aa

chore: fmt

d79e5aa

Update pallets/subtensor/src/benchmarks.rs

ef80fa7

Co-authored-by: orriin <[email protected]>

feat: add migrations

cf05234

chore: commit notes

3bbfe96

feat: migrations, monotonic nonces

a6620bf

chore: complete monotonic nonces with tests

2d51580

fix: migration tests

099c405

lints

5d670f8

distributedstatemachine force-pushed the feat/multiple_commits branch from d30b26e to 5d670f8 Compare July 2, 2024 12:22

distributedstatemachine requested a review from orriin July 2, 2024 12:38

chore: fmt

6165807

camfairchild reviewed Jul 12, 2024

View reviewed changes

open-junius reviewed Jul 12, 2024

View reviewed changes

pallets/subtensor/src/migration.rs Outdated Show resolved Hide resolved

open-junius reviewed Jul 12, 2024

View reviewed changes

pallets/subtensor/src/migration.rs Outdated Show resolved Hide resolved

open-junius reviewed Jul 12, 2024

View reviewed changes

pallets/subtensor/src/errors.rs Outdated Show resolved Hide resolved

orriin reviewed Jul 12, 2024

View reviewed changes

Update pallets/subtensor/src/migration.rs

6bdd497

Co-authored-by: orriin <[email protected]>

open-junius mentioned this pull request Jul 18, 2024

update doc for WeightCommits #648

Closed

13 tasks

unconst closed this Jul 23, 2024

distributedstatemachine reopened this Sep 2, 2024

distributedstatemachine requested a review from unconst as a code owner September 2, 2024 10:34

sam0x17 previously approved these changes Sep 4, 2024

View reviewed changes

JohnReedV previously approved these changes Sep 4, 2024

View reviewed changes

distributedstatemachine changed the base branch from main to devnet-ready September 5, 2024 17:04

Merge remote-tracking branch 'origin/devnet-ready' into feat/multiple…

b3c32fb

…_commits

sam0x17 approved these changes Sep 5, 2024

View reviewed changes

distributedstatemachine closed this Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/multiple commits #599

Feat/multiple commits #599

distributedstatemachine commented Jul 1, 2024 •

edited

Loading

orriin Jul 1, 2024

distributedstatemachine Jul 2, 2024

orriin Jul 12, 2024

camfairchild Jul 12, 2024

orriin Jul 12, 2024

orriin Jul 12, 2024

distributedstatemachine Jul 15, 2024

orriin Jul 12, 2024

distributedstatemachine Jul 15, 2024

orriin Jul 12, 2024

distributedstatemachine Jul 15, 2024

sam0x17 left a comment

sam0x17 Sep 4, 2024

JohnReedV Sep 4, 2024

sam0x17 commented Sep 5, 2024

		let last_processed_nonce = Self::get_last_processed_nonce(netuid, &who);
		ensure!(nonce > last_processed_nonce, Error::<T>::NonMonotonicNonce);

Feat/multiple commits #599

Feat/multiple commits #599

Conversation

distributedstatemachine commented Jul 1, 2024 • edited Loading

Description

Motivation for change

Behaviour pre-change

Behaviour post-change

Alternatives considered

Related Issue(s)

Type of Change

Checklist

Screenshots (if applicable)

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam0x17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam0x17 commented Sep 5, 2024

distributedstatemachine commented Jul 1, 2024 •

edited

Loading