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.

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

Eip 7709 implement gas costs #7983

Open
wants to merge 1 commit into
base: verkle
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.hyperledger.besu.ethereum.mainnet.ClearEmptyAccountStrategy.NotClearEmptyAccount;
import org.hyperledger.besu.ethereum.mainnet.ProtocolSpecBuilder.BlockValidatorBuilder;
import org.hyperledger.besu.ethereum.mainnet.blockhash.CancunBlockHashProcessor;
import org.hyperledger.besu.ethereum.mainnet.blockhash.Eip7709BlockHashProcessor;
import org.hyperledger.besu.ethereum.mainnet.blockhash.FrontierBlockHashProcessor;
import org.hyperledger.besu.ethereum.mainnet.blockhash.PragueBlockHashProcessor;
import org.hyperledger.besu.ethereum.mainnet.feemarket.BaseFeeMarket;
Expand Down Expand Up @@ -991,7 +992,7 @@ static ProtocolSpecBuilder verkleDefinition(
CoinbaseFeePriceCalculator.eip1559()))
.withdrawalsProcessor(new WithdrawalsProcessor(clearEmptyAccountStrategy))
.executionWitnessValidator(new ExecutionWitnessValidator.AllowedExecutionWitness())
.blockHashProcessor(new PragueBlockHashProcessor())
.blockHashProcessor(new Eip7709BlockHashProcessor())
.blockHeaderFunctions(new VerkleDevnetBlockHeaderFunctions())
.name("Verkle");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
public class Eip7709BlockHashLookup implements BlockHashLookup {
private static final Logger LOG = LoggerFactory.getLogger(Eip7709BlockHashLookup.class);
private static final long BLOCKHASH_SERVE_WINDOW = 256L;
private static final long WARM_STORAGE_READ_COST = 100L;

private final Address contractAddress;
private final long historyServeWindow;
Expand Down Expand Up @@ -81,6 +82,12 @@ public Hash apply(final MessageFrame frame, final Long blockNumber) {
return ZERO;
}

final long cost = cost(frame, blockNumber);
if (frame.getRemainingGas() < cost) {
return Hash.EMPTY;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it in the spec ? I see that in the first failure we return ZERO and in this one EMPTY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not in the spec but this will not surface to consensus because it will be handled properly in BlockHashOperation. This was the only way I found to stop execution if out of gas. Could have an exception if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

For precompiles execeptional halts (like this one, out of gas) are handled by returning null rather than Hash.EMPTY, which is as valid a response as any other hash.

}
frame.decrementRemainingGas(cost);

final Hash cachedHash = hashByNumber.get(blockNumber);
if (cachedHash != null) {
return cachedHash;
Expand All @@ -105,4 +112,19 @@ public Hash apply(final MessageFrame frame, final Long blockNumber) {
hashByNumber.put(blockNumber, blockHash);
return blockHash;
}

private long cost(final MessageFrame frame, final long blockNumber) {
final UInt256 key = UInt256.valueOf(blockNumber % BLOCKHASH_SERVE_WINDOW);
long gas = frame.getAccessWitness().touchAndChargeStorageLoad(contractAddress, key);

if (gas == 0) {
return getWarmStorageReadCost();
}

return gas;
}

protected long getWarmStorageReadCost() {
return WARM_STORAGE_READ_COST;
}
}
2 changes: 1 addition & 1 deletion ethereum/referencetests/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ dependencies {
}
verkleRefTestImplemention dependencies.create('ethereum:execution-spec-tests') {
version {
strictly '[email protected].8'
strictly '[email protected].9-alpha-1'
}
artifact {
name = 'fixtures'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,18 @@ public OperationResult execute(final MessageFrame frame, final EVM evm) {
return new OperationResult(cost, null);
}

final long remainingGas = frame.getRemainingGas();
final BlockHashLookup blockHashLookup = frame.getBlockHashLookup();
final Hash blockHash = blockHashLookup.apply(frame, blockArg.toLong());
final long lookupCost = remainingGas - frame.getRemainingGas();
if (Hash.EMPTY.equals(blockHash)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems quite strange to use Hash.EMPTY as a way to determine if there is not enough gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend null, it matches the way halts are detected in precompiles.

return new OperationResult(lookupCost, ExceptionalHaltReason.INSUFFICIENT_GAS);
}
// give lookupCost back as it will be taken after
frame.incrementRemainingGas(lookupCost);
frame.pushStackItem(blockHash);

return new OperationResult(cost, null);
return new OperationResult(cost + lookupCost, null);
}

/**
Expand Down
Loading