feat: Submit a slot number alongside nonce #5297

marcellorigotti · 2024-09-25T12:40:08Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Updated the Change Electoral system to Nonce Electoral system, which takes both nonce value and slot as votes and update the nonce to the new value only if the new value is different than the previous one and the slot is higher than the previous one.

engine/src/witness/sol/nonce_witnessing.rs

codecov · 2024-09-25T16:41:02Z

Codecov Report

Attention: Patch coverage is 34.96503% with 93 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (ab7104b) to head (5b18b78).

Files with missing lines	Patch %	Lines
...in/pallets/cf-elections/src/vote_storage/change.rs	2%	58 Missing ⚠️
...lections/src/electoral_systems/monotonic_change.rs	72%	7 Missing and 6 partials ⚠️
engine/src/witness/sol.rs	0%	9 Missing ⚠️
engine/src/witness/sol/nonce_witnessing.rs	0%	9 Missing ⚠️
state-chain/pallets/cf-environment/src/lib.rs	0%	3 Missing ⚠️
...cf-elections/src/electoral_systems/mocks/access.rs	86%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5297    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        488     489     +1     
  Lines      84898   84876    -22     
  Branches   84898   84876    -22     
======================================
- Hits       60375   60229   -146     
- Misses     21822   21932   +110     
- Partials    2701    2715    +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dandanlen

Looks good to me, just some minor comments mostly around naming. I'll take a closer look at tests on Monday.

dandanlen · 2024-09-27T12:19:19Z

state-chain/runtime/src/lib.rs

@@ -724,6 +724,7 @@ impl pallet_cf_governance::Config for Runtime {
 	type UpgradeCondition = (
 		pallet_cf_validator::NotDuringRotation<Runtime>,
 		pallet_cf_swapping::NoPendingSwaps<Runtime>,
+		pallet_cf_environment::NoUsedNonce<Runtime>,


If you write this as a nested tuple then you don't need to change the trait definition:

type UpgradeCondition = ( pallet_cf_validator::NotDuringRotation<Runtime>, ( pallet_cf_swapping::NoPendingSwaps<Runtime>, pallet_cf_environment::NoUsedNonce<Runtime>, ) );

dandanlen · 2024-09-27T12:19:50Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

There's a typo in the file name.

dandanlen · 2024-09-27T12:23:28Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

 		Settings: Member + Parameter + MaybeSerializeDeserialize + Eq,
 		Hook: OnChangeHook<Identifier, Value> + 'static,
 		ValidatorId: Member + Parameter + Ord + MaybeSerializeDeserialize,
-	> Change<Identifier, Value, Settings, Hook, ValidatorId>
+	> NonceWitnessing<Identifier, Value, Slot, Settings, Hook, ValidatorId>


Naming nit: we rename this to something specific (nonce witnessing instead of change, 'Slot' which is Solana-specific) but haven't renamed any of the generics to match this more specific purpose (for example nonce account instead of identifier).

I would suggest either we keep a more general name (MonotonicChange? BlockHeight?), or we rename the generics to match the more specific use case. This would make the code easier to reason about (no need to remember whether value is the nonce or the address etc.).

imo, making the names more specific is better in this case

Why? (Would be good to give a reason otherwise it's just a question of who has the most willingness to argue 😅 )

yeah i don't think it matters much, but unless it has a general use better to name it specific. Like we don't name every parameter "variable", the name becomes more general as what it is actually doing becomes more general. Also there are no doubts about how it's intended to be used when it's specifically named.

Yes, but in this case, we're implementing a type with a bunch of generics, so it's pretty clear that it's supposed to work generically. And of course I'm not saying we should name everything variable and thing. What I mean is that naming should be as specific as possible within its own context. In this particular case, for example, there's nothing in the functionality of the implementation that is nonce-specific.

dandanlen · 2024-09-27T12:32:56Z

state-chain/pallets/cf-elections/src/vote_storage/nonce.rs

+	) -> Result<VoteComponents<Self>, CorruptStorageError> {
+		Ok(VoteComponents {
+			bitmap_component: Some(partial_vote.value),
+			individual_component: Some((_properties, partial_vote.slot)),


Why are the properties used here?

edit: I see now, these are vote properties () not election properties. Still, they value should have a leading underscore unless it's unused.

dandanlen · 2024-09-27T12:48:51Z

state-chain/pallets/cf-elections/src/vote_storage/nonce.rs

+use crate::{CorruptStorageError, SharedDataHash};
+
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, TypeInfo)]
+pub struct NonceVote<Value, Slot> {


Similar to the other comment, this vote type seems like it could be used more generally, so maybe there is a better / more general name than NonceVote? MonotonicChangeVote or something?

I think we can generalise it when it necessary? else it's a little confusing it has a general name and is used for one (quite specific) thing. Though, if we go the way of composing the existing vote storage then this conversation is void.

My view is that if we implement something generically, then it should have generic names. Reading generic code with specifically-named values is confusing because you always have the specific use-case in mind.

dandanlen · 2024-09-27T13:40:50Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

+				counts
+					.entry(vote.value)
+					.and_modify(|(count, slots)| {
+						*count += 1;


We don't really need this count, it's already implicitly tracked in the length of slots.

dandanlen · 2024-09-27T13:42:51Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

-					Some(vote.clone())
+					let mut slots = slots.clone();
+					let num_slots = slots.len() as u32;
+					let (_, median_vote, _) = {


Let's avoid calling it the median vote, it's not a median ;)

consensus_slot or consensus_height if we're using more general naming?

dandanlen · 2024-09-27T13:50:31Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

-				if previous_value != value {
+			if let Some((value, slot)) = election_access.check_consensus()?.has_consensus() {
+				let (identifier, previous_value, previous_slot) = election_access.properties()?;
+				if previous_value != value && slot > previous_slot {


Due to the implementation of is_vote_valid, I think this statement should always be true (It's not possible to vote, and therefore not possible to gain consensus, if this condition is violated). So as an extra safety measure, we could add and else { log_or_panic!(..) here to catch any logic errors?

Here we have the on-chain logic, which should prevent it on its own, without having to rely on correct behaviour from the engines. So I don't mind having this here as a way of ensuring the chain logic is valid independent of engine behaviour - though I do think there should be a comment here that we don't expect this to be false because in general the engines should behave.

Yes I'm not saying we remove it, just that we should add something so that we don't silently fail if the assumption is wrong.

dandanlen · 2024-09-27T14:48:04Z

state-chain/pallets/cf-environment/src/lib.rs

+
+impl<T: Config> ExecutionCondition for NoUsedNonce<T> {
+	fn is_satisfied() -> bool {
+		SolanaAvailableNonceAccounts::<T>::get().len() == 10


I thought it was 7 😅

Could we not use SolanaUnavailableNonceAccounts::<T>::iter().next().is_none()?

It was originally 7 indeed. However, I did some optimizations to be able to bump it up to ten, as that is the bottleneck.

Sorry, should been less indirect: we can't assume that it will always be 10. We've already change it once, we might change it again. So the implementation should not assume the number.

engine/src/witness/sol.rs

kylezs · 2024-10-01T07:09:03Z

state-chain/pallets/cf-elections/src/electoral_systems/nonce_wintessing.rs

 		Settings: Member + Parameter + MaybeSerializeDeserialize + Eq,
 		Hook: OnChangeHook<Identifier, Value> + 'static,
 		ValidatorId: Member + Parameter + Ord + MaybeSerializeDeserialize,
-	> Change<Identifier, Value, Settings, Hook, ValidatorId>
+	> NonceWitnessing<Identifier, Value, Slot, Settings, Hook, ValidatorId>


imo, making the names more specific is better in this case

state-chain/pallets/cf-elections/src/electoral_systems/tests/nonce_witnessing.rs

kylezs · 2024-10-01T07:32:28Z

state-chain/pallets/cf-elections/src/vote_storage/nonce.rs

+use crate::{CorruptStorageError, SharedDataHash};
+
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Encode, Decode, TypeInfo)]
+pub struct NonceVote<Value, Slot> {


I think we can generalise it when it necessary? else it's a little confusing it has a general name and is used for one (quite specific) thing. Though, if we go the way of composing the existing vote storage then this conversation is void.

marcellorigotti · 2024-10-02T13:10:38Z

I kept everything generic in the end!

kylezs · 2024-10-08T14:33:36Z

state-chain/pallets/cf-elections/src/vote_storage/change.rs

+		vote: &Self::Vote,
+		mut _h: H,
+	) -> Self::PartialVote {
+		(*vote).clone()


This means we don't have a partial vote, and we always vote the full vote. The plan is to extend this electoral system for the contract witnessing, and so there the value would be much larger in size - and we don't want every validator submitting all that data - we should probably store a hash of the value in the bitmap, and then construct the full vote from the shared data - like we do in the Bitmap VoteStorage impl

- keep everything generalized

dandanlen · 2024-10-09T12:59:15Z

state-chain/pallets/cf-elections/src/electoral_systems/monotonic_change.rs

+					let mut blocks_height = blocks_height.clone();
+					let (_, consensus_block_height, _) = {
+						blocks_height
+							.select_nth_unstable(threshold_from_share_count(num_votes) as usize)


We are computing a threshold here based on the number of votes rather than the number of authorities. Is this intentional? Why?

(
For comparison, for monotonic_median consensus we do:

active_votes.select_nth_unstable((num_authorities - success_threshold) as usize);

)

My idea was to keep only the valid votes into consideration for the slot.
Cause if someone voted for a wrong value their slot could be way off and it doesn't make much sense to keep that into consideration.

Yes, but if we are in this branch of the code, then we know that there are at least success_threshold valid votes. Our thresholds should always be based on the total number of authorities.

Ok I'll switch back to use the authority_count

dandanlen · 2024-10-09T14:04:22Z

engine/src/witness/sol/nonce_witnessing.rs

 pub async fn get_durable_nonce<SolRetryRpcClient>(
 	sol_client: &SolRetryRpcClient,
 	nonce_account: SolAddress,
-) -> Result<Option<SolHash>>
+	previous_slot: SlotNumber,
+) -> Result<Option<MonotonicChangeVote<SolHash, SlotNumber>>>


nit: To keep the idea of a vote within the VoterApi, this fn should not return a vote, it can just return (SolHash, SlotNumber). (and it's the VoterApi's job to convert that into a vote)

dandanlen · 2024-10-09T14:06:49Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs

I don't see a test that checks that we can only form consensus if the block increases. (ie. everyone votes for a new value, but at a lower block).

Actually this is not true, we can form consensus even if the block decrease.
Votes are filtered at the moment of voting only, so if we manually create votes that have a lower block we can still form consensus.

I am implementing is_vote_valid() for the mock so that we can test that these bad votes are correctly rejected!

dandanlen · 2024-10-10T14:01:35Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/monotonic_change.rs

@@ -152,6 +156,37 @@ fn consensus_when_all_votes_the_same_but_different_slot() {
 	);
 }

+#[test]
+fn votes_with_old_value_or_lower_block_are_rejected() {


dandanlen · 2024-10-11T10:30:04Z

As discussed: the upgrade condition check doesn't work, we can delete it.

We need to migrate by deleting all the old elections and then requesting nonce witness elections for any missing nonces.

albert-llimos reviewed Sep 25, 2024

View reviewed changes

engine/src/witness/sol/nonce_witnessing.rs Outdated Show resolved Hide resolved

engine/src/witness/sol/nonce_witnessing.rs Outdated Show resolved Hide resolved

marcellorigotti force-pushed the testNonceElectionChanges branch from c07cc38 to 77e7b82 Compare September 25, 2024 15:52

marcellorigotti marked this pull request as ready for review September 25, 2024 15:52

marcellorigotti requested review from kylezs and dandanlen as code owners September 25, 2024 15:52

dandanlen reviewed Sep 27, 2024

View reviewed changes

kylezs reviewed Oct 1, 2024

View reviewed changes

marcellorigotti force-pushed the testNonceElectionChanges branch from 4f9fa4c to 7b7fdd3 Compare October 2, 2024 13:06

marcellorigotti force-pushed the testNonceElectionChanges branch from 4229a1b to 4c08f3b Compare October 2, 2024 16:57

kylezs reviewed Oct 8, 2024

View reviewed changes

marcellorigotti added 14 commits October 9, 2024 15:36

include slot in the change electoral system

261c508

use new VoteStorage

14c4b06

cargo fmt

7f95876

update tests

2ef2ab3

remove comment

3dd2fe2

prevent upgrade if we are using a nonce

5df1519

add missing import

789e63f

fix check_consensus

dd8a997

address comments:

ba43448

- keep everything generalized

missing imports

bda58ed

fix

1d9b039

fix

abc1097

use SharedDataHash for partialVote

dfdcaf3

inline code

d41341b

dandanlen reviewed Oct 9, 2024

View reviewed changes

marcellorigotti force-pushed the testNonceElectionChanges branch from b86c5ad to d41341b Compare October 9, 2024 13:56

fmt

3ab315d

dandanlen reviewed Oct 9, 2024

View reviewed changes

marcellorigotti added 2 commits October 10, 2024 13:55

addressed comments

f076f47

fix test

5b18b78

dandanlen reviewed Oct 10, 2024

View reviewed changes

dandanlen approved these changes Oct 10, 2024

View reviewed changes

dandanlen added this pull request to the merge queue Oct 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

dandanlen self-requested a review October 11, 2024 10:28

marcellorigotti and others added 2 commits October 14, 2024 16:08

WIP: migrations

7967376

WIP2

a2a8039

feat: Submit a slot number alongside nonce #5297

Are you sure you want to change the base?

feat: Submit a slot number alongside nonce #5297

Conversation

marcellorigotti commented Sep 25, 2024 • edited Loading

Pull Request

Checklist

Summary

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

dandanlen left a 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

dandanlen Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

kylezs Oct 2, 2024 • edited Loading

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

kylezs Oct 1, 2024 • edited Loading

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

dandanlen Oct 2, 2024 • edited Loading

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

kylezs Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

marcellorigotti commented Oct 2, 2024

kylezs Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellorigotti Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandanlen Oct 9, 2024 • edited Loading

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

dandanlen commented Oct 11, 2024

marcellorigotti commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

dandanlen Oct 2, 2024 •

edited

Loading

kylezs Oct 2, 2024 •

edited

Loading

kylezs Oct 1, 2024 •

edited

Loading

dandanlen Oct 2, 2024 •

edited

Loading

kylezs Oct 1, 2024 •

edited

Loading

kylezs Oct 8, 2024 •

edited

Loading

marcellorigotti Oct 9, 2024 •

edited

Loading

dandanlen Oct 9, 2024 •

edited

Loading