Add SDK support for creating transactions with address table lookups #23728

jstarry · 2022-03-17T16:06:35Z

Problem

Rust SDK doesn't provide any convenience APIs for creating transactions which make use of on-chain lookup tables.

Summary of Changes

Added CompileError error type to differentiate types of message compilation failures
Added Message::try_compile method to compile messages using address lookup tables
Added CompiledKeys::try_extract_table_lookup to remove any non-signer compiled keys that can be replaced with an address table lookup
AccountKeys::compile_instructions is now fallible and was renamed to AccountKeys::try_compile_instructions. It returns an error when compiling instructions with an unknown key or if u8 conversion fails
CompiledKeys::try_into_message_components now returns a Result instead of an Option to make use of the new CompileError

Fixes #

sdk/program/src/message/account_keys.rs

t-nelson · 2022-03-24T16:00:35Z

sdk/program/src/message/account_keys.rs

+    ) -> Result<Vec<CompiledInstruction>, CompileError> {
+        let mut account_index_map = BTreeMap::<&Pubkey, u8>::new();
+        for (index, key) in self.iter().enumerate() {
+            let index = u8::try_from(index).map_err(|_| CompileError::TooManyAccountKeys)?;


let's improve self-doc with an explicit constant for max supported, rather than implicitly relying on u8::MAX.

Also seems like we should be preventing a too large AccountKeys from ever being created rather than checking it here (and presumably other places in the future)

There's a runtime limit of 64 but this compile error is for correctness not for runtime validity. I just want to make sure that the index actually fits in a u8 because that's how we serialize it in a transaction. There's many ways to create an invalid transaction on the client side so I think it's ok for now to allow large AccountKeys to be created because they will be rejected during sanitization and the runtime check could change in the future so I don't want to hardcode that into the client.

hmm, the error seems misleading in that case since we can have too many account keys without getting a TooManyAccountKeys 🤔

i typically prefer to fail asap rather than let pathology fester until some other process notices later. killing it at the client, especially in the first implementation, which the others will copy seems like the less wrong thing to do

hmm, the error seems misleading in that case since we can have too many account keys without getting TooManyAccountKeys 🤔

Agreed, that was a bad name. I updated to AccountIndexOverflow and AddressTableLookupIndexOverflow.

i typically prefer to fail asap rather than let pathology fester until some other process notices later. killing it at the client, especially in the first implementation, which the others will copy seems like the less wrong thing to do

I'm hesitant to do runtime checks on the client because runtime rules can change over time. For example, the serialized size of a transaction could change, the total number of accounts lockable by a transaction (currently 64) could be updated later, etc.

sdk/program/src/message/account_keys.rs

sdk/program/src/message/compiled_keys.rs

jstarry · 2022-03-27T02:02:46Z

@t-nelson can you take another look? CI failure is unrelated to this change

t-nelson

lgtm, just a nit

a rebase should get you past that downstream build error

sdk/program/src/message/compiled_keys.rs

Pull request has been modified.

mergify · 2022-03-31T04:32:32Z

automerge label removed due to a CI failure

jstarry · 2022-03-31T04:34:11Z

Yay new downstream build failure:

error[E0432]: unresolved imports `bytemuck_derive::AnyBitPattern`, `bytemuck_derive::CheckedBitPattern`, `bytemuck_derive::NoUninit`
--


    <span class="term-fgx12 term-fg1">| </span>  <span class="term-fgx9 term-fg1">no `AnyBitPattern` in the root</span>

mergify · 2022-03-31T06:24:27Z

automerge label removed due to a CI failure

…olana-labs#23728) * Add SDK support for creating transactions with address table lookups * fix bpf compilation * rename compile error variants to indicate overflow * Add doc tests * fix bpf compatibility * use constant for overflow tests * Use cfg_attr for dead code attribute * resolve merge conflict

jstarry mentioned this pull request Mar 17, 2022

Refactor instruction compilation and update message account key ordering #23729

Merged

jstarry force-pushed the lut/create-v0-message branch from 5a331a9 to 924ede5 Compare March 17, 2022 16:14

jstarry mentioned this pull request Mar 17, 2022

Support sending VersionedTransaction in tests #22862

Closed

jstarry force-pushed the lut/create-v0-message branch 7 times, most recently from 6ad1047 to e1673e1 Compare March 24, 2022 09:37

jstarry changed the title ~~Add API for creating transactions with address table lookups~~ Add SDK support for creating transactions with address table lookups Mar 24, 2022

jstarry marked this pull request as ready for review March 24, 2022 09:38

jstarry requested a review from t-nelson March 24, 2022 09:38

jstarry force-pushed the lut/create-v0-message branch from e1673e1 to 39e1242 Compare March 24, 2022 10:10

t-nelson reviewed Mar 24, 2022

View reviewed changes

jstarry force-pushed the lut/create-v0-message branch 5 times, most recently from d090ae5 to 05c90a5 Compare March 25, 2022 09:54

jstarry added the CI Pull Request is ready to enter CI label Mar 25, 2022

solana-grimes removed the CI Pull Request is ready to enter CI label Mar 25, 2022

jstarry force-pushed the lut/create-v0-message branch from 05c90a5 to e066c6a Compare March 25, 2022 14:48

t-nelson previously approved these changes Mar 30, 2022

View reviewed changes

sdk/program/src/message/compiled_keys.rs Outdated Show resolved Hide resolved

jstarry force-pushed the lut/create-v0-message branch from 800170d to dece769 Compare March 31, 2022 03:13

jstarry added the automerge Merge this Pull Request automatically once CI passes label Mar 31, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Mar 31, 2022

jstarry added 7 commits March 31, 2022 12:55

Add SDK support for creating transactions with address table lookups

4a86267

fix bpf compilation

565d87b

rename compile error variants to indicate overflow

a77602c

Add doc tests

ed07b44

fix bpf compatibility

50384ea

use constant for overflow tests

1b5be73

Use cfg_attr for dead code attribute

f3f8fcb

jstarry force-pushed the lut/create-v0-message branch from dece769 to 04a7a8a Compare March 31, 2022 05:10

jstarry added the automerge Merge this Pull Request automatically once CI passes label Mar 31, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Mar 31, 2022

resolve merge conflict

e0771ee

jstarry force-pushed the lut/create-v0-message branch from 04a7a8a to e0771ee Compare March 31, 2022 07:07

jstarry merged commit 8832653 into solana-labs:master Mar 31, 2022

jstarry deleted the lut/create-v0-message branch March 31, 2022 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SDK support for creating transactions with address table lookups #23728

Add SDK support for creating transactions with address table lookups #23728

jstarry commented Mar 17, 2022 •

edited

Loading

t-nelson Mar 24, 2022

jstarry Mar 24, 2022

t-nelson Mar 24, 2022

jstarry Mar 25, 2022

jstarry commented Mar 27, 2022

t-nelson left a comment

mergify bot commented Mar 31, 2022

jstarry commented Mar 31, 2022

mergify bot commented Mar 31, 2022

Add SDK support for creating transactions with address table lookups #23728

Add SDK support for creating transactions with address table lookups #23728

Conversation

jstarry commented Mar 17, 2022 • edited Loading

Problem

Summary of Changes

t-nelson Mar 24, 2022

Choose a reason for hiding this comment

jstarry Mar 24, 2022

Choose a reason for hiding this comment

t-nelson Mar 24, 2022

Choose a reason for hiding this comment

jstarry Mar 25, 2022

Choose a reason for hiding this comment

jstarry commented Mar 27, 2022

t-nelson left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 31, 2022

jstarry commented Mar 31, 2022

mergify bot commented Mar 31, 2022

jstarry commented Mar 17, 2022 •

edited

Loading