SIMD-0106: Epoch Rewards Partition Data PDA

[CriesofCarrots]

Partitioned epoch rewards (SIMD 0015) will split epoch rewards distribution across multiple blocks at the start of an epoch. This extension of that SIMD proposes storing data about the rewards calculation and partitions in a sysvar account at a program-derived address (PDA), which can be queried by clients, or indeed within the runtime, to performantly identify the partition for the distribution to any particular address.

[t-nelson]

just a couple nits from me. lgtm otherwise!

[t-nelson]

lgtm now. @ripatel-fd wanna take a look?

[ripatel-fd]

I support this change. I added some comments regarding technical details.
cc @jumpsiegel @lheeger-jump can you also take a look?

[t-nelson]

can we add something like "metadata" or "parameters" to the title?

[CriesofCarrots]

can we add something like "metadata" or "parameters" to the title?

I used "Partition Data" to be consistent across PRs, but open to something else if you prefer.

[t-nelson]

heh, i know we use it a lot, but i've never found "data" to be a particularly strong adjective 😅

[jstarry]

The SIMD should explain how this is done since it's also important for consensus. Are they read-only only after they are created?

[CriesofCarrots]

As you stated on your SIMD, I think how dynamically addressed sysvars would have their write-locks demoted is outside the scope of this proposal. But I expanded the section a little.

[jstarry]

This SIMD needs to address the specifics of write-lock demotion if you state that these sysvars should be loaded as read-only. Right now we only have a way to use feature gates to introduce new read-only sysvars.. we don't have a design for this new kind of sysvar which is created each epoch yet. So I think this SIMD can reference SIMD 105 and say that the new sysvars will be added to the dynamic list when they are created on-chain. Also, I don't think that the "Security considerations" is the right place to discuss read-only loading.. it should be in the design section.

[CriesofCarrots]

new sysvars will be added to the dynamic list when they are created on-chain

Are you thinking just insert in the Bank::reserved_account_keys HashSet? It doesn't look like that field gets persisted in snapshots in your current implementation, but that seems like it would be necessary for these sysvars.
That probably deserves some thought; I'm still inclined to address this kind of account in a separate proposal, like I thought we agreed to on your SIMD 0105.

[jstarry]

I'm fine with handling in a separate SIMD but this SIMD says they should only be loadable as read-only but doesn't explain that the design is still TBD. So I think we should say that it's TBD or design it here.

[jstarry]

As for lack of persistence, we can add all these sysvars in a loop from some starting epoch to the current epoch. This seems fine to do in each restart because epochs will only be in the hundreds to thousands.

[CriesofCarrots]

I feel like the use of "should" here is consistent with rfc2119, and saying "a new proposal should be introduced" is the same thing as saying the design is TBD. But I'm also happy to remove the paragraph entirely.

[jstarry]

Oh sorry I didn't realize "should" meant that. I interpreted it as "must" since this would impact consensus. I think we should omit the paragraph or add the implementation details here with "must" details.

[jstarry]

Can you also include the motivation for including this data on-chain? I personally think that storing it in the first blocks metadata would be great. We already use the metadata to record the list of rewards.

[CriesofCarrots]

Added. But yes, planning to do both. Will write up a separate SIMD for the metadata approach.

[CriesofCarrots]

I reverted this one as well. Here's what I had written:
"If the partition data were stored in an on-chain account, it would be simple and
low-compute-cost for a client (with a snapshot or running node) or the runtime
itself to find the rewards for a particular address for any historical epoch
after partitioned epoch rewards are activated."

I do feel like this is already articulated in the summary.

[jstarry]

Ok, how open are you to removing the PDA approach and just using the metadata? I know it's already implemented so we're at least slightly committed to that approach, so I won't push back too hard. But I generally prefer we limit the number of ways accounts can be written outside of the SVM because it opens up security issues and increases the amount of code that other protocol implementations need to write.

Getting specific reward data for an account is already impossible afaik from the runtime so I don't see much value in making the partition data accessible in an account. Since reward data is only in metadata, I think it makes sense for the partition data to also only be in metadata.

[CriesofCarrots]

Ok, how open are you to removing the PDA approach and just using the metadata?

Somewhat opposed. I think it makes sense to have some sort of persistent record of the partitioned-rewards data on chain, but it's true I don't know of a specific in-program use-case for it. From the rpc perspective, it's also really nice to be able to figure out the rewards block before having to determine what data store to use to find that block.
@t-nelson , perhaps you'd care to chime in here?

[jstarry]

Is that very different from the rpc fetching the first block in the epoch to find out where the rewards are? If I recall correctly, there's an extra call getBlocks to figure out what the first block actually is, is that the part you're trying to improve?

[CriesofCarrots]

there's an extra call getBlocks to figure out what the first block actually is, is that the part you're trying to improve?

Yes. There's potentially quite a bit of latency in each long-term storage query, so it would be nice to avoid the extra call.

[jstarry]

If the main motivation is to save some latency overhead on step 1 of fetching rewards when step 2 requires high latency big table fetching most of the time anyways, it doesn't seem like this proposal is worth the runtime complexity.

[t-nelson]

i think i'm mostly fine with the following changes taken into consideration

---

i find that the more recently introduced commentary actually detracts from the proposal by making, especially the design section, less concise. if an alternate consideration doesn't reach the level to justify its inclusion in the Alternatives Considered section, imo it can be left out. if we need some rationale for a decision, that can be inlined as a clause nearer the introduction of that concept.

[CriesofCarrots]

Closing this. Ultimately, @jstarry was convincing that we don't currently need this data in account state. Will propose and implement the alternative described in this SIMD to allow RPC to support partitioned rewards for now. If an on-chain use-case for the rewards partition data arises in the future, we can resuscitate this PR then.