feat: add support for creating version 0 transactions by jstarry · Pull Request #27142 · solana-labs/solana

jstarry · 2022-08-15T09:27:08Z

Problem

Can't create version 0 transactions with web3

Summary of Changes

Added VersionedMessage type which can be used to represent legacy or v0 messages
Added VersionedTransaction class which is message version agnostic by using VersionedMessage
Added MessageV0 class to serialize/deserialize v0 messages
Implemented serialization/deserialization for v0 transactions and messages
Introduced MessageCompiledInstruction type as a successor to the CompiledInstruction type because it's inefficient and unnecessary to encode instruction data as a base58 string

Fixes #

codecov · 2022-08-15T11:05:54Z

Codecov Report

Merging #27142 (e92391c) into master (a97346a) will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #27142     +/-   ##
=========================================
+ Coverage    76.9%    77.0%   +0.1%     
=========================================
  Files          48       51      +3     
  Lines        2505     2649    +144     
  Branches      355      360      +5     
=========================================
+ Hits         1927     2041    +114     
- Misses        448      476     +28     
- Partials      130      132      +2

jordaaash

Looks great, easy to follow, I had only minor feedback.

jordaaash · 2022-08-17T18:56:11Z

+/**
+ * Layout for a signature
+ */
+export const signature = (property: string = 'signature') => {
+  return BufferLayout.blob(64, property);
+};
+


Should we add this to https://github.com/solana-labs/buffer-layout-utils/blob/master/src/web3.ts and consume it here? Could move most or all of these into it and eliminate some duplication

I'd be happy with that but I don't feel it's a big priority for simple layouts for signatures and public keys

jordaaash · 2022-08-17T19:26:30Z

-  accounts: number[];
-  /** The program input data encoded as base 58 */
-  data: string;
+  accountKeyIndexes: number[];


Feels like this should be accounts if it represents the same thing as in CompiledInstruction? MessageCompiledInstruction is a bit confusing as a name (it's just a CompiledInstruction with the data raw instead of encoded)

MessageCompiledInstruction is meant to be the successor to CompiledInstruction. Encoding the data is unnecessary and inefficient so that was the main reason I wanted to move away from CompiledInstruction. The accounts -> accountKeyIndexes isn't as big of a deal but I feel that it's more clear and descriptive.

jordaaash · 2022-08-17T19:29:58Z

    );
  }

+  get version(): TransactionVersion {


Conceptually feels a little weird that a TransactionVersion is referenced by a Message, instead of say a MessageVersion. While the transaction is versioned, isn't it because the message is versioned rather than the other way around?

I personally feel that it's valid to talk about the version of a transaction when you really mean the version of a transaction's message. The only thing that makes this a "message version" to me is the need to encode the version in the signed message, but it really refers to the structure of the transaction as a whole. It's likely that in the future, the structure of the signatures portion of the transaction is updated and then versioning won't be specific to the message structure as it appears to be right now since v0 transactions didn't update how signatures are encoded.

jordaaash · 2022-08-17T21:16:36Z

+ * Common interface for versioned messages and legacy messages
+ */
+export interface VersionedMessage {
+  get version(): TransactionVersion;


Yeah, MessageVersion makes way more sense to me here. wdyt?

jordaaash · 2022-08-17T21:32:44Z

+        const transferIxData = encodeData(SYSTEM_INSTRUCTION_LAYOUTS.Transfer, {
+          lamports: BigInt(LAMPORTS_PER_SOL),
+        });
+        const transaction = new VersionedTransaction(


All this looks good. I'm imagining from #27213 that there will later be some higher level interface than using lookup table indexes directly?

Yup, that higher level interface looks like this:

const txMessage = new TransactionMessage({ instructions, recentBlockhash, payerKey, }); const versionedTx = new VersionedTransaction( txMessage.compile({ version: 0, addressLookupTableAccounts, }); );

steveluscher · 2022-08-25T22:10:06Z

+  writableIndexes: Array<number>;
+  readonlyIndexes: Array<number>;


I was looking for a way that you could architect this type such that it was impossible to screw it up. As written, you could create malformed data like this:

const matl: MessageAddressTableLookup = { accountKey: /* ... */, writableIndexes: [0, 1], readonlyIndexes: [1, 2], };

If the storage was, instead, implemented as a sparse array, it would be impossible to overlap a readable/writeable index.

type Readable = 0; // or 'r' type Writeable = 1; // or 'w' type IndexesWithWriteability = (Readable | Writeable | undefined)[]; const indexes: IndexesWithWriteability = []; indexes[0] = 1; indexes[1] = 1; indexes[1] = 0; // OVERWRITTEN! Note how overlapping index 1 is impossible here. indexes[2] = 0;

Then you'd do stuff like this at the point of serialization:

const readonlyIndexes = []; const writeableIndexes = []; indexes.forEach((writeability, index) => { if (writeability === 0) { readonlyIndexes.push(index); } else if (writeability === 1) { writeableIndexes.push(index); } }); const writeableIndexesLength = shortvec.encodeLength( encodedWriteableIndexesLength, writeableIndexes.length, );

Oh yeah great point, it's definitely possible to create invalid transaction messages with the MessageV0 constructor. This MessageAddressTableLookup type directly maps to the serialization format of v0 messages (MessageV0 is just a barebones data class) so it's not meant to be created or handled directly by devs. In the draft PR here you can see that the construction of v0 messages has a high level API MessageV0.compile(..) which will build these lookup data structures internally in a valid way:

solana/web3.js/src/message/v0.ts

Lines 71 to 80 in 25d2566

const lookupTableAccounts = args.addressLookupTableAccounts || [];

for (const lookupTable of lookupTableAccounts) {

const extractResult = compiledKeys.extractTableLookup(lookupTable);

if (extractResult !== undefined) {

const [addressTableLookup, loadedAddresses] = extractResult;

addressTableLookups.push(addressTableLookup);

allLoadedAddresses.writable.push(...loadedAddresses.writable);

allLoadedAddresses.readonly.push(...loadedAddresses.readonly);

}

}

solana/web3.js/src/message/compiled-keys.ts

Lines 104 to 123 in 25d2566

extractTableLookup(

lookupTable: AddressLookupTableAccount,

): [MessageAddressTableLookup, LoadedAddresses] | undefined {

const [writableIndexes, drainedWritableKeys] =

this.drainKeysFoundInLookupTable(

lookupTable.state.addresses,

keyMeta =>

!keyMeta.isSigner && !keyMeta.isInvoked && keyMeta.isWritable,

);

const [readonlyIndexes, drainedReadonlyKeys] =

this.drainKeysFoundInLookupTable(

lookupTable.state.addresses,

keyMeta =>

!keyMeta.isSigner && !keyMeta.isInvoked && !keyMeta.isWritable,

);

// Don't extract lookup if no keys were found

if (writableIndexes.length === 0 && readonlyIndexes.length === 0) {

return;

}

Yeah, only thing is that MessageV0 is exported in #27213, so by Murphy's Law someone will definitely eventually create one with hand rolled indices that overlap.

I think that's fine. The same holds for any other field in MessageV0... staticAccountKeys could have dup keys, instructions could reference invalid account indexes, the header values could be contradictory, etc.

steveluscher · 2022-08-25T22:26:02Z

+import {Message} from './legacy';
+import {MessageV0} from './v0';


It's really stressing me out that we're going to have to hard-require the implementation for every version from here to the end of time, but I don't have anything helpful to suggest at this point :)

I wonder if, at some point, we let people create builds (hard forks? 😅) of web3.js that no longer include old transaction implementations, if their use case is to produce new transactions and not to interpret old ones.

That said, the existence of this module definitely encourages hard-requiring the whole universe. What would you think about getting rid of this, and instead offering a lightweight getSerializedMessageVersion() utility? That way the end user can decide if they want to:

Hard require every version and switch between them, or

Fatal/skip if they encounter a version they are not interested in supporting.

I'm totally in favor of adding a utility to get the serialized message version but I think it's important to provide the VersionedMessage.deserialize utility in addition since anyone deserializing an arbitrary message would need this logic anyways. I think it only makes sense to forgo this utility if you don't need to deserialize messages at all and are only concerned with creating/serializing a particular set of transaction versions. I'm trying to modularize the code that's specific to a particular transaction version so I think we're in decent position for that already.

Here's a PR to add that utility function you suggested: #27415

jstarry force-pushed the web3/versioned-transactions branch from 3b212ce to 524b51e Compare August 15, 2022 11:02

jstarry force-pushed the web3/versioned-transactions branch from 524b51e to e2206a9 Compare August 15, 2022 15:14

feat: add support for version 0 transactions

a1250b0

jstarry force-pushed the web3/versioned-transactions branch from e2206a9 to a1250b0 Compare August 15, 2022 19:44

jstarry requested review from jordaaash and steveluscher August 15, 2022 21:24

jordaaash reviewed Aug 17, 2022

View reviewed changes

jstarry added 4 commits August 18, 2022 22:52

chore: feedback

f3419b1

chore: update VersionedMessage type

6818741

chore: use literals for version getter

38eb503

chore: fix lint error

c657363

This was referenced Aug 22, 2022

feat: update Connection to support versioned transactions #27068

Merged

Versioned Transaction and Address Lookup Table feature activation #26391

Closed

chore: switch to VersionedMessage.deserialize

e92391c

jstarry merged commit 1ccfc65 into solana-labs:master Aug 25, 2022

jstarry deleted the web3/versioned-transactions branch August 25, 2022 13:45

steveluscher reviewed Aug 25, 2022

View reviewed changes

steveluscher added this to the web3.js Roadmap – August 2022 milestone Sep 1, 2022

Digiminey mentioned this pull request Oct 11, 2024

[Snyk] Fix for 2 vulnerabilities Digiminey/solana#201

Open

This was referenced Oct 13, 2024

[Snyk] Fix for 1 vulnerabilities adamlaska/solana#300

Open

[Snyk] Fix for 1 vulnerabilities adamlaska/solana#302

Open

Digiminey mentioned this pull request Oct 18, 2024

[Snyk] Fix for 1 vulnerabilities Digiminey/solana#203

Open

This was referenced Oct 20, 2024

[Snyk] Fix for 1 vulnerabilities adamlaska/solana#308

Open

[Snyk] Fix for 1 vulnerabilities adamlaska/solana#309

Open

[Snyk] Security upgrade @solana/web3.js from 1.50.0 to 1.54.1 adamlaska/solana#312

Open

		writableIndexes: Array<number>;
		readonlyIndexes: Array<number>;

	const lookupTableAccounts = args.addressLookupTableAccounts \|\| [];
	for (const lookupTable of lookupTableAccounts) {
	const extractResult = compiledKeys.extractTableLookup(lookupTable);
	if (extractResult !== undefined) {
	const [addressTableLookup, loadedAddresses] = extractResult;
	addressTableLookups.push(addressTableLookup);
	allLoadedAddresses.writable.push(...loadedAddresses.writable);
	allLoadedAddresses.readonly.push(...loadedAddresses.readonly);
	}
	}

	extractTableLookup(
	lookupTable: AddressLookupTableAccount,
	): [MessageAddressTableLookup, LoadedAddresses] \| undefined {
	const [writableIndexes, drainedWritableKeys] =
	this.drainKeysFoundInLookupTable(
	lookupTable.state.addresses,
	keyMeta =>
	!keyMeta.isSigner && !keyMeta.isInvoked && keyMeta.isWritable,
	);
	const [readonlyIndexes, drainedReadonlyKeys] =
	this.drainKeysFoundInLookupTable(
	lookupTable.state.addresses,
	keyMeta =>
	!keyMeta.isSigner && !keyMeta.isInvoked && !keyMeta.isWritable,
	);

	// Don't extract lookup if no keys were found
	if (writableIndexes.length === 0 && readonlyIndexes.length === 0) {
	return;
	}

		import {Message} from './legacy';
		import {MessageV0} from './v0';

Conversation

jstarry commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

codecov Bot commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jordaaash left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jstarry commented Aug 15, 2022 •

edited

Loading

codecov Bot commented Aug 15, 2022 •

edited

Loading

jstarry Aug 26, 2022 •

edited

Loading