fix: nonce-based Transactions no longer mutate the transaction in place when you compileMessage

[steveluscher]

Problem

Right now, compileMessage() is side-effectful. If you start with a nonce-based Transaction instance whose recentBlockhash member is undefined, running compileMessage will ‘fill in’ the recentBlockhash member with the value of nonceInfo.nonce. It will also have the side effect of prepending the AdvanceNonceAccount instruction to instructions.

compileMessage has no right to do this. It should perform reads only. The only thing that can result from this is confusion. Why does a nonce-based transaction have no recent blockhash at one point in time, but does in another? What if recentBlockhash differs from nonceInfo.nonce because someone overwrote it? Which will the developer expect to be used? Why did the size of the instructions list change when I called compileMessage()?

Summary of Changes

• Write tests to cover more of these cases.
• Always use the nonceInfo.nonce as the recent blockhash when compiling messages, even if the first transaction instruction
• Never mutate recentBlockhash or instructions when compiling messages for nonce-based transactions

[steveluscher]

@jstarry, @jordansexton, I came across this when implementing #25303. This is, of course, the kind of thing that I should just just leave well alone, but here we are.

Can I put your heads together to see if there's anything about this PR that would break something, somewhere? Nobody should be relying on the fact that compileMessage mutates Transaction in place, but you never know.

[steveluscher]

Copy this.instructions if you're going to mess with it.

[steveluscher]

Always use the nonce as the recentBlockhash, but only in the scope of the compileMessage function.

[steveluscher]

Bug here. If the nonce advance instruction already happened to be at the head of the list (eg. because someone put it there) we wouldn't end up using the nonce as the recentBlockhash in the Message.

[steveluscher]

The changes in this test represent the prior behavior that I've changed. No longer does simply calling sign() (which in turn calls compileMessage()) mutate the Transaction instance.

[codecov]

Codecov Report

Merging #25829 (d1c934d) into master (8caced6) will decrease coverage by 6.7%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #25829       +/-   ##
===========================================
- Coverage    82.1%    75.4%     -6.8%     
===========================================
  Files         628       40      -588     
  Lines      171471     2347   -169124     
  Branches        0      339      +339     
===========================================
- Hits       140878     1771   -139107     
+ Misses      30593      459    -30134     
- Partials        0      117      +117     

[jstarry]

Looks great!