SIMD-0242: Static Nonce Account Only#5555
Conversation
jstarry
force-pushed
the
feat/static-nonce
branch
from
bfafac9 to
72a5c66
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5555 +/- ##
=========================================
- Coverage 83.4% 83.3% -0.1%
=========================================
Files 827 827
Lines 373587 373692 +105
=========================================
+ Hits 311580 311603 +23
- Misses 62007 62089 +82 🚀 New features to boost your workflow:
|
apfitzge
left a comment
apfitzge
left a comment
There was a problem hiding this comment.
Implementation looks correct to me.
The function that checks the nonce is static is run in both our initial scheduler filters as well as at runtime - @topointon-jump should be happy with this since the runtime check will mean this just works for frankendancer.
A follow-up qusetion I have with this change...should we allow our SDK type constructors to even construct such a transaction?
| @@ -1,4 +1,5 @@ | |||
| #![cfg(test)] | |||
| #![allow(clippy::arithmetic_side_effects)] | |||
There was a problem hiding this comment.
was just lazy when copying in some message creation code. I replaced the offending arithmetic with saturating and checked methods instead: cf4f96a
| ix.accounts.first().and_then(|idx| { | ||
| let index = usize::from(*idx); | ||
| if !self.is_writable(index) { | ||
| if (require_static_nonce_account && index >= self.static_account_keys().len()) |
There was a problem hiding this comment.
It looks like we really only require the number of static account keys;
I think exposing static account keys is reasonable. Can you think of any reason we'd want to limit the interface to only the number of static account keys?
There was a problem hiding this comment.
It looks like we really only require the number of static account keys;
Yeah, just figured we will be using static_account_keys more in the future so I exposed the full slice rather than just the length.
Can you think of any reason we'd want to limit the interface to only the number of static account keys?
Nope, we always have the full list of static account keys available, seems fine to expose it
| message: &impl SVMMessage, | ||
| ) -> Option<(Pubkey, AccountSharedData, NonceData)> { | ||
| let nonce_address = message.get_durable_nonce()?; | ||
| let require_static_nonce_account = self |
There was a problem hiding this comment.
Should we pass this in instead of lookup in feature-set per transaction?
There was a problem hiding this comment.
I did consider that. I didn't love that it would technically allow calling a bank method with a feature gate enabled which the bank didn't have enabled. Also this will only get called for transactions which don't have a recent blockhash, so not too bad I think. Would be nice to do some refactoring in this area tho, wdyt?
Had the same thought, I'll look into making a change to sdk for this. Seems unlikely for any folks to have a nonce account in an ALT but still seems nice. |
SDK PR is here: anza-xyz/solana-sdk#100 |
| (require_static_nonce_account::id(), "SIMD-0242: Static Nonce Account Only"), | ||
| (raise_block_limits_to_60m::id(), "Raise block limit to 60M SIMD-0256"), | ||
| /*************** ADD NEW FEATURES HERE ***************/ |
There was a problem hiding this comment.
any reason to not put this at the end?
| message: &impl SVMMessage, | ||
| ) -> Option<(Pubkey, AccountSharedData, NonceData)> { | ||
| let nonce_address = message.get_durable_nonce()?; | ||
| let require_static_nonce_account = self |
4 participants
Problem
Durable nonce transactions were unintentionally allowed to load their nonce accounts from address lookup tables rather than restricted to being a static account.
Summary of Changes
Implement SIMD-0242 by adding a feature gate to start requiring nonces to be static
Fixes #