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

SIMD-0079: Allow commission decrease at any time during an epoch #79

Merged

Conversation

bji
Copy link
Contributor

@bji bji commented Oct 26, 2023

No description provided.

@bji bji force-pushed the allow_commission_decrease_at_any_time branch 3 times, most recently from fc1f344 to df088b1 Compare October 26, 2023 16:31
@bji bji changed the title Added allow_commission_decrease_at_any_time proposal. SIMD-0080: Allow commission decrease at any time during an epoch Oct 26, 2023
@bji bji force-pushed the allow_commission_decrease_at_any_time branch 3 times, most recently from a59014d to ed409b8 Compare October 26, 2023 16:34
@jacobcreech
Copy link
Contributor

Hey @bji, was this discussed at all in the Solana Tech discord? I don't see any discussion or coming to consensus that this change should be made into a SIMD.

@CriesofCarrots
Copy link
Contributor

@jacobcreech , the discussion happened on github: solana-labs/solana#33847 (comment)

@CriesofCarrots
Copy link
Contributor

@jacobcreech , should this be SIMD 79 because it's github issue number 79? Is the numbering scheme articulated somewhere? Sorry if I missed it.

@jacobcreech
Copy link
Contributor

@jacobcreech , should this be SIMD 79 because it's github issue number 79? Is the numbering scheme articulated somewhere? Sorry if I missed it.

Yes, it should be 79 according to https://github.com/solana-foundation/solana-improvement-documents/blob/main/proposals/0001-simd-process.md#draft

@bji
Copy link
Contributor Author

bji commented Oct 26, 2023

I don't know how I got 80. I swear I read issue #80 somewhere, but I guess I made a mistake. My apologies. I will rename.

EDIT: Yes there it is. If you click the #80 link you see that github describes the issue as "#80". Not sure why if it's actually issue 79. Oh well.

Ah I see. The pull request is 79 but the issue was 80. I thought that the SIMDs were supposed to be numbered by an issue number for an issue created to describe the SIMD, not numbered by a particular PR number for the SIMD.

Here's where I got confused (from 0001-simd-process.md):

"- Now that your proposal has an open pull request, use the issue number of the
PR to update the XXXX- prefix to the number."

I read "issue number" and thought an issue was supposed to be created as a means of numbering proposals, so I created an issue, got the number 80 from it, and used that for my SIMD number. My mistake - never needed to create an issue, just needed to create a PR and then rename the SIMD using the PR number (didn't realize PR numbers were called "issue numbers of the PR").

@bji bji force-pushed the allow_commission_decrease_at_any_time branch from ed409b8 to 0c3aa1a Compare October 26, 2023 18:26
@bji bji changed the title SIMD-0080: Allow commission decrease at any time during an epoch SIMD-0079: Allow commission decrease at any time during an epoch Oct 26, 2023
@bji bji force-pushed the allow_commission_decrease_at_any_time branch from 0c3aa1a to 51f37a8 Compare October 26, 2023 18:29
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me overall! Mostly small comments

@bji bji force-pushed the allow_commission_decrease_at_any_time branch from 51f37a8 to 7252757 Compare November 7, 2023 22:30
Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@joncinque
Copy link
Contributor

We just need someone from Firedancer to sign off too. I posted in the Discord firedancer channel about it.

Comment on lines +67 to +60
Validators will now be able to decrease commission at any time in the epoch,
but only increase commission in the first half of epochs (because of the
commission_updates_only_allowed_in_first_half_of_epoch feature already
implemented).
Copy link
Contributor

@ripatel-fd ripatel-fd Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just stating a technical fact.

But the impact section should describe how it affects the ecosystem. (Does it fix previously broken behavior, does it unblock a project, does it benefit delegators, etc...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is splitting hairs on the purpose of SIMD sections. I'm going to leave this as-is.

Copy link
Contributor

@ripatel-fd ripatel-fd Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the purpose of sections is documented for a reason in the template. Clearly stating the impact on each SIMD helps with prioritizing work on these features (which always involve a lot of testing, btw). Unless I'm mistaken, this seems like "nice to have" level of importance to the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

## Impact

How will the implemented proposal impacts dapp developers, validators, and core contributors?

The text included states how the change will impact validators. There is no impact to the other categories outside of the truism that "any change could affect anyone".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how many validators have the policy mentioned in the motivation section, or how common this problem is? Something like that would be nice to document as impact, because I agree with Richard that unless we understand that this change does seem "nice to have"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is one good reason not enough?

Copy link
Contributor

@ripatel-fd ripatel-fd Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is one good reason not enough?

@bji Yes -- Long story short, we should to scrutinize feature activations more, especially by triaging them by usefulness. The impact section would be the place to make an argument why the feature is important.

The text included states how the change will impact validators.

If most validators didn't notice activation of this feature, is it really impactful? But your point is valid, the template should be clearer about the purpose of this section.

Considering this doesn't improve performance or reliability and it's unclear whether anyone else will use this feature, I would say it's barely enough justification. The risk of any feature flag is that it halts the network if the implementation is botched. Even though this change looks small, it's still a lot of work to write up all the test cases and test compatibility between Labs / Firedancer. The reward for this one is ... somewhat unknown.

This was one of the discussions at Block Zero, which I think you were also participating in: The feature activation backlog keeps growing because core contributors can only implement so many features per month.

IMHO The best way to address this problem is to focus on the most impactful features first.

To be clear, I'm still fine with merging this as-is. This is just a suggested change, not a request. Also, there's no need to keep resolving this conversation when it's still active.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original feature which prevented changes to the commission in the second half was an over-reach. It had a goal ("prevent rug pulls") and it created collateral damage by implementing a more restrictive than necessary policy.

It doesn't seem like the best way to have technical discussions to have to provide justification for whether or not the reasons are "good enough". What you think is important, someone else might not. It's necessary to include importance in prioritization of effort, but when discussing technical merits of a proposal, I think justification just should be a minor factor, and having "one good reason" should be good enough.

In this case, I've already given a justification of why this feature is important to me.

I can imagine other scenarios where it might matter. Will these sway anyone? Or is a back and forth discussion about whether "enough reasons have been imagined yet" going to be a part of the process?

  • Someone may accidentally set a high commission in the first half of epoch, and not realize it until the second half. It would be better if they could correct their mistake than if they were prevented from correcting it.

  • Someone may set a high commission in the first half of the epoch but change their mind and want to change it back down in the second half. It would be better if they could make their change rather than being restricted without reason from doing so.

  • Someone may receive a command from their government or other authority notifying them that if they accept commission, they are breaking the law and will go to jail. If this occurs in the second half of an epoch, and they can't set commission to 0, they will have no way to prevent going to jail since their validator already earned vote credits and cannot be prevented otherwise from earning commission (YES this is super far-out, but since the threshold for someone feeling this feature is justified is arbitrary, I have no idea how far out to go here).

  • Someone may run a sophisticated commission selection scheme that sets a 100% commission in the first half of the epoch, and then has their stakers vote on reducing it incrementally until they reach a compromise (there would be some other, not disclosed here, reason for stakers to vote for 'some commission' above 0 instead of just 0). If commission cannot be set in the second half of epochs, this mechanism may not work properly (YES this is super far out; see the previous paragraph for why)

...

@bji bji force-pushed the allow_commission_decrease_at_any_time branch from 7252757 to 5bdfab7 Compare November 9, 2023 17:37
@ripatel-fd
Copy link
Contributor

ripatel-fd commented Nov 10, 2023

I asked my colleagues to review and approve once I have a second opinion from my team. (It looks good to me). Thanks for submitting this.

Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Green light from Firedancer side, but considering this low priority.

@ripatel-fd ripatel-fd merged commit 1c7f202 into solana-foundation:main Nov 10, 2023
2 checks passed
@bji
Copy link
Contributor Author

bji commented Nov 10, 2023

Green light from Firedancer side, but considering this low priority.

Thank you; the implementation has already been provided to Solana Labs as a PR so from their perspective it should be an easy thing to take. Once it's reached consensus within the cluster Firedancer would have to implement it; but I think it's a very easy implementation.

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

Successfully merging this pull request may close these issues.

7 participants