feat: add new vote storage by marcellorigotti · Pull Request #6030 · chainflip-io/chainflip-backend

marcellorigotti · 2025-07-31T15:54:00Z

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

Add new VoteStorage to avoid using SharedDataHash in case the Value voted on is smaller than an Hash (32 bytes),
this makes less likely to delay consensus cause we don't have the full vote for some of the available hashes and vote extrinsics more readable since validators will always vote with the full vote.

coderabbitai · 2025-07-31T15:54:08Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/pro-2400

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

codecov · 2025-07-31T16:59:39Z

Codecov Report

❌ Patch coverage is 42.01681% with 69 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ate-chain/pallets/cf-elections/src/benchmarking.rs	3%	36 Missing ⚠️
...elections/src/migrations/vote_storage_migration.rs	0%	14 Missing ⚠️
...ts/cf-elections/src/electoral_systems/composite.rs	11%	3 Missing and 5 partials ⚠️
.../cf-elections/src/vote_storage/bitmap_numerical.rs	86%	6 Missing ⚠️
...e-chain/runtime/src/migrations/solana_elections.rs	0%	3 Missing ⚠️
state-chain/pallets/cf-elections/src/lib.rs	75%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

marcellorigotti · 2025-08-04T12:14:47Z

Migration is there, what's missing is to make this works with the other migrations regarding the election pallet.
Waiting for others PR to get merged first such that this one can be based on those.

MxmUrw · 2025-09-03T08:23:30Z

state-chain/pallets/cf-elections/src/migrations/vote_storage_migration.rs

+		SharedData::<T, I>::drain();
+		BitmapComponents::<T, I>::drain();
+		IndividualComponents::<T, I>::drain();
+		ElectionConsensusHistory::<T, I>::drain();


Shouldn't we also drain ElectionConsensusHistoryUpToDate? For example, it's cleared in clear_election_votes().

MxmUrw · 2025-09-03T08:35:28Z

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

 	type ElectionState = ();
-	type VoteStorage =
-		vote_storage::individual::Individual<(), vote_storage::individual::shared::Shared<Value>>;
+	type VoteStorage = vote_storage::bitmap_numerical::BitmapNoHash<Value>;


I'm not sure we want to change this vote storage - it's used for solana block height, where I think most of the validators are going to vote for different heights. Or is that not the case?

From observation the span of these votes is ~10 different votes, which means we can do some deduplication here.
Currently we save the SharedDataHash for each validator but since the votes usually span across 10 blocks we end up not having the full vote for some of them which means consensus gets delayed since we need to retrieve the original vote. Using the new VoteStorage means we save a bitmap for each of those ~10 different values but no hash at all, meaning we always have access to the full vote.

TLDR: if we know the span of vote is limited like in this case it makes sense to use this new vote storage to obtain some deduplication without delaying consensus. Furthermore we also save some storage in this case since there is no need to save 150 Hashes.
If the span of vote is potentially 150 different votes, then it makes sense to use the Identity VoteStorage instead for numerical values since it will always be smaller than an hash

Ah right, I see. Cool, then BitmapNoHash does sound like the best option.

MxmUrw · 2025-09-03T08:49:40Z

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

 	type ElectionState = ();
-	type VoteStorage =
-		vote_storage::individual::Individual<(), vote_storage::individual::shared::Shared<Value>>;
+	type VoteStorage = vote_storage::bitmap_numerical::BitmapNoHash<Value>;


Again, I'm not sure we should change this. This is currently only used in bitcoin fee tracking, and since we're going to do sampling we can't guarantee that the validators are going to vote with the same values. Of course, we could for example do some rounding on the voters' side to ensure that the votes fall into "a few buckets". But usually, computing a median implies that we have a bunch of different values, so I would leave this as individual storage.

See answer above, I agree on this one though, if we switch to the new fee witnessing (which means it's very likely we will have 150 different values) we should then use the Identity VoteStorage

we should then use the Identity VoteStorage

Yes, I'm then fine with keeping it currently as you have it, and switch over to identity in my later fee PR.

MxmUrw · 2025-09-03T08:56:06Z

state-chain/pallets/cf-elections/src/migrations.rs

-	}
-}
+pub type PalletMigration<T, I> =
+	(vote_storage_migration::VoteStorageMigration<T, I>, PlaceholderMigration<6, Pallet<T, I>>);


Yeah I think this is correct. Later on we do the migrations::bitcoin_elections::Migration which increases the storage version to 7.

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

state-chain/pallets/cf-elections/src/benchmarking.rs

MxmUrw · 2025-09-03T09:27:57Z

state-chain/pallets/cf-elections/src/benchmarking.rs

+		#[extrinsic_call]
+		provide_shared_data(RawOrigin::Signed(validator_id), Box::new(shared_data_value.clone()));
+
+		assert!(SharedData::<T, I>::get(shared_data_hash).is_some());


I think we could also check that the shared data we get is the one we provided, as we did previously, no?

MxmUrw · 2025-09-03T09:28:41Z

state-chain/pallets/cf-elections/src/benchmarking.rs

+		SharedDataReferenceCount::<T, I>::insert(
+			shared_data_hash,
+			UniqueMonotonicIdentifier::from(0),
+			ReferenceDetails::<BlockNumberFor<T>> {
+				count: 1u32,
+				created: BlockNumberFor::<T>::from(1u32),
+				expires: BlockNumberFor::<T>::from(10u32),
+			},
 		);


Just for clarification, why did calling the vote() function break?

It doesn't, vote() works fine here but since we don't use SharedData nothing get's inserted in the SharedDataReferenceCount which then causes provide_shared_data to return UnreferencedSharedData

I'll also add a comment here to explain

I'll also add a comment here to explain

Perfect, a comment like what you wrote above would be already very helpful.

MxmUrw · 2025-09-03T09:30:07Z

state-chain/pallets/cf-elections/src/benchmarking.rs

 		// Setup a validator set of 150 as in the case of Mainnet.
 		let election_identifier =
-			setup_validators_and_vote::<T, I>(150, BenchmarkValue::benchmark_value());
+			setup_validators_and_vote::<T, I>(3, BenchmarkValue::benchmark_value());


The comment should also say 3. And I think it would be useful to explain to future readers why using 150 doesn't work.

I'll update the comment.
Same as the epoch index, I didn't investigate why 150 doesn't work, but I tested it and the authority set size get set to 3 max.

Would be good to know and understand I think. What error does it fail with?

What happens is that the epoch authority size ends up being 3.
When a validator with authority index >= 3 votes the index is out of bound when accessing the bitmap causing CorruptedStorage, thinking about this the check ensure_can_vote in vote() should prevent this to happen though! I'll investigate this a bit further.

the index is out of bound when accessing the bitmap causing CorruptedStorage

And previously we only didn't notice this because we ran the benchmarks with a different votestorage, right?

MxmUrw · 2025-09-03T09:30:36Z

state-chain/pallets/cf-elections/src/benchmarking.rs

 		// Setup a validator set of 150 and reach consensus
 		let election_identifier =
-			setup_validators_and_vote::<T, I>(150, BenchmarkValue::benchmark_value());
+			setup_validators_and_vote::<T, I>(3, BenchmarkValue::benchmark_value());


Same as above.

MxmUrw

LGTM!

MxmUrw · 2025-09-04T11:38:03Z

state-chain/pallets/cf-elections/src/benchmarking.rs

+		#[block]
+		{}


This looks strange, what is this?

we need either a #[block] or #[extrinsic] in the benchmarks.
Since we don't really care about measuring the weights here but it is more a test we just run an empty block just to make it compile. This could be moved into the tests but then we would have to mock another ES since the MockESRunner is a composite of 1 ES only.

I see, I wasn't aware of that 👍

MxmUrw · 2025-09-04T11:41:37Z

state-chain/pallets/cf-elections/src/benchmarking.rs

 	}

+	#[benchmark]
+	fn test_mismatch<T: crate::pallet::Config<I>, I: 'static>() {


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

MxmUrw · 2025-09-04T11:44:20Z

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

+	) -> Result<(), sp_runtime::DispatchError> {
+		Ok(())
+	}


Does this need a default implementation?

🤔 this is the mock impl

…te.rs Co-authored-by: Maxim Urschumzew <MxmUrw@users.noreply.github.com>

* add new vote storage * add migration * fix migrations * fix benchmarks * address comments * last fix for benchmarks * add check to prevent vote mismatch * remove from test * Update state-chain/pallets/cf-elections/src/electoral_systems/composite.rs Co-authored-by: Maxim Urschumzew <MxmUrw@users.noreply.github.com> --------- Co-authored-by: Maxim Urschumzew <MxmUrw@users.noreply.github.com>

marcellorigotti added 3 commits August 26, 2025 14:28

add new vote storage

ebcfba8

add migration

47f4780

fix migrations

60d48c3

marcellorigotti force-pushed the feature/pro-2400 branch from a0e44e2 to 60d48c3 Compare August 26, 2025 12:28

marcellorigotti marked this pull request as ready for review August 26, 2025 13:06

marcellorigotti requested review from MxmUrw, dandanlen and nmammeri as code owners August 26, 2025 13:06

marcellorigotti removed the request for review from nmammeri August 26, 2025 13:06

marcellorigotti requested a review from albert-llimos as a code owner September 2, 2025 09:44

marcellorigotti removed the request for review from albert-llimos September 2, 2025 09:45

fix benchmarks

54631ea

marcellorigotti force-pushed the feature/pro-2400 branch from 1605655 to 54631ea Compare September 2, 2025 13:57

MxmUrw reviewed Sep 3, 2025

View reviewed changes

MxmUrw self-requested a review September 3, 2025 09:31

address comments

c03e478

MxmUrw approved these changes Sep 3, 2025

View reviewed changes

marcellorigotti added 3 commits September 4, 2025 10:38

last fix for benchmarks

0334a1b

add check to prevent vote mismatch

cde4811

remove from test

d4dcde7

MxmUrw reviewed Sep 4, 2025

View reviewed changes

Update state-chain/pallets/cf-elections/src/electoral_systems/composi…

d82c2d2

…te.rs Co-authored-by: Maxim Urschumzew <MxmUrw@users.noreply.github.com>

marcellorigotti added this pull request to the merge queue Sep 4, 2025

Merged via the queue into main with commit 137a90d Sep 4, 2025
54 checks passed

marcellorigotti deleted the feature/pro-2400 branch September 4, 2025 20:06

Conversation

marcellorigotti commented Jul 31, 2025

Pull Request

Checklist

Summary

Uh oh!

coderabbitai bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Chat

Support

CodeRabbit Commands (Invoked using PR/Issue comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Status, Documentation and Community

Uh oh!

codecov bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marcellorigotti commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MxmUrw Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MxmUrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Jul 31, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov bot commented Jul 31, 2025 •

edited

Loading

MxmUrw Sep 3, 2025 •

edited

Loading