Skip to content

SIMD-0242: Static Nonce Account Only#242

Merged
Benhawkins18 merged 5 commits into
solana-foundation:mainfrom
apfitzge:alt_noncery
Mar 28, 2025
Merged

SIMD-0242: Static Nonce Account Only#242
Benhawkins18 merged 5 commits into
solana-foundation:mainfrom
apfitzge:alt_noncery

Conversation

@apfitzge
Copy link
Copy Markdown
Contributor

@apfitzge apfitzge commented Feb 3, 2025

No description provided.

@apfitzge apfitzge changed the title Static Nonce Account Only SIMD-0242: Static Nonce Account Only Feb 3, 2025
@apfitzge
Copy link
Copy Markdown
Contributor Author

apfitzge commented Feb 3, 2025

This is a necessary precursor for #192

@apfitzge apfitzge marked this pull request as ready for review February 3, 2025 20:10
@apfitzge apfitzge requested a review from jstarry February 3, 2025 20:11
t-nelson
t-nelson previously approved these changes Feb 3, 2025
Copy link
Copy Markdown
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.

pretty sure supporting this in the first place was an unintentional oversight

@apfitzge
Copy link
Copy Markdown
Contributor Author

apfitzge commented Feb 3, 2025

pretty sure supporting this in the first place was an unintentional oversight

Yeah. Think just a natural consequence of adding the two features independently, there was no special consideration for how the two should interact.

Comment thread proposals/0242-static-nonce-account.md Outdated
Comment on lines +46 to +47
- Nonce transactions MUST have a nonce account index that is less than or equal
to the number of statically included accounts in the transaction.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Must be strictly less than the number of static accounts if we are talking about an index.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This still needs to be updated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comment was marked as spam.

Comment thread proposals/0242-static-nonce-account.md Outdated
Comment on lines +25 to +26
- However, that relaxation cannot happen if the nonce account is advanced in a
looked up account.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you explain further why the relaxation cannot happen in this case? It's not too obvious I think. My understanding is that if we attempt to process a durable nonce transaction that uses an invalid lookup, we wouldn't be able to include the transaction because the nonce is not advanceable and therefore there wouldn't be a way to ensure that it would only be processable once. This is a blocker to implementing the other proposal which aims to allow including any transaction which has an invalid lookup into a block.

Copy link
Copy Markdown
Contributor

@2501babe 2501babe Feb 4, 2025

Choose a reason for hiding this comment

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

it also sort of resolves a catch-22: if all bad lookups get processed, and all bad nonces get discarded, and lookup happens first, does that mean bad nonces get processed? but to process it you need a nonce

a failed nonce lookup should be the same as a nonexistent nonce, for any sane definition of nonce validation. and we still need to validate nonces (and blockhashes for blockhash transactions) to process an alt lookup failure even for a lookup failure for a transation with a static nonce. processing requires a new nonce state to be written, so the only possible way we could "process" a failure to resolve a nonce from an alt would be to pretend its a blockhash transaction and skip blockhash validation for it

a simpler solution would just be to clarify in simd192 that failed nonce lookup == failed nonce validation. but im inclined to support simd242 because im in favor of restricting nonces in this way (and several others)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think it makes sense to elaborate the motivation so these details are explicit


- Nonce transactions MUST have a nonce account index that is less than or equal
to the number of statically included accounts in the transaction.
- Leader nodes MUST drop nonce transactions that do not meet this requirement
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We also need the recent blockhashes sysvar to be static 💀

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And we should explicitly state what happens if you append a bunch of lookup accounts to the advance nonce ix 💀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@jstarry jstarry Feb 4, 2025

Choose a reason for hiding this comment

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

Wait actually we don't need this change. We would never process the actual advance nonce ix for a tx with invalid lookups, we advance the nonce in the runtime. So only the nonce needs to be static. Let's revert 3328140

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

did we drop the in-program verify+advance?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so we never actually need the recent blockhashes sysvar to advance the nonce; but if the transaction would otherwise have succeeded, not having that account as an input would make it fail on the nonce ix processing?

That seems extremely odd, but probably not relevant to this SIMD

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, not having the recent blockhashes sysvar as an account input would make it fail the nonce ix processing in the SVM. But if nonce ix processing fails in the SVM, the runtime takes over and manually advances the nonce since the runtime obviously has access to the latest blockhash and therefore knows what the next durable nonce should be.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

signers are already required to be statically specified?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, signers are already required to be static.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still need to revert 3328140

@t-nelson
Copy link
Copy Markdown
Contributor

this ready for fd review?

@apfitzge
Copy link
Copy Markdown
Contributor Author

reached out to Tom for fd review. for some reason github won't let me request his review

Copy link
Copy Markdown
Contributor

@topointon-jump topointon-jump left a comment

Choose a reason for hiding this comment

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

I support making these simpler, just have one question about where the check itself should go.


- Nonce transactions MUST have a nonce account index that is less the number of
statically included accounts in the transaction.
- Leader nodes MUST drop nonce transactions that do not meet this requirement
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious - why is this check not in the SVM transaction sanitization, instead of the block builder? This might make sense from an API perspective, and so that users are charged for submitting these invalid transactions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it could be done in SVM, really anywhere before we get to actually publishing a block.
Likely the best place to do that is as early as possible though, since without loading the nonce account to determine if its valid or not we cannot charge fees.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@topointon-jump, as discussed elsewhere, agave will implement feature-gated checks in the runtime to check the new constraint and fail the transaction.

This detail is implementation specific, which iirc we should avoid in proposals, so making a comment here for your records. Agave's runtime will mark these transactions with a specific error, which should enable frankendancer to use it w/ minimal changes.

@apfitzge
Copy link
Copy Markdown
Contributor Author

@jstarry or @t-nelson, do you have any remaining comments? if not, would appreciate an approval so we can close this and begin implementation.

@agca52

This comment was marked as spam.

@apfitzge
Copy link
Copy Markdown
Contributor Author

@Benhawkins18 approval has been received from anza (@jstarry) and firedancer (@topointon-jump). Can we merge, or initiate "final comment" period for this proposal?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants