Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Commit

Permalink
fix: update equality checks for replacement transactions to prevent e…
Browse files Browse the repository at this point in the history 
…rroneous underpriced errors (#1650)

* add miner.priceBump option

* add error for locked transaction

* fix replacement tx logic

use maxPriorityFeePerGas, maxFeePerGas, and gasPrice to check if a
transaction is an eligible replacement.

fix bug to include replacement transaction EQUAL to price bump

* modify test to include extra fail cases

* update comment

* add back comment

* Fix/improve error message

Co-authored-by: David Murdoch <187813+davidmurdoch@users.noreply.github.com>

* allow string for priceBump option

Co-authored-by: David Murdoch <187813+davidmurdoch@users.noreply.github.com>
  • Loading branch information
@MicaiahReid @davidmurdoch
MicaiahReid and davidmurdoch committed Nov 24, 2021
1 parent 3a2646f commit b892c25
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 19 deletions.
56 changes: 42 additions & 14 deletions src/chains/ethereum/ethereum/src/transaction-pool.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
INTRINSIC_GAS_TOO_LOW,
CodedError,
UNDERPRICED,
REPLACED
REPLACED,
TRANSACTION_LOCKED
} from "@ganache/ethereum-utils";
import { EthereumInternalOptions } from "@ganache/ethereum-options";
import { Executables } from "./miner/executables";
Expand All @@ -27,7 +28,7 @@ import { TypedTransaction } from "@ganache/ethereum-transaction";
function shouldReplace(
replacee: TypedTransaction,
replacerNonce: bigint,
replacerGasPrice: bigint,
replacer: TypedTransaction,
priceBump: bigint
): boolean {
const replaceeNonce = replacee.nonce.toBigInt();
Expand All @@ -36,15 +37,42 @@ function shouldReplace(
return false;
}

const gasPrice = replacee.effectiveGasPrice.toBigInt();
const thisPricePremium = gasPrice + (gasPrice * priceBump) / 100n;
// if the transaction being replaced is in the middle of being mined, we can't
// replpace it so let's back out early
if (replacee.locked) {
throw new CodedError(
TRANSACTION_LOCKED,
JsonRpcErrorCode.TRANSACTION_REJECTED
);
}

// if our replacer's price is `gasPrice * priceBumpPercent` better than our
// replacee's price, we should do the replacement!.
if (!replacee.locked && replacerGasPrice > thisPricePremium) {
return true;
} else {
const replacerTip =
"maxPriorityFeePerGas" in replacer
? replacer.maxPriorityFeePerGas.toBigInt()
: replacer.effectiveGasPrice.toBigInt();
const replacerMaxFee =
"maxFeePerGas" in replacer
? replacer.maxFeePerGas.toBigInt()
: replacer.effectiveGasPrice.toBigInt();
const replaceeTip =
"maxPriorityFeePerGas" in replacee
? replacee.maxPriorityFeePerGas.toBigInt()
: replacee.effectiveGasPrice.toBigInt();
const replaceeMaxFee =
"maxFeePerGas" in replacee
? replacee.maxFeePerGas.toBigInt()
: replacee.effectiveGasPrice.toBigInt();

const tipPremium = replaceeTip + (replaceeTip * priceBump) / 100n;
const maxFeePremium = replaceeMaxFee + (replaceeMaxFee * priceBump) / 100n;

// if both the tip and max fee of the new transaction aren't
// `priceBumpPercent` more than the existing transaction, this transaction is
// underpriced
if (replacerTip < tipPremium || replacerMaxFee < maxFeePremium) {
throw new CodedError(UNDERPRICED, JsonRpcErrorCode.TRANSACTION_REJECTED);
} else {
return true;
}
}

Expand Down Expand Up @@ -84,9 +112,9 @@ export default class TransactionPool extends Emittery.Typed<{}, "drain"> {
#options: EthereumInternalOptions["miner"];

/**
* Minimum price bump percentage to replace an already existing transaction (nonce)
* Minimum price bump percentage needed to replace a transaction that already exists in the transaction pool.
*/
#priceBump: bigint = 10n;
#priceBump: bigint;

#blockchain: Blockchain;
constructor(
Expand All @@ -98,6 +126,7 @@ export default class TransactionPool extends Emittery.Typed<{}, "drain"> {
this.#blockchain = blockchain;
this.#options = options;
this.#origins = origins;
this.#priceBump = options.priceBump;
}
public readonly executables: Executables = {
inProgress: new Set(),
Expand Down Expand Up @@ -183,7 +212,6 @@ export default class TransactionPool extends Emittery.Typed<{}, "drain"> {
let executableOriginTransactions = executables.get(origin);

const priceBump = this.#priceBump;
const newGasPrice = transaction.effectiveGasPrice.toBigInt();
let length: number;
if (
executableOriginTransactions &&
Expand All @@ -197,7 +225,7 @@ export default class TransactionPool extends Emittery.Typed<{}, "drain"> {
// necessarily sorted
for (let i = 0; i < length; i++) {
const pendingTx = pendingArray[i];
if (shouldReplace(pendingTx, txNonce, newGasPrice, priceBump)) {
if (shouldReplace(pendingTx, txNonce, transaction, priceBump)) {
// do an in-place replace without triggering a re-sort because we
// already know where this transaction should go in this "byNonce"
// heap.
Expand Down Expand Up @@ -277,7 +305,7 @@ export default class TransactionPool extends Emittery.Typed<{}, "drain"> {
// necessarily sorted
for (let i = 0; i < length; i++) {
const queuedTx = queuedArray[i];
if (shouldReplace(queuedTx, txNonce, newGasPrice, priceBump)) {
if (shouldReplace(queuedTx, txNonce, transaction, priceBump)) {
// do an in-place replace without triggering a re-sort because we
// already know where this transaction should go in this "byNonce"
// heap.
Expand Down
63 changes: 58 additions & 5 deletions src/chains/ethereum/ethereum/tests/transaction-pool.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ describe("transaction pool", async () => {
let common: Common;
let blockchain: any;
let origins: Map<string, Heap<TypedTransaction>>;
const optionsJson = { wallet: { deterministic: true } };
const priceBump = 10n;
const optionsJson = {
wallet: { deterministic: true },
miner: { priceBump: priceBump }
};
const options = EthereumOptionsConfig.normalize(optionsJson);
let futureNonceRpc, executableRpc: TypedRpcTransaction;
before(function () {
Expand All @@ -44,19 +48,22 @@ describe("transaction pool", async () => {
from: from,
type: "0x2",
maxFeePerGas: "0xffffffff",
maxPriorityFeePerGas: "0xff",
gasLimit: "0xffff"
};
executableRpc = {
from: from,
type: "0x2",
maxFeePerGas: "0xffffffff",
maxPriorityFeePerGas: "0xff",
gasLimit: "0xffff",
nonce: "0x0"
};
futureNonceRpc = {
from: from,
type: "0x2",
maxFeePerGas: "0xffffff",
maxPriorityFeePerGas: "0xff",
gasLimit: "0xffff",
nonce: "0x2"
};
Expand Down Expand Up @@ -165,9 +172,47 @@ describe("transaction pool", async () => {
found.serialized.toString(),
executableTx.serialized.toString()
);
// the second time around, the gas price won't be high enough to replace, so it'll throw

const replacementRpc = JSON.parse(JSON.stringify(executableRpc));
replacementRpc.maxPriorityFeePerGas = "0xffff";
const replacementTx1 = TransactionFactory.fromRpc(replacementRpc, common);
// even if the tip is high enough, the max fee isn't enough to replace, so it'll throw
await assert.rejects(
txPool.prepareTransaction(executableTx, secretKey),
txPool.prepareTransaction(replacementTx1, secretKey),
{
code: -32003,
message: "transaction underpriced"
},
"replacement transaction with insufficient gas price to replace should have been rejected"
);

replacementRpc.maxPriorityFeePerGas = executableRpc.maxPriorityFeePerGas;
replacementRpc.maxFeePerGas = "0xffffffffff";
const replacementTx2 = TransactionFactory.fromRpc(replacementRpc, common);
// even if the maxFee is high enough, the tip isn't enough to replace, so it'll throw
await assert.rejects(
txPool.prepareTransaction(replacementTx2, secretKey),
{
code: -32003,
message: "transaction underpriced"
},
"replacement transaction with insufficient gas price to replace should have been rejected"
);

const legacyReplacementRpc: TypedRpcTransaction = {
from: from,
type: "0x0",
gasPrice: "0xffffffff",
gasLimit: "0xffff",
nonce: "0x0"
};
const replacementTx3 = TransactionFactory.fromRpc(
legacyReplacementRpc,
common
);
// the gasPrice is higher than the tip but lower than the maxFee, which isn't enough, so it'll throw
await assert.rejects(
txPool.prepareTransaction(replacementTx3, secretKey),
{
code: -32003,
message: "transaction underpriced"
Expand Down Expand Up @@ -251,12 +296,19 @@ describe("transaction pool", async () => {
executableTx.serialized.toString()
);

// raise our replacement transaction's prices by exactly the price bump amount
const originalMaxFee = Quantity.from(executableRpc.maxFeePerGas).toBigInt();
const originalTip = Quantity.from(
executableRpc.maxPriorityFeePerGas
).toBigInt();
const maxFeePremium = originalMaxFee + (originalMaxFee * priceBump) / 100n;
const tipPremium = originalTip + (originalTip * priceBump) / 100n;
// our replacement transaction needs to have a sufficiently higher gasPrice
const replacementRpc: TypedRpcTransaction = {
from: from,
type: "0x2",
maxFeePerGas: "0xffffffffff",
maxPriorityFeePerGas: "0xffffffff",
maxFeePerGas: Quantity.from(maxFeePremium).toString(),
maxPriorityFeePerGas: Quantity.from(tipPremium).toString(),
gasLimit: "0xffff",
nonce: "0x0"
};
Expand Down Expand Up @@ -298,6 +350,7 @@ describe("transaction pool", async () => {
from: from,
type: "0x2",
maxFeePerGas: "0xffffffffff",
maxPriorityFeePerGas: "0xffff",
gasLimit: "0xffff",
nonce: "0x2"
};
Expand Down
19 changes: 19 additions & 0 deletions src/chains/ethereum/options/src/miner-options.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,18 @@ export type MinerConfig = {
type: Data;
hasDefault: true;
};

/**
* Minimum price bump percentage needed to replace a transaction that already exists in the transaction pool.
*
* @defaultValue ""
*/
priceBump: {
type: bigint;
rawType: string | number | bigint;
hasDefault: true;
cliType: string;
};
};
};

Expand Down Expand Up @@ -262,5 +274,12 @@ export const MinerOptions: Definitions<MinerConfig> = {
cliDescription: "Set the extraData block header field a miner can include.",
default: () => DATA_EMPTY,
cliType: "string"
},
priceBump: {
normalize: BigInt,
cliDescription:
"Minimum price bump percentage needed to replace a transaction that already exists in the transaction pool.",
default: () => 10n,
cliType: "string"
}
};
6 changes: 6 additions & 0 deletions src/chains/ethereum/utils/src/errors/errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,9 @@ export const VM_EXCEPTION = "VM Exception while processing transaction: ";
*/
export const VM_EXCEPTIONS =
"Multiple VM Exceptions while processing transactions: : \n\n";

/**
* Returned if a replacement transaction is sent while the potentially replaced transaction is being mined.
*/
export const TRANSACTION_LOCKED =
"transaction can't be replaced, mining has already started. (please open an issue with reproduction steps: https://github.com/trufflesuite/ganache/issues/new)";

0 comments on commit b892c25

Please sign in to comment.