AddressLoader: allow for non-owned MessageAddressTableLookup

[apfitzge]

Problem

• [Tracking] TransactionView #2255 - we no longer have owned data for these MessageAddressTableLookup nor do we have a slice.
• The interface for AddressLoader is overly restrictive

Summary of Changes

• Add new function to AddressLoader which allows a for a non-owning iterator
• Keep the old function to mostly allow for backwards compatibility

Fixes #

[apfitzge]

I hate adding something new to sdk. But feel we are somewhat bound by previous mistakes in the area.

• SanitizedTransaction construction depends on this trait (rather than LoadedAddresses).
• ^ above forces AddressLoader to be part of sdk
• We originally chose to just re-use the MessageAddressTableLookup for AddressLoader (easier at the time)
• ^ above forces us to have an owned MessageAddressTableLookup even though the behavior it is supposed to abstract does not require that ownership

[apfitzge]

@jstarry think you're familiar with loading. Please let me know what you think.
For now, I'm keeping the interface which forces allocation internally - though it seems possible we may in the future want to allow for a pre-allocated vec to be passed in and populated.

@LucasSte relevant to the new TransactionView and iteration over the ATLs - iterator would use Item = MessageAddressTableLookupRef<'a>

Another option would be to move this out of sdk entirely, and make SanitizedTransaction::try_new just take LoadedAddresses as an argument instead.

[jstarry]

Can you explain more why you wouldn't have owned data or even a slice of lookups?

[apfitzge]

Can you explain more why you wouldn't have owned data or even a slice of lookups?

The new transaction type(s) that are in progress, do not deserialize data in the same way our current transaction types. There's no forced allocation - the transaction accesses fields directly from the packet data and some cached offsets.

Some difficulty comes with the instructions and ATLs in the message, since these structs do not have fixed serialized sizes (because their internal vecs are serialized inline).
Current plan is to handle ATLs similarly to how we handled instructions: #2580 - only providing an interface to iterate over them (that's the only way we ever use them).

[jstarry]

Got it, makes sense to me. Can we add a new AddressLoaderByRef (naming?) trait instead of adding a new method to the existing trait? Adding a new method is technically a breaking change to the sdk. And yes it's rather unfortunate that this all lives in the sdk.

[apfitzge]

Got it, makes sense to me. Can we add a new AddressLoaderByRef (naming?) trait instead of adding a new method to the existing trait? Adding a new method is technically a breaking change to the sdk. And yes it's rather unfortunate that this all lives in the sdk.

That seems reasonable. It seems we could move this new trait out of sdk. Though I wonder where the best place for it is...new crate, or maybe runtime?

For Bank we change the impl of AddressLoader to call this new trait, but keep the old sdk trait for SanitizedTransaction stuff.

edit: Thinking for a moment, I think runtime is probably good. This address resolver is ideally non-public (imo). It should be meant strictly for our runtime to prepare transactions for execution, not used by users.

[jstarry]

Yeah runtime sounds reasonable to me. Note that it's still useful for users to construct a SanitizedTransaction by supplying their own loaded addresses (they can get them from tx metadata via RPC). But this doesn't need to be via a trait necessarily.

[apfitzge]

Yeah runtime sounds reasonable to me. Note that it's still useful for users to construct a SanitizedTransaction by supplying their own loaded addresses (they can get them from tx metadata via RPC). But this doesn't need to be via a trait necessarily.

Yeah, my thinking is we won't change SanitizedTransaction. Our internal code won't use it anymore (eventually), but it the user-facing type and I don't see that going away entirely.

[apfitzge]

Don't even need a trait - this is just a function on Bank. We will/can use this for resolving our transactions inside core/runtime.

[apfitzge]

struct defined here because ideally this is the same type that Bank's function takes. If same struct, needs to be somewhere that's accessible to both transaction-view and runtime, so this made more sense to me. I don't think it makes much sense to put this in its' own crate for something so simple; nor does it belong in svm-transaction since those are already resolved and SVM needs no knowledge of these.

[apfitzge]

Iterator to access these using TransactionMeta is not here yet, want to agree on where this lives and I'll do a follow-up PR to add the iterator once this is settled.

[apfitzge]

Welp - turns out I am actually dumb. I already have the type I need, from svm-transaction.

It is there beacuse we (sometimes) need to re-sanitize transactions in the runtime - though that's not explicitly part of SVM.

[LucasSte]

It is advantageous to add another indirection before calling the function? It seems to me that we are doing more work to first create the references then to call the function.

[apfitzge]

Change here will not make this any faster - and you're right, it may even be slightly slower (copying a reference and 2 slice ptrs => ~40 bytes).

[apfitzge]

^ To clarify - we do this mapping so we can use a shared implementation. Not for a specific benefit to the current function.

Additionally, the plan is for this function to not be called in our processing pipeline.