Skip to content

Commit

Permalink
fix: short-circuit checks if ignition tran stil in mempool
Browse files Browse the repository at this point in the history 
There was a wrong check on whether the transactions against the last
network interaction are still in the mempool. If at least one
transaction is in the mempool we don't do the further checks that assume
the transacion with that nonce has been either dropped or replaced by
the user.

I have added a test for the failing case of rerun at the command line.

Fixes #506.
  • Loading branch information
@kanej
kanej committed Oct 4, 2023
1 parent d4140dc commit 65b1338
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 15 deletions.
34 changes: 32 additions & 2 deletions packages/core/src/internal/execution/nonce-management/get-nonce-sync-messages.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,29 @@ export async function getNonceSyncMessages(
transactions.map((tx) => jsonRpcClient.getTransaction(tx))
);

if (fetchedTransactions.some((tx) => tx === undefined)) {
// If at least one transaction for the future is still in the mempool,
// we do nothing
if (fetchedTransactions.some((tx) => tx !== undefined)) {
continue;
}

// If we are here, all the previously pending transactions for this
// future were dropped or replaced.

// Case 1: Confirmed transaction with this nonce
// There are more transactions up to the latest block than our nonce,
// that means that one transaction with our nonce was sent and confirmed
//
// Example:
//
// Ignition sends transaction with nonce 5
// It is replaced by the user, with user transaction nonce 5
// The user transaction confirms
// That means there is a block that includes it
// If we look at the latest transaction count, it will be at least 6
if (latestCount > nonce) {
// We know the ignition transaction was replaced, and the replacement
// transaction has at least one confirmation.
// We don't continue until the user's transactions have enough confirmations
if (safeConfirmationsCount <= nonce) {
throw new IgnitionError(ERRORS.EXECUTION.WAITING_FOR_NONCE, {
Expand All @@ -144,6 +161,14 @@ export async function getNonceSyncMessages(
// Case 2: There's a pending transaction with this nonce sent by the user

// We first handle confirmed transactions, that'w why this check is safe here
//
// Example:
//
// Ignition has sent a transaction with nonce 5
// It is replaced by the user, with user transaction nonce 5
// The user transaction is still in the mempool
// The pending count will show as larger than the nonce, and we know
// from the test above that it has not been confirmed
if (pendingCount > nonce) {
throw new IgnitionError(ERRORS.EXECUTION.WAITING_FOR_NONCE, {
sender,
Expand All @@ -153,13 +178,18 @@ export async function getNonceSyncMessages(
}

// Case 3: There's no transaction sent by the user with this nonce, but ours were still dropped

messages.push({
type: JournalMessageType.ONCHAIN_INTERACTION_DROPPED,
futureId: executionStateId,
networkInteractionId,
});
}

// Case 4: the user sent a set of transactions with nonces higher than
// our highest pending nonce.

// TODO if they have enough confirmation we continue, otherwise we throw
// and wait for further confirmations
}

return messages;
Expand Down
174 changes: 161 additions & 13 deletions packages/core/test/execution/nonce-management/get-nonce-sync-messages.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ import {
OnchainInteractionDroppedMessage,
OnchainInteractionReplacedByUserMessage,
} from "../../../src/internal/execution/types/messages";
import { NetworkInteractionType } from "../../../src/internal/execution/types/network-interaction";
import {
NetworkInteractionType,
OnchainInteraction,
} from "../../../src/internal/execution/types/network-interaction";
import { exampleAccounts } from "../../helpers";

const requiredConfirmations = 5;
Expand Down Expand Up @@ -122,15 +125,111 @@ describe("execution - getNonceSyncMessages", () => {
});

describe("second deployment run", () => {
it("should indicate the user replaced the transaction if the transaction's nonce is less than the latest", async () => {
it("should succeed with an in-flight transaction that has been mined between runs", async () => {
// Assuming a fresh chain, and a deployment that was halted on the first
// future and its deploy transaction is included in a block before the
// next run

// The in-flight transaction has been included in a block, so latest includes it
const latestCount = 1;
// Safest is x blocks in the past so is zero
const safestCount = 0;
// There are no pending
const pendingCount = latestCount;
// The nonce of the now complete transaction is 0
const nonce = 0;

const deploymentState =
setupDeploymentStateBasedOnExampleModuleWithOneTranWith(nonce);

const inflightTxHash = (
(
deploymentState.executionStates[
"Example#MyContract"
] as DeploymentExecutionState
).networkInteractions[0] as OnchainInteraction
).transactions[0].hash;

await assertSuccessOnGetNonceSyncResult({
ignitionModule: exampleModule,
deploymentState,
transactionCountEntries: {
[exampleAccounts[1]]: {
pending: pendingCount,
latest: latestCount,
number: () => safestCount,
},
},
// We are saying that the inflight transaction is now included in a block
getTransaction: (txHash: string) => {
if (txHash !== inflightTxHash) {
throw new Error(
`Mock getTransaction was not expecting the getTransaction request for: ${txHash}`
);
}

return { _kind: "FAKE_TRANSACTION" };
},
});
});

it("should succeed with an in-flight transaction that has not been mined between runs but is in the mempool", async () => {
// Assuming a fresh chain, and a deployment that was halted on the first
// future and its deploy transaction is in the mempool but not mined
// at the start of the second run

// The in-flight transaction has been included in a block, so latest includes it
const latestCount = 0;
// Safest is x blocks in the past so is zero
const safestCount = 0;
// There are no pending
const pendingCount = latestCount + 1;
// The nonce of the now complete transaction is 0
const nonce = 0;

const deploymentState =
setupDeploymentStateBasedOnExampleModuleWithOneTranWith(nonce);

const inflightTxHash = (
(
deploymentState.executionStates[
"Example#MyContract"
] as DeploymentExecutionState
).networkInteractions[0] as OnchainInteraction
).transactions[0].hash;

await assertSuccessOnGetNonceSyncResult({
ignitionModule: exampleModule,
deploymentState,
transactionCountEntries: {
[exampleAccounts[1]]: {
pending: pendingCount,
latest: latestCount,
number: () => safestCount,
},
},
// We are saying that the inflight transaction is still in the mempool
getTransaction: (txHash: string) => {
if (txHash !== inflightTxHash) {
throw new Error(
`Mock getTransaction was not expecting the getTransaction request for: ${txHash}`
);
}

return { _kind: "FAKE_TRANSACTION" };
},
});
});

it("should indicate the user replaced the transaction and the user transaction is safely confirmed", async () => {
// Set latest to be an arbitrary nonce
const latestCount = 30;
// Set safest to be the same as latest, it is not relevant
const safestCount = latestCount;
// There are no pending transactions
const pendingCount = latestCount;
// Set the nonce to be less than latest, indicating it was replaced
const nonce = latestCount - 2;
const nonce = latestCount - 1;

await assertGetNonceSyncResult(
{
Expand All @@ -155,13 +254,40 @@ describe("execution - getNonceSyncMessages", () => {
);
});

it("should error if the user has sent a non-ignition pending transaction that has not confirmed on the account", async () => {
it("should throw if the user replaced the transaction and the user transaction is mined but has not yet fully confirmed", async () => {
// set an arbitrary nonce
const nonce = 16;
// put the latest as bigger than the nonce being checked
const latest = nonce + 1;
// there are no pending
const pending = latest;
// the safest is behind the nonce
const safest = nonce - 1;

await assertGetNonceSyncThrows(
{
ignitionModule: exampleModule,
deploymentState:
setupDeploymentStateBasedOnExampleModuleWithOneTranWith(nonce),
transactionCountEntries: {
[exampleAccounts[1]]: {
pending,
latest,
number: () => safest,
},
},
},
`IGN404: You have sent transactions from 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC with nonce 16. Please wait until they get 5 confirmations before running Ignition again.`
);
});

it("should error if the user replace the transaction and the user transaction is in the mempool but not mined (pending)", async () => {
// Set latest to an arbitary nonce
const latestCount = 30;
// Safe is the same as latest
const safestCount = latestCount;
// Set the nonce to be larger than latest
const nonce = latestCount + 1;
// Set the nonce to be the latest (nonce's are ids not cardinalities)
const nonce = latestCount;
// Set pending larger than the nonce
const pendingCount = nonce + 1;

Expand All @@ -178,19 +304,21 @@ describe("execution - getNonceSyncMessages", () => {
},
},
},
`IGN404: You have sent transactions from ${exampleAccounts[1]} with nonce 31. Please wait until they get 5 confirmations before running Ignition again.`
`IGN404: You have sent transactions from ${exampleAccounts[1]} with nonce 30. Please wait until they get 5 confirmations before running Ignition again.`
);
});

it("should indicate the transaction was dropped if the nonce is higher than the latest", async () => {
it("should indicate if the ignition transaction was dropped from mempool (no user interference)", async () => {
// Set an arbitary latest
const latestCount = 30;
// The safest is exactly the same as latest
const safestCount = 40;
// Pending isn't relevant so is set to latest
const pendingCount = latestCount;
// Set the nonce higher than latest, indicating it was dropped
const nonce = latestCount + 1;
// Set the nonce to latest (note nonce is not a cardinality),
// indicating it was dropped as latest/pending should be larger
// than the nonce
const nonce = latestCount;

await assertGetNonceSyncResult(
{
Expand Down Expand Up @@ -262,6 +390,7 @@ async function assertGetNonceSyncThrows(
number: (num: number) => number;
};
};
getTransaction?: (txHash: string) => any;
},
errorMessage: string
) {
Expand All @@ -278,6 +407,7 @@ async function assertSuccessOnGetNonceSyncResult(ctx: {
number: (num: number) => number;
};
};
getTransaction?: (txHash: string) => any;
}) {
return assertGetNonceSyncResult(ctx, []);
}
Expand All @@ -287,6 +417,7 @@ async function assertGetNonceSyncResult(
ignitionModule,
deploymentState = deploymentStateReducer(),
transactionCountEntries = {},
getTransaction,
}: {
ignitionModule: IgnitionModule<
string,
Expand All @@ -301,14 +432,16 @@ async function assertGetNonceSyncResult(
number: (num: number) => number;
};
};
getTransaction?: (txHash: string) => any;
},
expectedResult: Array<
OnchainInteractionReplacedByUserMessage | OnchainInteractionDroppedMessage
>
) {
const mockJsonRpcClient = setupJsonRpcClient(
latestBlock,
transactionCountEntries
transactionCountEntries,
getTransaction
);

const result = await getNonceSyncMessages(
Expand All @@ -331,12 +464,20 @@ function setupJsonRpcClient(
latest: number;
number: (num: number) => number;
};
}
},
getTransaction?: (txHash: string) => any
): JsonRpcClient {
const mockJsonRpcClient = {
getLatestBlock: () => {
return { number: latestBlockNum };
},
getTransaction: (txHash: string) => {
if (getTransaction !== undefined) {
return getTransaction(txHash);
}

return undefined; // Inflight transactions are never found by default
},
getTransactionCount: (
address: string,
blockTag: "pending" | "latest" | number
Expand Down Expand Up @@ -396,7 +537,14 @@ function setupDeploymentStateBasedOnExampleModuleWithOneTranWith(
to: undefined,
data: "0x",
value: BigInt(0),
transactions: [],
transactions: [
{
hash: "0x123",
fees: {
gasPrice: BigInt(10000),
},
},
],
shouldBeResent: false,
nonce,
},
Expand Down

0 comments on commit 65b1338

Please sign in to comment.