Add allow_commission_decrease_at_any_time feature which will allow #33847

bji · 2023-10-24T20:36:03Z

vote account commission to be lowered at any time during the epoch regardless of the commission_updates_only_allowed_in_first_half_of_epoch feature. Fixes #33843.

AshwinSekar · 2023-10-24T21:20:53Z

cc @joncinque the author of the original feature.

codecov · 2023-10-24T21:50:07Z

Codecov Report

Merging #33847 (74a4a36) into master (79739e1) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #33847   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         819      819           
  Lines      220915   220991   +76     
=======================================
+ Hits       180899   180964   +65     
- Misses      40016    40027   +11

CriesofCarrots · 2023-10-25T15:07:50Z

Not to delay progress, but we need to get in the habit of starting with a SIMD for feature-gated changes, like this.
Can you please make some version of #33843 into a SIMD? It would be helpful to suss out what parts of the process are significant impediments to smaller changes.

bji · 2023-10-25T18:02:35Z

Not to delay progress, but we need to get in the habit of starting with a SIMD for feature-gated changes, like this. Can you please make some version of #33843 into a SIMD? It would be helpful to suss out what parts of the process are significant impediments to smaller changes.

Is a SIMD really needed for all features? This is a minor feature that really just "fixes" a shortcoming in an existing feature. Going through the SIMD process sure is painful ...

The only reason a feature gate is even added is because it's consensus breaking. I think of this more as any other "bug fix" change, it just happens to have a feature gate too for that reason.

joncinque · 2023-10-25T20:05:49Z

While I can certainly appreciate the added hurdle of a SIMD, we need to get into the habit of "feature gates require SIMDs" in almost all cases. This should be an easy one to get approved, and I'll be happy to help out!

bji · 2023-10-26T16:38:27Z

SIMD 0079 has been added to propose this feature.

bji · 2023-11-08T17:08:33Z

While I can certainly appreciate the added hurdle of a SIMD, we need to get into the habit of "feature gates require SIMDs" in almost all cases. This should be an easy one to get approved, and I'll be happy to help out!

The SIMD has been updated to respond to your comments, so it would be great to get that SIMD accepted now. Thanks.

AshwinSekar · 2023-11-08T17:52:24Z

sdk/src/feature_set.rs

@@ -724,6 +724,10 @@ pub mod update_hashes_per_tick6 {
    solana_sdk::declare_id!("FKu1qYwLQSiehz644H6Si65U5ZQ2cp9GxsyFUfYcuADv");
 }

+pub mod allow_commission_decrease_at_any_time {
+    solana_sdk::declare_id!("9wWpKE7vL7EmZcDTPm8n9pPVGJLynedAdAxh3BzZnAoF");


@joncinque you want to hold the key for this?

Yeah no problem, I ground out decoMktMcnmiq6t3u7g5BfgcQu91nKZr6RvMYf9z1Jb

joncinque

Looks great for the most part! Most of my comments are small, along with a way to avoid double deserialization in the function now

programs/vote/src/vote_state/mod.rs

joncinque · 2023-11-08T21:38:52Z

programs/vote/src/vote_state/mod.rs

+    let enforce_commission_update_rule =
+        if feature_set.is_active(&feature_set::allow_commission_decrease_at_any_time::id()) {
+            is_commission_increase(vote_account, commission)?
+        } else {
+            true
+        };
+
+    #[allow(clippy::collapsible_if)]
+    if enforce_commission_update_rule
+        && feature_set
+            .is_active(&feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id())
+    {
+        if !is_commission_update_allowed(clock.slot, epoch_schedule) {
+            return Err(VoteError::CommissionUpdateTooLate.into());
+        }
+    }


This instruction isn't used all that often, so it's not a huge performance impact, but we should note that it's deserializing the vote account twice. It's being done once in is_commission_increase, and again in update_commission.

You did the right thing to make the change as small as possible while not introducing potential forking. We could have is_commission_increase take in the VoteState directly and update this function a bit like:

Suggested change

let enforce_commission_update_rule =

if feature_set.is_active(&feature_set::allow_commission_decrease_at_any_time::id()) {

is_commission_increase(vote_account, commission)?

} else {

true

};

#[allow(clippy::collapsible_if)]

if enforce_commission_update_rule

&& feature_set

.is_active(&feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id())

{

if !is_commission_update_allowed(clock.slot, epoch_schedule) {

return Err(VoteError::CommissionUpdateTooLate.into());

}

}

let mut vote_state: VoteState = if feature_set.is_active(&feature_set::allow_commission_decrease_at_any_time::id()) {

let mut vote_state: VoteState = vote_account

.get_state::<VoteStateVersions>()?

.convert_to_current();

if is_commission_increase(&vote_state, commission)

&& feature_set

.is_active(&feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id())

{

if !is_commission_update_allowed(clock.slot, epoch_schedule) {

return Err(VoteError::CommissionUpdateTooLate.into());

}

}

vote_state

} else {

if feature_set

.is_active(&feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id())

{

if !is_commission_update_allowed(clock.slot, epoch_schedule) {

return Err(VoteError::CommissionUpdateTooLate.into());

}

}

vote_account

.get_state::<VoteStateVersions>()?

.convert_to_current()

};

I know it's a bit more code, but it isn't ridiculously ugly. What do you think? cc @AshwinSekar for another opinion

Yeah in the vote program we try to deserialize only once at the beginning of each instruction using either:

vote_account.get_state::<VoteStateVersions>()?.convert_to_current()

or verify_and_get_vote_state

I think you could just let is_commission_increase take a vote state and keep it like how you have it:

let mut vote_state: VoteState = vote_account.get_state::<VoteStateVersions>()?.convert_to_current(); let enforce_commission_update_rule = if feature_set.is_active(&feature_set::allow_commission_decrease_at_any_time::id()) { is_commission_increase(vote_state, commission)? } else { true }; #[allow(clippy::collapsible_if)] if enforce_commission_update_rule && feature_set .is_active(&feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id()) { if !is_commission_update_allowed(clock.slot, epoch_schedule) { return Err(VoteError::CommissionUpdateTooLate.into()); } }

If the vote state deserialization fails though, that could cause forking between old and new versions. old could fail during the comission check, and new could fail during the earlier deserialization, which means that two validators could give different errors for the same instruction.

Does the returned error affect consensus? I always thought that was the case

as far as I know the returned error doesn't impact bank hash, we only check .is_ok()/is_err() in places, but I might be wrong:

solana/accounts-db/src/accounts.rs

Lines 1399 to 1403 in bba6ea2

if execution_status.is_ok() || is_nonce_account || is_fee_payer {

// Add to the accounts to store

accounts.push((&*address, &*account));

transactions.push(Some(tx));

}

I don't mind being safe and making sure we return the same error, doesn't look too ugly.

joncinque · 2023-11-08T21:40:01Z

sdk/src/feature_set.rs

@@ -724,6 +724,10 @@ pub mod update_hashes_per_tick6 {
    solana_sdk::declare_id!("FKu1qYwLQSiehz644H6Si65U5ZQ2cp9GxsyFUfYcuADv");
 }

+pub mod allow_commission_decrease_at_any_time {
+    solana_sdk::declare_id!("9wWpKE7vL7EmZcDTPm8n9pPVGJLynedAdAxh3BzZnAoF");


Yeah no problem, I ground out decoMktMcnmiq6t3u7g5BfgcQu91nKZr6RvMYf9z1Jb

vote account commission to be lowered at any time during the epoch regardless of the commission_updates_only_allowed_in_first_half_of_epoch feature. Fixes solana-labs#33843. SIMD: 0080

bji · 2023-12-08T16:37:11Z

Please complete the review and/or merge the PR. Thanks.

joncinque

Apologies for the lateness, I didn't see any responses so assumed that you were still working on it.

Looks great, thanks for integrating all of our feedback! @AshwinSekar can you also take a look and approve if you have a moment?

joncinque · 2023-12-08T17:25:48Z

I went ahead and fixed the merge conflicts

AshwinSekar

looks fine to me @joncinque just to confirm, you're fine that we might be returning a different error in some scenarios?

joncinque · 2023-12-08T20:14:47Z

looks fine to me @joncinque just to confirm, you're fine that we might be returning a different error in some scenarios?

Unless I'm missing something, I don't think it's possible to return a different error with this implementation.

Also, I checked with the runtime team, and they said the error code doesn't make a difference for consensus, so we're all good from my side.

bji · 2023-12-08T20:24:59Z

looks fine to me @joncinque just to confirm, you're fine that we might be returning a different error in some scenarios?

Why would that be specifically? The way I altered the code to match your feedback should mean that the same errors are returned before and after the change ... unless I'm missing something?

mergify bot added the need:merge-assist label Oct 24, 2023

bji requested a review from AshwinSekar October 24, 2023 20:37

bji force-pushed the allow_commission_decrease_at_any_time branch from 92e8b80 to 166c6ad Compare October 24, 2023 20:45

AshwinSekar requested a review from joncinque October 24, 2023 21:20

bji force-pushed the allow_commission_decrease_at_any_time branch from 166c6ad to 9d30478 Compare October 26, 2023 16:37

CriesofCarrots mentioned this pull request Oct 26, 2023

SIMD-0079: Allow commission decrease at any time during an epoch solana-foundation/solana-improvement-documents#79

Merged

AshwinSekar reviewed Nov 8, 2023

View reviewed changes

joncinque reviewed Nov 8, 2023

View reviewed changes

bji force-pushed the allow_commission_decrease_at_any_time branch from 9d30478 to 16b8c15 Compare November 17, 2023 02:52

Add allow_commission_decrease_at_any_time feature which will allow

b3aebb5

vote account commission to be lowered at any time during the epoch regardless of the commission_updates_only_allowed_in_first_half_of_epoch feature. Fixes solana-labs#33843. SIMD: 0080

bji force-pushed the allow_commission_decrease_at_any_time branch from 16b8c15 to b3aebb5 Compare November 17, 2023 03:00

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 1, 2023

github-actions bot closed this Dec 8, 2023

bji reopened this Dec 8, 2023

joncinque previously approved these changes Dec 8, 2023

View reviewed changes

Merge branch 'master' into allow_commission_decrease_at_any_time

8ee2bbb

joncinque dismissed their stale review via 8ee2bbb December 8, 2023 17:25

Remove unused feature_set import

74a4a36

AshwinSekar approved these changes Dec 8, 2023

View reviewed changes

AshwinSekar merged commit 930ce1e into solana-labs:master Dec 8, 2023
28 checks passed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add allow_commission_decrease_at_any_time feature which will allow #33847

Add allow_commission_decrease_at_any_time feature which will allow #33847

bji commented Oct 24, 2023

AshwinSekar commented Oct 24, 2023

codecov bot commented Oct 24, 2023 •

edited

Loading

CriesofCarrots commented Oct 25, 2023

bji commented Oct 25, 2023 •

edited

Loading

joncinque commented Oct 25, 2023

bji commented Oct 26, 2023 •

edited

Loading

bji commented Nov 8, 2023

AshwinSekar Nov 8, 2023

joncinque Nov 8, 2023

joncinque left a comment

joncinque Nov 8, 2023

AshwinSekar Nov 8, 2023 •

edited

Loading

joncinque Nov 8, 2023

AshwinSekar Nov 8, 2023

joncinque Nov 8, 2023

bji commented Dec 8, 2023

joncinque left a comment

joncinque commented Dec 8, 2023

AshwinSekar left a comment

joncinque commented Dec 8, 2023

bji commented Dec 8, 2023

	if execution_status.is_ok() \|\| is_nonce_account \|\| is_fee_payer {
	// Add to the accounts to store
	accounts.push((&address, &account));
	transactions.push(Some(tx));
	}

Add allow_commission_decrease_at_any_time feature which will allow #33847

Add allow_commission_decrease_at_any_time feature which will allow #33847

Conversation

bji commented Oct 24, 2023

AshwinSekar commented Oct 24, 2023

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

CriesofCarrots commented Oct 25, 2023

bji commented Oct 25, 2023 • edited Loading

joncinque commented Oct 25, 2023

bji commented Oct 26, 2023 • edited Loading

bji commented Nov 8, 2023

AshwinSekar Nov 8, 2023

Choose a reason for hiding this comment

joncinque Nov 8, 2023

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

joncinque Nov 8, 2023

Choose a reason for hiding this comment

AshwinSekar Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

joncinque Nov 8, 2023

Choose a reason for hiding this comment

AshwinSekar Nov 8, 2023

Choose a reason for hiding this comment

joncinque Nov 8, 2023

Choose a reason for hiding this comment

bji commented Dec 8, 2023

joncinque left a comment

Choose a reason for hiding this comment

joncinque commented Dec 8, 2023

AshwinSekar left a comment

Choose a reason for hiding this comment

joncinque commented Dec 8, 2023

bji commented Dec 8, 2023

codecov bot commented Oct 24, 2023 •

edited

Loading

bji commented Oct 25, 2023 •

edited

Loading

bji commented Oct 26, 2023 •

edited

Loading

AshwinSekar Nov 8, 2023 •

edited

Loading