-
Notifications
You must be signed in to change notification settings - Fork 225
SIMD-0329: Track Last Accessed Slot for Accounts #329
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
base: main
Are you sure you want to change the base?
Conversation
f497894 to
c27d3b0
Compare
c27d3b0 to
e06bfba
Compare
9ed6698 to
14acb30
Compare
313ccef to
74f73f6
Compare
proposals/0329-last-accessed-slot.md
Outdated
| The additional 8 bytes per account may increase snapshot size if insufficient | ||
| unused account metadata bytes are available. Currently, there are enough unused | ||
| bytes (due to padding and deprecated fields) so this specific change shouldn't | ||
| increase state size. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets try to use the existing unused rent field on accounts so we don't have to resize accounts with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, probably better to be more explicit here
proposals/0329-last-accessed-slot.md
Outdated
| 6. **Feature Gate Activation**: This change is guarded by a feature gate. Upon | ||
| activation in a block at slot S, all existing accounts MUST have | ||
| `last_accessed_slot` initialized to S when loaded (e.g., from snapshot or | ||
| ledger). New accounts created after activation MUST initialize the field to | ||
| the current slot at creation time. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can intitialize this when the account is write locked after the feature gate activates to amoritize the cost over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the wording here, it's unclear. The intention was to have pre-activation accounts lazily initialized to S when the value is first accessed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though, I'd say this is mostly an implementation detail. Clients can choose how/when to update pre-activation accounts, the important detail is that the value should default to S. I'll clarify regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing the default value might need to be done over a whole epoch, similar to how rent collection used to be done.
And if the rent_epoch bytes are reused for this new field, then we can be in a situation where nodes disagree about the value for that field if we don't enforce an order of when accounts will be updated.
It's not clear from the current set of SIMDs, but will rent_epoch be completely deprecated / removed from consensus before this SIMD? If so, then we should be fine. If not, we'll probably need to do that deprecation with a separate SIMD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joncinque Correct me if I'm wrong, but isn't rent_epoch already removed from consensus? It's no longer in the accounts hash and it's hardcoded in the vm. cc @brooksprumo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing the default value might need to be done over a whole epoch, similar to how rent collection used to be done.
Discussed this offline: I assumed there already existing a migration path for account structure changes but that turned out to be wrong, so this will be more complicated than I expected. There should be a forthcoming SIMD for adding versioning to accounts, which will make this migration much more straightforward.
It's not clear from the current set of SIMDs, but will rent_epoch be completely deprecated / removed from consensus before this SIMD? If so, then we should be fine. If not, we'll probably need to do that deprecation with a separate SIMD.
I'm confused on this as well because there hasn't been an explicit deprecation SIMD, but it appears that for all intents and purposes rent_epoch has already been deprecated (stubbed out in the runtime and excluded from the lthash).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok great, I truly wasn't aware, but it sounds like rent_epoch has been totally deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, rent_epoch is practically gone. The field exists in some agave/sdk structs, but the value itself is not under consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Mostly small comments.
proposals/0329-last-accessed-slot.md
Outdated
| 2. **Update on Write-Lock**: Each time a transaction that write-locks an | ||
| account is included in a block, that account's `last_accessed_slot` MUST be | ||
| set to the block's slot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine this happening at three possible times:
- before the transaction is processed
- after the transaction is processed
- at the end of the block
I imagine the best option is to write the value after the transaction is processed.
Also, does this change if the transaction was successful or not? It seems like we'll need to write the data even on failures, which is a bit different from normal transaction processing, where only lamports are deducted from the fee payer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine this happening at three possible times:
This should be specified - agree that after the transaction is processed seems like the most natural place to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does this change if the transaction was successful or not? It seems like we'll need to write the data even on failures, which is a bit different from normal transaction processing, where only lamports are deducted from the fee payer.
This should definitely also be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update the doc to specify this should be updated like most other account state: after successful execution. The reason to only update on successful execution is to allow more flexibility in the future rent payment system: e.g programs can determine how rent payments work rather than requiring the fee payer to always cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh given this, it probably makes more sense to just rename this field to last_write_slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should update this field if-and-only-if the account is going to be stored. Aka when committing transactions.
proposals/0329-last-accessed-slot.md
Outdated
| 9. **On-chain ABI exposure**: `last_accessed_slot` is runtime metadata only and | ||
| is NOT added to the on-chain `AccountInfo` ABI. It is not exposed to | ||
| programs, and no new syscalls are introduced by this SIMD. Any future | ||
| on-chain exposure (e.g., a syscall or ABI extension) will be proposed via a | ||
| separate SIMD and feature gate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! We could reuse the rent epoch that exists on the account info in ABIv1, but a syscall in ABIv2 (TBD) will likely be easier to add
proposals/0329-last-accessed-slot.md
Outdated
| 5. **RPC and Client Exposure**: `last_accessed_slot` SHOULD be available | ||
| through relevant RPC endpoints that return account information, allowing | ||
| clients to access this metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, RPC will default rentEpoch to u64::MAX and add a new field for lastAccessedSlot when fetching or returning accounts.
For simulateTransaction, the lastAccessedSlot should probably be updated to the current slot.
proposals/0329-last-accessed-slot.md
Outdated
| 6. **Feature Gate Activation**: This change is guarded by a feature gate. Upon | ||
| activation in a block at slot S, all existing accounts MUST have | ||
| `last_accessed_slot` initialized to S when loaded (e.g., from snapshot or | ||
| ledger). New accounts created after activation MUST initialize the field to | ||
| the current slot at creation time. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing the default value might need to be done over a whole epoch, similar to how rent collection used to be done.
And if the rent_epoch bytes are reused for this new field, then we can be in a situation where nodes disagree about the value for that field if we don't enforce an order of when accounts will be updated.
It's not clear from the current set of SIMDs, but will rent_epoch be completely deprecated / removed from consensus before this SIMD? If so, then we should be fine. If not, we'll probably need to do that deprecation with a separate SIMD.
proposals/0329-last-accessed-slot.md
Outdated
| lthash.append( account.owner ) | ||
| lthash.append( account.pubkey ) | ||
| lthash.append( account.last_accessed_slot ) | ||
| return lthash.fini() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're changing the lthash anyways, let's also move account.data to the end of the account IMO (and introduce a hashing prefix)
Having account.data in the middle of the hash input was always broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. When/if 379 is accepted, this change should be included for v1 accounts.
proposals/0329-last-accessed-slot.md
Outdated
| values. | ||
|
|
||
| 8. **Accounts lattice hash update**: the lattice hash of an account must | ||
| include the `last_accessed_slot`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to specify a migration mechanism here. Implementations will probably need to track which accounts have the last_accessed_slot field depending on whether lthash set removal is done with the old hash (safe) or new hash (probably also safe but more cryptographic concerns here). This sort of tracking has overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's definitely more complexity in the migration than I originally thought due to the lack of account versioning. I believe one of the accountsdb people is working on a proposal to add a version field to the metadata that should make updates more straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, changing the per-account hash is non-trivial now that we have the accounts lt hash. See #319 (comment) for some more discussion.
|
Not opposed to the idea of adding metadata to accounts. But I'd wait until merging this proposal until we have another SIMD defining a use case for this new field. |
The rent mechanism described in SIMD-0344 (dynamic rent) will use the new field. |
proposals/0329-last-accessed-slot.md
Outdated
| - Track `last_accessed_slot` for each account as part of the account's metadata. | ||
| - Each time a transaction that write-locks an account is included in a block, | ||
| update that account's `last_accessed_slot` to the block's slot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename accessed everywhere with either modified or updated?
IMO the word "accessed" also includes, read-only accesses, which we explicitly do not want to trigger an account update.
Additionally, can we rename "last" to either "most recent" or "latest"? IMO the word "last" can be ambiguous, whereas "most recent" and "latest" both resolve the ambiguity by having the notion of time ordering in their definitions.
proposals/0329-last-accessed-slot.md
Outdated
| 4. **Initialization for Existing Accounts**: For accounts that exist before | ||
| activation of this feature, `last_accessed_slot` MUST be initialized to the | ||
| slot of the block that activates the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why using feature activation slot is required here, versus something like slot 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that in the absence of exact write history, we default to a value that is guaranteed to be as or more recent than the true value. ie latest_write_slot >= actual_latest_write_slot.
That said, defaulting to 0 might be easier for the implementation. If higher level mechanisms (like rent) want to use the value, they can interpret 0 in whichever way makes sense in their context. What do you think?
proposals/0329-last-accessed-slot.md
Outdated
| The additional 8 bytes per account may increase snapshot size if insufficient | ||
| unused account metadata bytes are available. Currently, there are enough unused | ||
| bytes (due to padding and deprecated fields) so this specific change shouldn't | ||
| increase state size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO increasing the size of an account is a non-starter at the moment. We must repurpose the existing-and-obsolete rent_epoch field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, this is from an older version of the proposal. I've updated it to specifically replace the deprecated rent_epoch field.
proposals/0329-last-accessed-slot.md
Outdated
| - Each time a transaction that write-locks an account is included in a block, | ||
| update that account's `last_accessed_slot` to the block's slot. | ||
|
|
||
| ## Motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation here in this section is quite sparse. Changing account format is an extremely invasive change, and should only be done when absolutely necessary. IMO the current motivation doesn't sufficiently indicate necessity, and so I would push back against this SIMD in its current form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I should explicitly mention that this field is required for the dynamic rent mechanism described in SIMD-0344. I'll add that.
|
Note: due to the lack of an account migration path, this SIMD depends on this proposal, which introduces a version field to accounts. |
No description provided.