-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
…aying on optimism)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a fair amount I don't understand in this PR, left a load of questions. Since there are so many, happy to go over in a call if that's easier.
function useGasOrdinaryGasUsed(uint8 size) public pure returns (uint256) { | ||
require(size != 0, "Size zero works differently due to zero-byte"); | ||
|
||
return 21629 + 11100 * uint256(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do 21629
& 11100
refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are just determined experimentally to fit the formula to the data. They're just the two constants involved in describing a straight line.
function useGas(uint8 size) external { | ||
require(size != 0, "Size zero works differently due to zero-byte"); | ||
|
||
uint256 iterations = 100 * uint256(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we multiply size by 100
?
Edit: can see your commit Scale up useGas by 10x (need lots of gas to be significant on L2)
. I'm still unsure on why 100
was chosen - was it arbitrary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some constraints on this number but the specific value is arbitrary yeah. For the sake of accuracy we need to create significant variation, and I limit it to uint8
to minimize the complications caused by zeros in the calldata.
(eg, if uint16
was used, you'd get a discontinuity when transitioning from 256 to 257.)
uint256 result = 21064 + 16 * nonZeroDataLen; | ||
|
||
if (nonZeroDataLen >= 4) { | ||
// I think this is because a different branch is used if the data | ||
// might match a method id. Less than 4 bytes and it can't match. | ||
result += 78; | ||
} | ||
|
||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this logic? Not sure I'm following
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solidity generates bytecode to inspect the method id. This creates different code paths for when you have less than 4 bytes, since that disqualifies it from being a method id without having to look at the data. The difference between these code paths is 78 gas. (At least that's my expectation, the point is that there is a discontinuity of 78 gas which needs to be accounted for.)
21,064 is the amount of gas used when the length of the data is zero.
16 gas is charged for each (non-zero) byte thereafter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the following error running this script using mainnet (works for arbitrum and optimism):
TypeError: Cannot read properties of null (reading 'baseFeePerGas')
command: yarn hardhat run --network mainnet scripts/estimateMedianBasefee.ts
Is it meant to work for mainnet or just L2s?
console.log({ size }); | ||
|
||
const balanceBefore = await ethers.provider.getBalance(await signer.getAddress(), referenceBlock); | ||
const ordinaryGas = await feeMeasurer.useGasOrdinaryGasUsed(size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "ordinary gas" mean?
ordinaryGasPrice = | ||
(lastResult.weiUsed - firstResult.weiUsed) / | ||
(lastResult.ordinaryGas - firstResult.ordinaryGas); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this calculation?
const weiUsedRelErrors = results.map((result) => { | ||
const prediction = (firstResult.weiUsed | ||
+ ordinaryGasPrice * (result.ordinaryGas - firstResult.ordinaryGas)); | ||
|
||
return Number(result.weiUsed - prediction) / Number(result.weiUsed); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this calculation?
const baselineExtraWei = firstResult.weiUsedExtra; | ||
const extraWeiPerByte = (lastResult.weiUsedExtra - firstResult.weiUsedExtra) / (lastResult.size - firstResult.size); | ||
|
||
const weiUsedRelErrors = results.map((result) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation for weiUsedRelErrors
here looks slightly different to the first one used when measuring ordinary gas price, why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running this script (yarn hardhat run scripts/measure.ts
), I'm getting some negative values in the terminal, is this expected? e.g.
{
ordinaryGasPrice: -6008632713n,
weiUsedRelErrors: [
0,
-11.948463340552308,
-4.502138945036356,
-1.7304120609794545e-9
]
}
...
weiUsedForOrdinaryGas: -126565839466632n,
weiUsedExtra: 134225179488512n,
ethUsed: '0.00000765934002188',
ethUsedForOrdinaryGas: '-0.000126565839466632',
Merging per discussion with @jacque006. |
What is this PR doing?
Adds
tools/fee-measurer
. SeeREADME.md
.How can these changes be manually tested?
Follow instructions from readme.
Does this PR resolve or contribute to any issues?
Contributes to #178.
Checklist
Guidelines
resolve conversation
button is for reviewers, not authors