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

fix: use adjusted time on estimate gas when latest block is being used #4287

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: use adjusted time on estimate gas when latest block is being used #4287

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
57 changes: 20 additions & 37 deletions src/chains/ethereum/ethereum/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,9 +909,6 @@ export default class EthereumApi implements Api {
blockNumber: QUANTITY | Ethereum.Tag = Tag.latest
): Promise<Quantity> {
const blockchain = this.#blockchain;
const blocks = blockchain.blocks;
const parentBlock = await blocks.get(blockNumber);
const parentHeader = parentBlock.header;
const options = this.#options;

const generateVM = async () => {
Expand All @@ -924,38 +921,26 @@ export default class EthereumApi implements Api {
);
return vm;
};
const { coinbase } = blockchain;
const tx = TransactionFactory.fromRpc(
transaction as Transaction,
blockchain.common
);
if (tx.from == null) {
tx.from = coinbase;
}
if (tx.gas.isNull()) {
// eth_estimateGas isn't subject to regular transaction gas limits
tx.gas = options.miner.callGasLimit;
}
const block = await blockchain.gasEstimateBlock(tx, blockNumber);
const runArgs: EstimateGasRunArgs = {
tx: tx.toVmTransaction(),
block,
skipBalance: true,
skipNonce: true
};
return new Promise((resolve, reject) => {
const { coinbase } = blockchain;
const tx = TransactionFactory.fromRpc(
transaction as Transaction,
blockchain.common
);
if (tx.from == null) {
tx.from = coinbase;
}
if (tx.gas.isNull()) {
// eth_estimateGas isn't subject to regular transaction gas limits
tx.gas = options.miner.callGasLimit;
}

const block = new RuntimeBlock(
Quantity.from((parentHeader.number.toBigInt() || 0n) + 1n),
parentHeader.parentHash,
new Address(parentHeader.miner.toBuffer()),
tx.gas,
parentHeader.gasUsed,
parentHeader.timestamp,
options.miner.difficulty,
parentHeader.totalDifficulty,
blockchain.getMixHash(parentHeader.parentHash.toBuffer()),
0n // no baseFeePerGas for estimates
);
const runArgs: EstimateGasRunArgs = {
tx: tx.toVmTransaction(),
block,
skipBalance: true,
skipNonce: true
};
estimateGas(
generateVM,
runArgs,
Expand Down Expand Up @@ -1274,9 +1259,7 @@ export default class EthereumApi implements Api {
@assertArgLength(1)
async eth_getBlockTransactionCountByHash(hash: DATA) {
const { blocks } = this.#blockchain;
const block = await blocks
.getByHash(hash)
.catch<Block>(_ => null);
const block = await blocks.getByHash(hash).catch<Block>(_ => null);
if (!block) return null;
const transactions = block.getTransactions();
return Quantity.from(transactions.length);
Expand Down
44 changes: 42 additions & 2 deletions src/chains/ethereum/ethereum/src/blockchain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import {
StructLog,
TraceTransactionOptions,
EthereumRawAccount,
TraceTransactionResult
TraceTransactionResult,
QUANTITY,
Tag
} from "@ganache/ethereum-utils";
import type { InterpreterStep } from "@ethereumjs/evm";
import { decode } from "@ganache/rlp";
Expand Down Expand Up @@ -49,7 +51,10 @@ import {
calculateIntrinsicGas,
InternalTransactionReceipt,
VmTransaction,
TypedTransaction
TypedTransaction,
LegacyTransaction,
EIP2930AccessListTransaction,
EIP1559FeeMarketTransaction
} from "@ganache/ethereum-transaction";
import { Block, RuntimeBlock, Snapshots } from "@ganache/ethereum-block";
import {
Expand Down Expand Up @@ -77,6 +82,7 @@ import { dumpTrieStorageDetails } from "./helpers/storage-range-at";
import { GanacheStateManager } from "./state-manager";
import { TrieDB } from "./trie-db";
import { Trie } from "@ethereumjs/trie";
import { Ethereum } from "./api-types";

const mclInitPromise = mcl.init(mcl.BLS12_381).then(() => {
mcl.setMapToMode(mcl.IRTF); // set the right map mode; otherwise mapToG2 will return wrong values.
Expand Down Expand Up @@ -577,6 +583,40 @@ export default class Blockchain extends Emittery<BlockchainTypedEvents> {

coinbase: Address;

gasEstimateBlock = async (
Copy link
Contributor

@jeffsmale90 jeffsmale90 Mar 13, 2023

Choose a reason for hiding this comment

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

it'd be nice if this were a verb, ie createEstimateGasBlock or something.

I haven't the time to look into this further today, but I wonder if there's some value in combining the readyNextBlock and this function, as they are very similar (it might be that it just becomes too complicated to be worthwhile though).

tx:
| LegacyTransaction
| EIP2930AccessListTransaction
| EIP1559FeeMarketTransaction,
blockNumber: QUANTITY | Ethereum.Tag = Tag.latest
Copy link
Contributor

@jeffsmale90 jeffsmale90 Mar 13, 2023

Choose a reason for hiding this comment

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

QUANTITY type shouldn't be used inside of the application logic - it's an alias type that's used to describe parameters in the API layer. Instead, accept type Quantity here, and do the conversion in the caller.

) => {
const previousBlock = await this.blocks.get(blockNumber);
const previousHeader = previousBlock.header;
const options = this.#options;

let timestamp = previousHeader.timestamp;
if (blockNumber === "latest")
timestamp = Quantity.from(this.#adjustedTime(previousHeader.timestamp));
Comment on lines +597 to +599
Copy link
Contributor

@jeffsmale90 jeffsmale90 Mar 13, 2023

Choose a reason for hiding this comment

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

Seems to me that if the request is for "latest", this should be incrementing the timestamp, otherwise we use the timestamp of the previous block, rather than the "next" block.


let gas = tx.gas;
if (gas.isNull())
// eth_estimateGas isn't subject to regular transaction gas limits
gas = options.miner.callGasLimit;

return new RuntimeBlock(
Quantity.from((previousHeader.number.toBigInt() || 0n) + 1n),
previousHeader.parentHash,
new Address(previousHeader.miner.toBuffer()),
gas,
previousHeader.gasUsed,
timestamp,
this.isPostMerge ? Quantity.Zero : options.miner.difficulty,
previousHeader.totalDifficulty,
this.getMixHash(previousHeader.parentHash.toBuffer()),
0n // no baseFeePerGas for estimates
);
};

#readyNextBlock = (previousBlock: Block, timestamp?: number) => {
const previousHeader = previousBlock.header;
const previousNumber = previousHeader.number.toBigInt() || 0n;
Expand Down