fix: use latest block for eth_fillTransaction gas estimation#1138
fix: use latest block for eth_fillTransaction gas estimation#1138luchenqun wants to merge 11 commits into
Conversation
|
@swift1337 the crux is in func (b *Backend) getHeightByBlockNum(ctx context.Context, blockNum rpctypes.BlockNumber) (height int64, err error) {
ctx, span := tracer.Start(ctx, "getHeightByBlockNum", trace.WithAttributes(attribute.Int64("blockNum", blockNum.Int64())))
defer func() { evmtrace.EndSpanErr(span, err) }()
if blockNum == rpctypes.EthEarliestBlockNumber {
status, err := b.ClientCtx.Client.Status(ctx)
if err != nil {
return 0, errors.New("failed to get earliest block height")
}
return status.SyncInfo.EarliestBlockHeight, nil
}
height = blockNum.Int64()
if height <= 0 {
// In cometBFT, LatestBlockNumber, FinalizedBlockNumber, SafeBlockNumber all map to the latest block height.The important detail is that In this codebase, when // Int64 converts block number to primitive type
func (bn BlockNumber) Int64() int64 {
if bn < 0 {
return 0
} else if bn == 0 {
return 1
}
return int64(bn)
}That means the old flow is effectively:
So the comment in This is also easy to verify in practice: if you deploy a contract after block 1 and then call |
|
@greptile review |
vladjdk
left a comment
There was a problem hiding this comment.
We should add a test for the intended behaviour here to prevent future regressions.
Greptile SummaryFixes a bug in Confidence Score: 5/5Safe to merge — targeted one-line bug fix with no regressions expected. The change is a single-line, well-understood bug fix. The old code resolved block 1 due to a quirk in No files require special attention. Important Files Changed
|
aljo242
left a comment
There was a problem hiding this comment.
fix is correct, but two issues:
ci has not run: all workflows are action_required -- fork pr, needs maintainer approval to trigger.
test doesn't pin the fix: the "estimate gas with latest block" case mocks EstimateGas with mock.AnythingOfType("*types.EthCallRequest"), which matches any request regardless of block number. the specific regression (block 1 vs latest) isn't asserted and could be reintroduced without breaking the test. assert that the BlockNumber on the EthCallRequest is EthLatestBlockNumber.
Summary
EthLatestBlockNumberinstead of numeric0wheneth_fillTransactionestimates gaseth_fillTransactionaligned with the latest state instead of block 1Why
SetTxDefaultscurrently builds aBlockNumberOrHashwithNewBlockNumber(big.NewInt(0))before callingEstimateGas.That is not equivalent to
latestin this codebase:BlockNumberFromCometreturns the numeric block number unchangedBlockNumber.Int64()maps numeric0to1CometHeaderByNumberresolves block 1, not the latest blockUsing
EthLatestBlockNumbermakes the call match the intended latest-state behavior.Verification
go test ./rpc/backend -run TestDoesNotExist -count=0Related: #1120