Migrate from address maps to address lookup tables by jstarry · Pull Request #21634 · solana-labs/solana

jstarry · 2021-12-06T14:50:24Z

Problem

The transaction v2 proposal has new proposed changes that rename "address maps" to "address lookup tables" and the design of address lookup tables has shifted.

Summary of Changes

Rename "address map indexes" to "address table lookups" and "mapped addresses" to "loaded addresses"
Address map account keys used to be appended to message.account_keys but address table lookups track the account key separately.

Fixes #

jstarry · 2021-12-08T04:03:07Z

@lijunwangs can you please advise at what the impact of changing these Postgres types and tables will be?

lijunwangs · 2021-12-08T16:50:31Z

+
+        // there should be at least 1 RW fee-payer account.
+        if self.header.num_readonly_signed_accounts >= self.header.num_required_signatures {
+            return Err(SanitizeError::IndexOutOfBounds);


I think probably SanitizeError::InvalidValue is more appropriate.

lijunwangs · 2021-12-08T16:54:02Z

+            let num_table_loaded_accounts = lookup
+                .writable_indexes
+                .len()
+                .saturating_add(lookup.readonly_indexes.len());


Do we need validations for writable_indexes?

The writable and readonly indexes are used to lookup addresses in an on-chain address table. We can't sanitize those here, the runtime will fail in another location if the lookup fails. The indexes can be as high as u8::MAX so there's no need to sanitize them

lijunwangs · 2021-12-08T16:59:18Z

+            }
+        }
+
+        Ok(())


Most of the errors returned in this function is IndexOutOfBounds, when one does run into such error -- it is actually difficult to figure out what causes it to fail. I guess some debug logging before the return will help.

This sanitization is done on the validator to filter out invalid transactions. It's not a method that users will interface with

Heh, I meant for the developer doing test. I found myself adding println! code in the error conditions to figure out what offended the sanitizer. I know this is not new code -- just copied from earlier location. And can be addressed separately.

lijunwangs · 2021-12-08T17:35:58Z

+#[serde(rename_all = "camelCase")]
+pub struct MessageAddressTableLookup {
+    /// Address lookup table account key
+    pub account_key: Pubkey,


How is the account_key used? Where do we actually store the table?

There are many such tables (users can create their own as they please) and they are all on-chain accounts so they are identified by this account key.

Got it! Thanks

lijunwangs · 2021-12-08T17:41:27Z

-CREATE TYPE "AddressMapIndexes" AS (
-    writable SMALLINT[],
-    readonly SMALLINT[]
+CREATE TYPE "TransactionMessageAddressTableLookup" AS (


Is the TransactionMessageAddressTable a global construct or stored in the transaction itself? If it is the former, we will have to persist that global table as well otherwise the indexes cannot be used.

Is the TransactionMessageAddressTable a global construct

It's stored on-chain so the full address table won't be included in the transaction object, but those addresses are resolved into "LoadedAddresses"

Cool. It should work then

jstarry · 2021-12-09T15:43:53Z

@lijunwangs can you please advise at what the impact of changing these Postgres types and tables will be?

lijunwangs · 2021-12-09T17:41:26Z

+            }
+        }
+
+        Ok(())


Heh, I meant for the developer doing test. I found myself adding println! code in the error conditions to figure out what offended the sanitizer. I know this is not new code -- just copied from earlier location. And can be addressed separately.

lijunwangs · 2021-12-09T17:43:23Z

+#[serde(rename_all = "camelCase")]
+pub struct MessageAddressTableLookup {
+    /// Address lookup table account key
+    pub account_key: Pubkey,


Got it! Thanks

lijunwangs · 2021-12-09T17:44:08Z

-CREATE TYPE "AddressMapIndexes" AS (
-    writable SMALLINT[],
-    readonly SMALLINT[]
+CREATE TYPE "TransactionMessageAddressTableLookup" AS (


Cool. It should work then

Pull request has been modified.

lijunwangs · 2021-12-09T19:45:04Z

 /// is bit is not set, all bytes are used to encode the legacy `Message`
 /// format.
-#[frozen_abi(digest = "x2F3RG2RhJQWN6L2N3jebvcAvNYFrhE3sKTPJ4sENvL")]
+#[frozen_abi(digest = "G4EAiqmGgBprgf5ePYemLJcoFfx4R7rhC1Weo2FVJ7fn")]


Just out of curiosity, how is the new digest generated?

It's generated using some procedural macros that build a type graph and hash the output. I just ran ./test-abi.sh locally to see what the mismatch was and updated the value.

codecov · 2021-12-09T21:41:16Z

Codecov Report

Merging #21634 (dec2065) into master (0224a8b) will decrease coverage by 0.0%.
The diff coverage is 89.0%.

@@            Coverage Diff            @@
##           master   #21634     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         511      511             
  Lines      143320   143304     -16     
=========================================
- Hits       116976   116962     -14     
+ Misses      26344    26342      -2

* Migrate from address maps to address lookup tables * update sanitize error * cargo fmt * update abi (cherry picked from commit 6c108c8)

* Migrate from address maps to address lookup tables * update sanitize error * cargo fmt * update abi (cherry picked from commit 6c108c8) Co-authored-by: Justin Starry <justin@solana.com>

jstarry added the v1.9 label Dec 6, 2021

jstarry force-pushed the migrate-address-maps branch from 04e54e3 to 5c96e59 Compare December 8, 2021 04:00

jstarry marked this pull request as ready for review December 8, 2021 04:02

jstarry requested a review from lijunwangs December 8, 2021 04:03

Migrate from address maps to address lookup tables

4e6c9d2

jstarry force-pushed the migrate-address-maps branch from 5c96e59 to 4e6c9d2 Compare December 8, 2021 04:04

lijunwangs reviewed Dec 8, 2021

View reviewed changes

jstarry added 2 commits December 9, 2021 10:42

update sanitize error

483c790

cargo fmt

a3c531c

lijunwangs previously approved these changes Dec 9, 2021

View reviewed changes

update abi

dec2065

lijunwangs reviewed Dec 9, 2021

View reviewed changes

jstarry merged commit 6c108c8 into solana-labs:master Dec 10, 2021

jstarry deleted the migrate-address-maps branch December 10, 2021 16:04

mergify Bot pushed a commit that referenced this pull request Dec 10, 2021

Migrate from address maps to address lookup tables (#21634)

b83f4ae

* Migrate from address maps to address lookup tables * update sanitize error * cargo fmt * update abi (cherry picked from commit 6c108c8)

mergify Bot mentioned this pull request Dec 10, 2021

Migrate from address maps to address lookup tables (backport #21634) #21773

Merged

brooksprumo mentioned this pull request Jan 5, 2022

v1.9/fix build #22311

Closed

jstarry mentioned this pull request Jul 4, 2022

Versioned Transaction and Address Lookup Table feature activation #26391

Closed

65 tasks

+                          }
+                      }
+                      Ok(())

Conversation

jstarry commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

jstarry commented Dec 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstarry commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 9, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jstarry commented Dec 6, 2021 •

edited

Loading