fix: transactions populated from RPC requests retain original account key order

[jordaaash]

Problem

Transaction.populate is kind of broken (see #21722). It's not so much that the implementation of populate itself is broken, but rather that attempting to reserialize a transaction that was deserialized from a message is conceptually invalid.

Summary of Changes

This PR attempts to fix the issue in a surgical and slightly hacky fashion, without adding to or changing the public API, or breaking correctly behaving applications.

This is accomplished by setting an internal Message property if the Transaction was created with populate. If this Message exists, it will be used instead of recompiling the message from the transaction, which is what causes the account key order to change.

Instructions cannot be added to a populated transaction. This will throw an error at runtime. Since correctly behaving applications should not add instructions to a compiled message, I regard this as non-breaking. YMMV. If there are concerns about this, it could be limited to populated transactions with non-null signature values.

I haven't added getters/setters for feePayer, recentBlockhash, nonceInfo, or instructions. If added, these could throw errors if these are set on a populated transaction. I wanted to first propose these changes and gather feedback.

Fixes #21722
Closes #23684 (duplicate, but also adds to the public API which I don't think we should do for this)

[t-nelson]

I think this gets the job done. Kinda sucks carrying the duplicate data around, but given the rest of the inefficiencies in the lib, probably a small price to pay for compatibility

[jstarry]

Current fix looks great!

I haven't added getters/setters for feePayer, recentBlockhash, nonceInfo, or instructions. If added, these could throw errors if these are set on a populated transaction. I wanted to first propose these changes and gather feedback.

What if we cache the fields which affect message compilation (feePayer, recentBlockhash, nonceInfo, and instructions) as _populatedMessageFields and then check in compileMessage if any of them have been modified. If modifications are detected, throw an error. Otherwise return the cached _message that you added.

[jordaaash]

What if we cache the fields which affect message compilation (feePayer, recentBlockhash, nonceInfo, and instructions) as _populatedMessageFields and then check in compileMessage if any of them have been modified. If modifications are detected, throw an error. Otherwise return the cached _message that you added.

Hmm, we could do that. My instinctual preference is to fail fast if someone tries to modify a Transaction that shouldn't be modified rather than wait until it's serialized. But throwing errors inside of setters is kind of gross on multiple levels.

@steveluscher thoughts?

[t-nelson]

But throwing errors inside of setters is kind of gross on multiple levels.

That was what motivated me to suggest not being able to deser into Transaction at all, but some new type that doesn't allow modifications other than addSignature

[steveluscher]

That was what motivated me to suggest not being able to deser into Transaction at all, but some new type that doesn't allow modifications other than addSignature

I've only thought about this for three seconds, but this strikes me as a worthwhile project. You'd have to upgrade all consumers of Transaction to understand and handle this new ImmutableTransaction or CompiledTransaction type (as @t-nelson said… compatibility vs…).

If you did it this way, TypeScript will prevent you from trying to modify such a transaction, saving you from having to find out at runtime, which is nice.

More work though.

[jstarry]

Cool, I think we all agree that overhauling the transaction class is the way to go but this PR is still a good incremental fix. I'm fine with it as is.

[codecov]

Codecov Report

Merging #23720 (c81f047) into master (2da4e3e) will decrease coverage by 11.7%.
The diff coverage is n/a.

❗ Current head c81f047 differs from pull request most recent head de58333. Consider uploading reports for the commit de58333 to get more accurate results

@@             Coverage Diff             @@
##           master   #23720       +/-   ##
===========================================
- Coverage    81.8%    70.0%    -11.8%     
===========================================
  Files         581       36      -545     
  Lines      158312     2255   -156057     
  Branches        0      322      +322     
===========================================
- Hits       129518     1580   -127938     
+ Misses      28794      560    -28234     
- Partials        0      115      +115     

[jordaaash]

What if we cache the fields which affect message compilation (feePayer, recentBlockhash, nonceInfo, and instructions)

@steveluscher @jstarry this has been done. Implementation is pretty ugly, but I'm not sure how to make it cleaner and still check everything.

[steveluscher]

LGTM! Since there are so many throws you might consider doing something like…

function fail(): never {
  throw new Error(...);
}

if (transaction.foo !== originalTransaction.foo) {
  fail();
}

…but ¯\_(ツ)_/¯

[jstarry]

What if we cache the fields which affect message compilation (feePayer, recentBlockhash, nonceInfo, and instructions)

@steveluscher @jstarry this has been done. Implementation is pretty ugly, but I'm not sure how to make it cleaner and still check everything.

I think I came up with something that would be cleaner here: jordaaash#1. What do you think? (cc @steveluscher to take a look)

[steveluscher]

I think I came up with something that would be cleaner here…

Nice; yeah, at one point I had thought about hashing properties of the transaction to be able to detect modifications, but then I forgot.

You could even use one of these ‘stable stringify’ implementations and just throw a giant bundle of properties at it. https://www.npmjs.com/search?q=stable%20stringify

Pluses: easy way to get a unique identifier
Minuses: have to carry around giant strings in memory, JSON.stringify is no picnic performance-wise

Either way, if any material property ever gets added to Transaction and we forget to add it to this check, we're back to square one. That's probably an argument to go for the solution based on two different types of transaction.

[jstarry]

I believe we will deprecate this Transaction type and this hacky fix will be temporary until we migrate to a better Transaction type. I don't think this needs to be particularly performant or conscious of memory usage because a transaction itself can only be 1232 bytes of data and any stringified properties are therefore limited in size as well.

Pull request has been modified.

[jordaaash]

Haha that approach is so much cleaner. I merged your stringify change, though I'm not sure offhand how the objects, especially TransactionInstruction, are going to get stringified. I'll do some testing.

[steveluscher]

If this need to stringify objects in a way that's stable – regardless of whether keys are in different orders – ever comes up again, this is one of my favs.

https://github.com/facebook/relay/blob/ec03bf455d28b7eb30e5251731bf768eac2c2abd/packages/relay-runtime/store/OperationExecutor.js#L1668-L1670

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[jstarry]

@Mergifyio rebase

[mergify]

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/13)
Rebasing (2/13)
Rebasing (3/13)
Rebasing (4/13)
Rebasing (5/13)
Rebasing (6/13)
Auto-merging core/src/accounts_hash_verifier.rs
CONFLICT (content): Merge conflict in core/src/accounts_hash_verifier.rs
Auto-merging runtime/src/accounts_db.rs
CONFLICT (content): Merge conflict in runtime/src/accounts_db.rs
Auto-merging runtime/src/accounts_hash.rs
CONFLICT (content): Merge conflict in runtime/src/accounts_hash.rs
error: could not apply a6e703691... add hash calc config.use_write_cache (#24005)
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply a6e703691... add hash calc config.use_write_cache (#24005)

err-code: 8C36A

[jstarry]

@jordansexton do you mind rebasing and merging this when you get the chance?

Pull request has been modified.

[jordaaash]

@jstarry unfortunately there's a failing test which surfaced this issue:

solana/web3.js/src/connection.ts

Lines 3897 to 3905 in 6c5a3ca

	transaction = Transaction.populate(transactionOrMessage);
	}
	if (transaction.nonceInfo && signers) {
	transaction.sign(...signers);
	} else {
	let disableCache = this._disableBlockhashCaching;
	for (;;) {
	transaction.recentBlockhash = await this._recentBlockhash(disableCache);

Transaction simulation from a Message relies on modifying the transaction after populating it. If I merge this change as is, it will break transaction simulation.

I did something very hacky and unset the internal properties in this instance to keep the current behavior of simulateTransaction:

de58333

If tests pass now, I'll merge away.