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 3, 2023
1 parent 27c9b30 commit 41e103f
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 8 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
112 changes: 106 additions & 6 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,6 +125,54 @@ describe("execution - getNonceSyncMessages", () => {
});

describe("second deployment run", () => {
it("should succeed with an in-flight transaction but no user transactions", 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 indicate the user replaced the transaction if the transaction's nonce is less than the latest", async () => {
// Set latest to be an arbitrary nonce
const latestCount = 30;
Expand Down Expand Up @@ -155,6 +206,33 @@ describe("execution - getNonceSyncMessages", () => {
);
});

it("should throw if the user replaced the transaction (nonce less than latest) but it is not yet safely 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 has sent a non-ignition pending transaction that has not confirmed on the account", async () => {
// Set latest to an arbitary nonce
const latestCount = 30;
Expand Down Expand Up @@ -189,8 +267,10 @@ describe("execution - getNonceSyncMessages", () => {
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 higher than latest (note nonce is not a cardinality,
// when comparing to count nonce has to be incremented), indicating it
// was dropped
const nonce = latestCount + 3;

await assertGetNonceSyncResult(
{
Expand Down Expand Up @@ -262,6 +342,7 @@ async function assertGetNonceSyncThrows(
number: (num: number) => number;
};
};
getTransaction?: (txHash: string) => any;
},
errorMessage: string
) {
Expand All @@ -278,6 +359,7 @@ async function assertSuccessOnGetNonceSyncResult(ctx: {
number: (num: number) => number;
};
};
getTransaction?: (txHash: string) => any;
}) {
return assertGetNonceSyncResult(ctx, []);
}
Expand All @@ -287,6 +369,7 @@ async function assertGetNonceSyncResult(
ignitionModule,
deploymentState = deploymentStateReducer(),
transactionCountEntries = {},
getTransaction,
}: {
ignitionModule: IgnitionModule<
string,
Expand All @@ -301,14 +384,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 +416,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 +489,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 41e103f

Please sign in to comment.