Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 27, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions web3.js/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,17 +327,24 @@ export class Transaction {
return this._message;
}

const {nonceInfo} = this;
if (nonceInfo && this.instructions[0] != nonceInfo.nonceInstruction) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.recentBlockhash = nonceInfo.nonce;
this.instructions.unshift(nonceInfo.nonceInstruction);
let recentBlockhash;
let instructions: TransactionInstruction[];
if (this.nonceInfo) {
recentBlockhash = this.nonceInfo.nonce;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (this.instructions[0] != this.nonceInfo.nonceInstruction) {
instructions = [this.nonceInfo.nonceInstruction, ...this.instructions];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
instructions = this.instructions;
}
} else {
recentBlockhash = this.recentBlockhash;
instructions = this.instructions;
}
const {recentBlockhash} = this;
if (!recentBlockhash) {
throw new Error('Transaction recentBlockhash required');
}

if (this.instructions.length < 1) {
if (instructions.length < 1) {
console.warn('No instructions provided');
}

Expand All @@ -351,8 +358,8 @@ export class Transaction {
throw new Error('Transaction fee payer required');
}

for (let i = 0; i < this.instructions.length; i++) {
if (this.instructions[i].programId === undefined) {
for (let i = 0; i < instructions.length; i++) {
if (instructions[i].programId === undefined) {
throw new Error(
`Transaction instruction index ${i} has undefined program id`,
);
Expand All @@ -361,7 +368,7 @@ export class Transaction {

const programIds: string[] = [];
const accountMetas: AccountMeta[] = [];
this.instructions.forEach(instruction => {
instructions.forEach(instruction => {
instruction.keys.forEach(accountMeta => {
accountMetas.push({...accountMeta});
});
Expand Down Expand Up @@ -471,7 +478,7 @@ export class Transaction {
});

const accountKeys = signedKeys.concat(unsignedKeys);
const instructions: CompiledInstruction[] = this.instructions.map(
const compiledInstructions: CompiledInstruction[] = instructions.map(
instruction => {
const {data, programId} = instruction;
return {
Expand All @@ -484,7 +491,7 @@ export class Transaction {
},
);

instructions.forEach(instruction => {
compiledInstructions.forEach(instruction => {
invariant(instruction.programIdIndex >= 0);
instruction.accounts.forEach(keyIndex => invariant(keyIndex >= 0));
});
Expand All @@ -497,7 +504,7 @@ export class Transaction {
},
accountKeys,
recentBlockhash,
instructions,
instructions: compiledInstructions,
});
}

Expand Down
151 changes: 136 additions & 15 deletions web3.js/test/transaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,122 @@ describe('Transaction', () => {
expect(message.header.numReadonlySignedAccounts).to.eq(0);
expect(message.header.numReadonlyUnsignedAccounts).to.eq(1);
});

it('uses the nonce as the recent blockhash when compiling nonce-based transactions', () => {
const nonce = new PublicKey(1);
const nonceAuthority = new PublicKey(2);
const nonceInfo = {
nonce: nonce.toBase58(),
nonceInstruction: SystemProgram.nonceAdvance({
noncePubkey: nonce,
authorizedPubkey: nonceAuthority,
}),
};
const transaction = new Transaction({
feePayer: nonceAuthority,
nonceInfo,
});
const message = transaction.compileMessage();
expect(message.recentBlockhash).to.equal(nonce.toBase58());
});

it('prepends the nonce advance instruction when compiling nonce-based transactions', () => {
const nonce = new PublicKey(1);
const nonceAuthority = new PublicKey(2);
const nonceInfo = {
nonce: nonce.toBase58(),
nonceInstruction: SystemProgram.nonceAdvance({
noncePubkey: nonce,
authorizedPubkey: nonceAuthority,
}),
};
const transaction = new Transaction({
feePayer: nonceAuthority,
nonceInfo,
}).add(
SystemProgram.transfer({
fromPubkey: nonceAuthority,
lamports: 1,
toPubkey: new PublicKey(3),
}),
);
const message = transaction.compileMessage();
expect(message.instructions).to.have.length(2);
const expectedNonceAdvanceCompiledInstruction = {
accounts: [1, 4, 0],
data: (() => {
const expectedData = Buffer.alloc(4);
expectedData.writeInt32LE(
4 /* SystemInstruction::AdvanceNonceAccount */,
0,
);
return bs58.encode(expectedData);
})(),
programIdIndex: (() => {
let foundIndex = -1;
message.accountKeys.find((publicKey, ii) => {
if (publicKey.equals(SystemProgram.programId)) {
foundIndex = ii;
return true;
}
});
return foundIndex;
})(),
};
expect(message.instructions[0]).to.deep.equal(
expectedNonceAdvanceCompiledInstruction,
);
});

it('does not prepend the nonce advance instruction when compiling nonce-based transactions if it is already there', () => {
const nonce = new PublicKey(1);
const nonceAuthority = new PublicKey(2);
const nonceInfo = {
nonce: nonce.toBase58(),
nonceInstruction: SystemProgram.nonceAdvance({
noncePubkey: nonce,
authorizedPubkey: nonceAuthority,
}),
};
const transaction = new Transaction({
feePayer: nonceAuthority,
nonceInfo,
})
.add(nonceInfo.nonceInstruction)
.add(
SystemProgram.transfer({
fromPubkey: nonceAuthority,
lamports: 1,
toPubkey: new PublicKey(3),
}),
);
const message = transaction.compileMessage();
expect(message.instructions).to.have.length(2);
const expectedNonceAdvanceCompiledInstruction = {
accounts: [1, 4, 0],
data: (() => {
const expectedData = Buffer.alloc(4);
expectedData.writeInt32LE(
4 /* SystemInstruction::AdvanceNonceAccount */,
0,
);
return bs58.encode(expectedData);
})(),
programIdIndex: (() => {
let foundIndex = -1;
message.accountKeys.find((publicKey, ii) => {
if (publicKey.equals(SystemProgram.programId)) {
foundIndex = ii;
return true;
}
});
return foundIndex;
})(),
};
expect(message.instructions[0]).to.deep.equal(
expectedNonceAdvanceCompiledInstruction,
);
});
});

if (process.env.TEST_LIVE) {
Expand Down Expand Up @@ -455,15 +571,8 @@ describe('Transaction', () => {
);
transferTransaction.sign(account1);

let expectedData = Buffer.alloc(4);
expectedData.writeInt32LE(4, 0);

expect(transferTransaction.instructions).to.have.length(2);
expect(transferTransaction.instructions[0].programId).to.eql(
SystemProgram.programId,
);
expect(transferTransaction.instructions[0].data).to.eql(expectedData);
expect(transferTransaction.recentBlockhash).to.eq(nonce);
expect(transferTransaction.instructions).to.have.length(1);
expect(transferTransaction.recentBlockhash).to.be.undefined;
Comment on lines +574 to +575
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const stakeAccount = Keypair.generate();
const voteAccount = Keypair.generate();
Expand All @@ -476,12 +585,8 @@ describe('Transaction', () => {
);
stakeTransaction.sign(account1);

expect(stakeTransaction.instructions).to.have.length(2);
expect(stakeTransaction.instructions[0].programId).to.eql(
SystemProgram.programId,
);
expect(stakeTransaction.instructions[0].data).to.eql(expectedData);
expect(stakeTransaction.recentBlockhash).to.eq(nonce);
expect(stakeTransaction.instructions).to.have.length(1);
expect(stakeTransaction.recentBlockhash).to.be.undefined;
});

it('parse wire format and serialize', () => {
Expand Down Expand Up @@ -596,6 +701,22 @@ describe('Transaction', () => {
expect(compiledMessage3).not.to.eql(message);
});

it('constructs a transaction with nonce info', () => {
const nonce = new PublicKey(1);
const nonceAuthority = new PublicKey(2);
const nonceInfo = {
nonce: nonce.toBase58(),
nonceInstruction: SystemProgram.nonceAdvance({
noncePubkey: nonce,
authorizedPubkey: nonceAuthority,
}),
};
const transaction = new Transaction({nonceInfo});
expect(transaction.recentBlockhash).to.be.undefined;
expect(transaction.lastValidBlockHeight).to.be.undefined;
expect(transaction.nonceInfo).to.equal(nonceInfo);
});

it('constructs a transaction with last valid block height', () => {
const blockhash = 'EETubP5AKHgjPAhzPAFcb8BAY1hMH639CWCFTqi3hq1k';
const lastValidBlockHeight = 1234;
Expand Down