Refactor instruction compilation and update message account key ordering #23729

jstarry · 2022-03-17T16:10:14Z

Problem

Instruction compilation needs to be performed for other message versions but is currently closely tied to the legacy message implementation

Summary of Changes

Message account key ordering is still deterministic but keys are no longer sorted by the order of their appearance in instruction accounts, they are sorted by default sort order of PublicKey
Add compile_instructions method to AccountKeys to enable future support for compiling instructions for messages using on-chain address lookup tables
Add CompiledKeys struct for compiling the account keys for transaction messages. This will be used in Add SDK support for creating transactions with address table lookups #23728 for compiling message keys for v0 transaction messages

Fixes #

t-nelson

lgtm with a few nits. any idea if there's a perf delta? my gut says this is faster, but get_keys was such a cluster that it's hard to be sure

t-nelson · 2022-03-18T05:19:52Z

sdk/program/src/message/compiled_keys.rs

+    pub(crate) fn try_create_message_header(&self) -> Option<MessageHeader> {
+        Some(MessageHeader {
+            num_required_signatures: u8::try_from(
+                self.writable_signer_keys
+                    .len()
+                    .checked_add(self.readonly_signer_keys.len())?,
+            )
+            .ok()?,
+            num_readonly_signed_accounts: u8::try_from(self.readonly_signer_keys.len()).ok()?,
+            num_readonly_unsigned_accounts: u8::try_from(self.readonly_non_signer_keys.len())
+                .ok()?,
+        })
+    }
+
+    pub(crate) fn into_message_static_keys(self) -> Vec<Pubkey> {


almost seems like combining these into a

pub(crate) fn into_message_components(self) -> Option<(MessageHeader, Vec<Pubkey>)>

would be more ergonomic. wdyt?

Hmm yeah that's what I had originally, switched back to that.

t-nelson · 2022-03-18T05:23:43Z

sdk/program/src/message/compiled_keys.rs

+    use {super::*, crate::instruction::AccountMeta};
+
+    #[test]
+    fn test_compile_with_dup_keys() {


this name threw me off 'cause the key dupe comes from duplicating the instruction rather than an AccountMeta

sdk/program/src/message/compiled_keys.rs

sdk/program/src/message/legacy.rs

Pull request has been modified.

jstarry · 2022-03-18T07:17:14Z

lgtm with a few nits. any idea if there's a perf delta? my gut says this is faster, but get_keys was such a cluster that it's hard to be sure

Thanks for the review. Any reason you're asking about perf here? This code path is only used by validators when voting. I think there could be a small hit for transactions with a small number of keys but yeah my gut says that this is faster because it removes an O(N^2) loop and we were already building a BTreeSet to dedup program ids anyways, might as well extend it to all transaction keys.

t-nelson · 2022-03-18T16:21:05Z

Any reason you're asking about perf here? This code path is only used by validators when voting.

Just thinking about some of the higher TPS client-side use-cases. Pretty sure voting can absorb any (negligible) negative delta this might introduce, might compound in other situations though.

t-nelson

thanks! lgtm

Pull request has been modified.

CriesofCarrots · 2022-04-19T19:00:53Z

I guess I missed this one while I was ooo. Ended up being a bit of a time-bomb in parse_token when starting to pull in solana-program v1.10 :(

* Add SPL Token 2022 to the list of known token ids * Fix tests to accommodate #23729 * Test parsing of basic token-2022 instructsions Co-authored-by: Tyera Eulberg <tyera@solana.com>

jstarry · 2022-04-20T07:56:47Z

I guess I missed this one while I was ooo. Ended up being a bit of a time-bomb in parse_token when starting to pull in solana-program v1.10 :(

Oh no, what was the issue that you encountered? I see a commit that fixes tests, was there anything beyond broken tests that this caused issues for?

CriesofCarrots · 2022-04-21T05:32:35Z

Ha, no, sorry to be melodramatic! It was just the tests that were broken. Took me a minute to figure out why, though, since the other crate tests were fine.

* Add SPL Token 2022 to the list of known token ids * Fix tests to accommodate #23729 * Test parsing of basic token-2022 instructsions Co-authored-by: Tyera Eulberg <tyera@solana.com> (cherry picked from commit 4138066) # Conflicts: # Cargo.lock # programs/bpf/Cargo.lock

…24625) * Add SPL Token 2022 to the list of known token ids (#23067) * Add SPL Token 2022 to the list of known token ids * Fix tests to accommodate #23729 * Test parsing of basic token-2022 instructsions Co-authored-by: Tyera Eulberg <tyera@solana.com> (cherry picked from commit 4138066) # Conflicts: # Cargo.lock # programs/bpf/Cargo.lock * Fix lock conflicts Co-authored-by: Michael Vines <mvines@gmail.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

Cribbed from #23729 to ease backports

…#26057) * Make instruction-parsing tests less brittle Cribbed from #23729 to ease backports * RPC instruction parser tests are missing some cases (#25951) * Fix num-accounts typo and extend test * Add extra test cases to parse_system * Add extra test cases to parse_vote * Add extra test cases to parse_stake * Fixup parse_bpf_loader * Add extra test cases to parse_bpf_upgradeable_loader * Add extra test cases to parse_associated_token Co-authored-by: Justin Starry <justin@solana.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

* Add SPL Token 2022 to the list of known token ids * Fix tests to accommodate solana-labs#23729 * Test parsing of basic token-2022 instructsions Co-authored-by: Tyera Eulberg <tyera@solana.com>

jstarry mentioned this pull request Mar 17, 2022

Support sending VersionedTransaction in tests #22862

Closed

jstarry requested a review from t-nelson March 17, 2022 16:17

jstarry changed the title ~~Refactor: Make instruction compilation usable for other message versions~~ Refactor instruction compilation and relax message account key ordering strictness Mar 18, 2022

jstarry changed the title ~~Refactor instruction compilation and relax message account key ordering strictness~~ Refactor instruction compilation and update message account key ordering Mar 18, 2022

jstarry force-pushed the refactor/compiled-keys branch 2 times, most recently from 68866fd to 29d0af5 Compare March 18, 2022 02:28

t-nelson previously approved these changes Mar 18, 2022

View reviewed changes

jstarry force-pushed the refactor/compiled-keys branch from 29d0af5 to 726cff5 Compare March 18, 2022 07:11

t-nelson previously approved these changes Mar 18, 2022

View reviewed changes

jstarry force-pushed the refactor/compiled-keys branch from ba344ff to 4d88fa8 Compare March 21, 2022 04:17

jstarry added 3 commits March 21, 2022 12:42

Refactor: Make instruction compilation usable for other message versions

c1c8c0d

apply trents feedback

6a37a56

Fix tests

19cb3db

jstarry force-pushed the refactor/compiled-keys branch from 4d88fa8 to 19cb3db Compare March 21, 2022 04:45

Fix bpf compatiblity

8e6fae3

jstarry merged commit 1535748 into solana-labs:master Mar 21, 2022

jstarry deleted the refactor/compiled-keys branch March 21, 2022 13:59

CriesofCarrots pushed a commit to mvines/solana that referenced this pull request Apr 19, 2022

Fix tests to accommodate solana-labs#23729

dec597d

CriesofCarrots pushed a commit to mvines/solana that referenced this pull request Apr 19, 2022

Fix tests to accommodate solana-labs#23729

4646b36

CriesofCarrots pushed a commit that referenced this pull request Jun 18, 2022

Make instruction-parsing tests less brittle

efc0d0b

Cribbed from #23729 to ease backports

CriesofCarrots pushed a commit that referenced this pull request Jun 18, 2022

Make instruction-parsing tests less brittle

4a77b86

Cribbed from #23729 to ease backports

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor instruction compilation and update message account key ordering #23729

Refactor instruction compilation and update message account key ordering #23729

jstarry commented Mar 17, 2022 •

edited

Loading

t-nelson left a comment

t-nelson Mar 18, 2022

jstarry Mar 18, 2022 •

edited

Loading

t-nelson Mar 18, 2022

jstarry commented Mar 18, 2022

t-nelson commented Mar 18, 2022

t-nelson left a comment

CriesofCarrots commented Apr 19, 2022

jstarry commented Apr 20, 2022

CriesofCarrots commented Apr 21, 2022

Refactor instruction compilation and update message account key ordering #23729

Refactor instruction compilation and update message account key ordering #23729

Conversation

jstarry commented Mar 17, 2022 • edited Loading

Problem

Summary of Changes

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Mar 18, 2022

Choose a reason for hiding this comment

jstarry Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

t-nelson Mar 18, 2022

Choose a reason for hiding this comment

jstarry commented Mar 18, 2022

t-nelson commented Mar 18, 2022

t-nelson left a comment

Choose a reason for hiding this comment

CriesofCarrots commented Apr 19, 2022

jstarry commented Apr 20, 2022

CriesofCarrots commented Apr 21, 2022

jstarry commented Mar 17, 2022 •

edited

Loading

jstarry Mar 18, 2022 •

edited

Loading