generic get nonce and ix signers #2826

apfitzge · 2024-09-03T22:28:18Z

Problem

Need getting nonce to be generic over tx trait (or using RuntimeTransaction cached value in future?)

Summary of Changes

implement get_durable_nonce and get_ix_signers for checking transaction
add tests

Fixes #

apfitzge · 2024-09-03T22:29:33Z

runtime/Cargo.toml

@@ -62,6 +62,7 @@ solana-loader-v4-program = { workspace = true }
 solana-measure = { workspace = true }
 solana-metrics = { workspace = true }
 solana-perf = { workspace = true }
+solana-program = { workspace = true }


limited_deserialize from sdk is a trick, the one from here is what we need

apfitzge · 2024-09-04T13:18:46Z

runtime/src/bank/check_transactions.rs

+    }
+
+    #[test]
+    fn test_get_ix_signers() {


this test was copied from sdk and just modified to test the new generic fn

apfitzge · 2024-09-04T13:19:19Z

runtime/src/bank/check_transactions.rs

@@ -345,4 +408,273 @@ mod tests {
            .check_and_load_message_nonce_account(&message, &bank.next_durable_nonce())
            .is_none());
    }
+
+    #[test]
+    fn test_get_durable_nonce() {


afaict, there are no tests for the equivalent function in sdk. I did my best to cover the branches in logic.

tao-stones

looks good overall. Will look into tests if you decided to keep these functions in bank, or move them out.

sdk/program/src/program_utils.rs

tao-stones · 2024-09-05T23:23:04Z

runtime/src/bank/check_transactions.rs

@@ -184,6 +188,54 @@ impl Bank {
    }
 }

+/// If the message uses a durable nonce, return the pubkey of the nonce account
+fn get_durable_nonce(message: &impl SVMMessage) -> Option<&Pubkey> {


Would it be possible to make this, and get_is_signer(), functions extension of SVMMessage, or at least make them live outside of Bank - they don't need `bank anyway?

Moved functions and tests to a new mod inside runtime: svm_message_ext.

runtime/src/bank/check_transactions.rs

tao-stones

lgtm - just a nit and CI

runtime/src/lib.rs

jstarry · 2024-09-10T01:59:19Z

runtime/src/lib.rs

@@ -34,6 +34,7 @@ pub mod stake_weighted_timestamp;
 pub mod stakes;
 pub mod static_ids;
 pub mod status_cache;
+pub mod svm_message_ext;


Can we keep these extensions in the svm-transaction crate? Even though these methods are used by the runtime, they're still useful in other contexts. Off chain clients may want to check if a transaction uses a durable nonce for example.

It'd add a dependency on bincode in svm-transaction trait, which is undesirable.

When I talked with @buffalojoec about this we discussed creating a separate crate for derived behavior.
Not sure if we'd like to do that now, or wait until there's a larger set of derived behaviors.
Didn't feel right to create a new crate for 2 functions, but lmk if you feel that'd be more appropriate.

I'm definitely in favor of leaving it in runtime until it's needed off-chain, or creating a new crate. I also agree svm-transaction should be lightweight and basically only used to roll a custom transaction type beyond the ones provided by SDK, for interacting with the SVM API.

another option is to rework the implementation to avoid bincode - would be relatively straight-forward since all we use it for is to read a 4-byte discriminant.

@jstarry thoughts on which approach you think is best here?

Just adding one more point to supplement my argument for the runtime/new crate. Lmk if it's off-base.

Even though nonce management is split between runtime and SVM right now, the concept of a durable nonce is actually more pertinent to the runtime, when checking transaction age. There's an argument to be made for actually making the act of advancing the nonce during transaction execution a plugin that extends SVM, since not everyone will use nonces in other execution environments.

Anyway that's my perspective on it. I think we should keep the SVM crate composable and as light as possible, opening up custom behavior through plugins implemented elsewhere rather than bloat.

I agree with @buffalojoec that this is a runtime concept and doesn't make sense for it to live in the svm transaction crate. I think it's going to all make much more sense when we have a "runtime transaction" abstraction

buffalojoec · 2024-09-11T14:53:49Z

runtime/src/bank/check_transactions.rs

@@ -193,7 +193,7 @@ mod tests {
            setup_nonce_with_bank,
        },
        solana_sdk::{
-            feature_set::FeatureSet, hash::Hash, message::Message, signature::Keypair,
+            self, feature_set::FeatureSet, hash::Hash, message::Message, signature::Keypair,


Do you want this self in here?

buffalojoec · 2024-09-11T14:55:48Z

runtime/src/lib.rs

@@ -34,6 +34,7 @@ pub mod stake_weighted_timestamp;
 pub mod stakes;
 pub mod static_ids;
 pub mod status_cache;
+pub mod svm_message_ext;


nit: I don't feel strongly about this, but I suggest a different module name like nonce_extraction_utils (or better), to avoid this one day becoming a catch-all for SVM message extensions. Or maybe that's your intention..? 😅

buffalojoec · 2024-09-11T15:00:35Z

runtime/src/lib.rs

@@ -34,6 +34,7 @@ pub mod stake_weighted_timestamp;
 pub mod stakes;
 pub mod static_ids;
 pub mod status_cache;
+pub mod svm_message_ext;


Just adding one more point to supplement my argument for the runtime/new crate. Lmk if it's off-base.

Even though nonce management is split between runtime and SVM right now, the concept of a durable nonce is actually more pertinent to the runtime, when checking transaction age. There's an argument to be made for actually making the act of advancing the nonce during transaction execution a plugin that extends SVM, since not everyone will use nonces in other execution environments.

Anyway that's my perspective on it. I think we should keep the SVM crate composable and as light as possible, opening up custom behavior through plugins implemented elsewhere rather than bloat.

apfitzge · 2024-09-14T21:44:42Z

@buffalojoec think i hit all your comments now, forgot to ping friday!

apfitzge changed the title ~~generic get nonce and ix signers + tests~~ generic get nonce and ix signers Sep 3, 2024

apfitzge commented Sep 3, 2024

View reviewed changes

apfitzge marked this pull request as ready for review September 4, 2024 12:44

apfitzge requested review from jstarry and tao-stones September 4, 2024 12:45

apfitzge self-assigned this Sep 4, 2024

apfitzge commented Sep 4, 2024

View reviewed changes

tao-stones reviewed Sep 5, 2024

View reviewed changes

tao-stones previously approved these changes Sep 6, 2024

View reviewed changes

runtime/src/lib.rs Outdated Show resolved Hide resolved

jstarry reviewed Sep 10, 2024

View reviewed changes

buffalojoec reviewed Sep 11, 2024

View reviewed changes

apfitzge added 6 commits September 12, 2024 14:17

generic get nonce and ix signers + tests

d933f37

move to svm_message_ext

4b81290

remove solana_sdk::self import

6820d69

rename to nonce_extraction

6136223

limit publicness

f1eb581

fix

05bd262

apfitzge dismissed tao-stones’s stale review via 05bd262 September 12, 2024 19:20

apfitzge force-pushed the nonce_generic branch from 0e86751 to 05bd262 Compare September 12, 2024 19:20

fix

7aa81ac

apfitzge requested a review from buffalojoec September 14, 2024 21:44

jstarry approved these changes Sep 25, 2024

View reviewed changes

apfitzge merged commit 56672cd into anza-xyz:master Sep 25, 2024
52 checks passed

apfitzge deleted the nonce_generic branch September 25, 2024 11:30

ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024

generic get nonce and ix signers (anza-xyz#2826)

ee8945a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic get nonce and ix signers #2826

generic get nonce and ix signers #2826

apfitzge commented Sep 3, 2024

apfitzge Sep 3, 2024

apfitzge Sep 4, 2024

apfitzge Sep 4, 2024 •

edited

Loading

tao-stones left a comment

tao-stones Sep 5, 2024

apfitzge Sep 6, 2024

tao-stones left a comment

jstarry Sep 10, 2024

apfitzge Sep 10, 2024 •

edited

Loading

buffalojoec Sep 10, 2024

apfitzge Sep 10, 2024

apfitzge Sep 11, 2024

buffalojoec Sep 11, 2024

jstarry Sep 14, 2024

buffalojoec Sep 11, 2024

buffalojoec Sep 11, 2024

buffalojoec Sep 11, 2024

apfitzge commented Sep 14, 2024

generic get nonce and ix signers #2826

generic get nonce and ix signers #2826

Conversation

apfitzge commented Sep 3, 2024

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apfitzge commented Sep 14, 2024

apfitzge Sep 4, 2024 •

edited

Loading

apfitzge Sep 10, 2024 •

edited

Loading