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-0180: Use Vote Account Address To Key Leader Schedule #180

Merged

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Oct 3, 2024

No description provided.

@jstarry jstarry changed the title SIMD-XXXX: Leader Schedule Migration SIMD-0180: Leader Schedule Migration Oct 3, 2024
@jstarry jstarry force-pushed the leader-schedule-migration branch from b14c2bf to deccb77 Compare October 3, 2024 23:15
up by first finding the vote account for the designated vote pubkey for a
particular leader slot in bank epoch stakes. Bank epoch stakes are keyed by
leader schedule epoch and therefore the vote account state should be retrieved
by looking up the epoch stakes for the current epoch. Since only valid vote
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirming my understanding that this shouldn't cause an issue at epoch boundary:
Since the leader schedule is generated an epoch before, if we receive a shred from epoch E + 1, we can still use the root bank from epoch E to perform the vote account -> node pubkey lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. We can only validate shreds for epoch E + 1 once we have a root bank in epoch E

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

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

i find the repeated "migration" reminders and back references to current implementation break the flow of the text and make it difficult to follow

that we're migrating is implicit from the nature of the proposed changes

there are only a few places where i find the existing implementation references helpful

  • retaining the random draw algo
  • shred signer
  • collector address
  • rpc suggestions

throughout are instances of our trademark ambiguous use of "pubkey." i think most here should be "account address" with exception of the places where we are talking about cryptographic signing/verification.

there are also several usages of "should" throughout that suggest optionality of details that are in fact requirements. most should(lol) be "must" as per rfc2119

assuming i read everything correctly, the proposed protocol changes themselves are correct and well motivated

@@ -0,0 +1,91 @@
---
simd: '0180'
title: Leader Schedule Migration
Copy link
Contributor

Choose a reason for hiding this comment

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

title isn't particularly explanatory

Suggested change
title: Leader Schedule Migration
title: Use Vote Account Address To Key Leader Schedule

vote pubkey. Then use the existing stake weighted randomized leader schedule
generation using vote pubkeys and their delegated stake rather than node id
pubkeys and the accumulated delegated stake across (potentially more than one)
vote accounts. As before, only valid and initialized vote accounts should be
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 include any of the old vote state versions as "valid"? are validity criteria documented somewhere that we can link out to?

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 added a section for this.. we don't explicitly check vote state version for the initialization check. We just check the size of the account and whether there are some non-zero bytes. Less than ideal for sure but out of scope for this SIMD.

@jstarry jstarry changed the title SIMD-0180: Leader Schedule Migration SIMD-0180: Use Vote Account Address To Key Leader Schedule Feb 5, 2025
@jstarry jstarry force-pushed the leader-schedule-migration branch from deccb77 to 43508cd Compare February 5, 2025 07:11
@jstarry
Copy link
Contributor Author

jstarry commented Feb 5, 2025

@t-nelson thanks for the feedback, addressed your comments I think. Can you take another look?

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

lgtm now thanks

@Benhawkins18 Benhawkins18 merged commit 4271f2e into solana-foundation:main Feb 27, 2025
2 checks passed
@MUSICNFTACCESS
Copy link

I feel this would be a benefit I vote yes

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

Successfully merging this pull request may close these issues.

6 participants