fix: fixed system-coder tests

[tiago18c]

issue: These tests were failing due to attempt at advancing the nonce account twice in the same slot.

fix: waiting one slot for the tests that advance the nonce more than once

[vercel]

@tiago18c is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[swaroop-osec]

lgtm

I approved this after reviewing the code, but the workflow tests are still failing, even though they pass locally with Surfpool.

[tiago18c]

Yeah, this also fixed things locally, but still failing CI. I think its still somewhat related to surfpool and recent blockhashes sysvar. Will figure something out

[acheroncrypto]

This test was added ~4 years ago (#1920), and it worked until last week. It even continued to work after the surfpool changes (#4210), but as mentioned in #4190 (comment), we download whatever comes from that S3 link rather than installing the specified version.

The test failures are clearly a result of that new binary, so it would be much better to let the maintainers of surfpool know about this bug/inconsistency than to make changes to our test in order to make the surfpool validator happy.

FWIW, I also asked about why we needed to make changes to our tests in the initial PR (#4106 (comment)), but the author did not respond.

[tiago18c]

Was about to add that this is definitely on surfpool. Previously was running the tests locally with 1.0.0-rc1 and it worked, tried the binaries from s3 and it didn't work. Also found a surfpool PR related to nonces that only made it to 1.0.0

[MicaiahReid]

I'm not yet convinced that this is a bug with surfpool, but I am investigating! But unless I am misunderstanding the mechanics of the nonce advance instruction, this is a bug with the existing test that the solana-test-validator (and previous versions of surfpool) would let slide, but is now not allowed with latest surfpool (due to the fix in this surfpool PR).

My understanding of a transaction with an advance nonce account instruction is that it must:

1. Have the nonce advance instruction be first in the set of instructions
2. The transaction's recent blockhash is replaced with the nonce account's current nonce

If I make a slight modification to the it("Advances a nonce account" test to add some logging:

    const nonceAccountBefore = await program.account.nonce.fetch(
      nonceKeypair.publicKey,
    );
    console.log("Nonce Before:", nonceAccountBefore.nonce.toString());
    const recentBlockhashBefore =
      await program.provider.connection.getLatestBlockhash();
    console.log("Recent Blockhash Before:", recentBlockhashBefore.blockhash);
    // These have to be separate to make sure advance is in another slot.
    const sig = await program.methods
      .advanceNonceAccount(provider.wallet.publicKey)
      .accounts({
        nonce: nonceKeypair.publicKey,
        recentBlockhashes: SYSVAR_RECENT_BLOCKHASHES_PUBKEY,
      })
      .rpc();
    // get transaction from sig
    const tx = await program.provider.connection.getParsedTransaction(sig, {
      commitment: "confirmed",
    });
    console.log("Advance Nonce Tx:", tx);

I can see the following logs:

Nonce Before: 6ByNUHSHPHhcy3kG4VSFk7bHdLGMQqVxrvoFb9WDNeFJ
Recent Blockhash Before: HYx5U8uk4DRe8w4KDjKreDptFmicHaZfXfAhbdvWJ7hG
Advance Nonce Tx: {
  blockTime: 1770055091,
  meta: {
    computeUnitsConsumed: 150,
    costUnits: 1487,
    err: null,
    fee: 5000,
    innerInstructions: [],
    logMessages: [
      'Program 11111111111111111111111111111111 invoke [1]',
      'Program 11111111111111111111111111111111 success'
    ],
    postBalances: [ 499999998067416700, 1447680, 1, 42706560 ],
    postTokenBalances: [],
    preBalances: [ 499999998067421700, 1447680, 1, 42706560 ],
    preTokenBalances: [],
    rewards: [],
    status: { Ok: null },
    loadedAddresses: undefined
  },
  slot: 1606,
  transaction: {
    message: {
      accountKeys: [Array],
      instructions: [Array],
      recentBlockhash: 'HYx5U8uk4DRe8w4KDjKreDptFmicHaZfXfAhbdvWJ7hG',
      addressTableLookups: undefined
    },
    signatures: [
      '32mAyAWNnkkGQLpHgLDyypYWh5bLtmQRqqVyYYbMk1W6r7qPfVNUD8e1Xh7pcbA4ky7JnjZkqCE4wEFYhUoY1gmk'
    ]
  },
  version: undefined
}

The transaction's blockhash is using the recent blockhash from the network, rather than the nonce. When run against the solana test validator and previous versions of surfpool, this was errantly allowed by the network. Surfpool fixes this bug to properly validate blockhashes for nonce-incrementing transactions, so the transaction is correctly rejected with Blockhash not found.

The .rpc() method when calling anchor programs does not (as far as I can tell) detect if the transaction is incrementing the nonce, and is just blindly overriding the recent blockhash field with a hash from the network.

[tiago18c]

We both have the same understanding of transaction nonces and its requirements.

That's why I started this PR by correcting the behavior of that same test to use the web3.js Transaction API to use the expected workflow https://github.com/solana-foundation/anchor/pull/4210/changes#diff-10a9dd5b8ca19a3f54a069cfe3c432b749c6b9c81ce13cf634bfdc790cc9ca95R304-R324 yet it still fails on the supposed correct version of surfpool (did not fix the workflow on all tests).

[MicaiahReid]

We both have the same understanding of transaction nonces and its requirements.

That's why I started this PR by correcting the behavior of that same test to use the web3.js Transaction API to use the expected workflow https://github.com/solana-foundation/anchor/pull/4210/changes#diff-10a9dd5b8ca19a3f54a069cfe3c432b749c6b9c81ce13cf634bfdc790cc9ca95R304-R324 yet it still fails on the supposed correct version of surfpool (did not fix the workflow on all tests).

Okay, glad to confirm our understanding! It looks like sendAndConfirm, if you look at the anchor source code, is overwriting the recentBlockhash you're setting:

  async sendAndConfirm(
    tx: Transaction | VersionedTransaction,
    signers?: Signer[],
    opts?: ConfirmOptionsWithBlockhash,
  ): Promise<TransactionSignature> {
    if (opts === undefined) {
      opts = this.opts;
    }
    if (isVersionedTransaction(tx)) {
      if (signers) {
        tx.sign(signers);
      }
    } else {
      tx.feePayer = tx.feePayer ?? this.wallet.publicKey;
      tx.recentBlockhash = (
        await this.connection.getLatestBlockhash(opts.preflightCommitment)
      ).blockhash;

      if (signers) {
        for (const signer of signers) {
          tx.partialSign(signer);
        }
      }
    }
...

So the transaction being processed by surfpool does not have a valid nonce as the blockhash.

Note, if we update sendAndConfirm to only update blockhash if missing:

      if (!tx.recentBlockhash) {
        tx.recentBlockhash = (
          await this.connection.getLatestBlockhash(opts.preflightCommitment)
        ).blockhash;
      }

The tests pass as expected

[tiago18c]

Thanks for the find @MicaiahReid , I wasn't aware of this behavior in sendAndConfirm