Skip to content
This repository was archived by the owner on Apr 11, 2021. It is now read-only.

Hotfix - estimate gas#13

Closed
K-Ho wants to merge 3 commits intomasterfrom
Hotfix/EstimateGas
Closed

Hotfix - estimate gas#13
K-Ho wants to merge 3 commits intomasterfrom
Hotfix/EstimateGas

Conversation

@K-Ho
Copy link
Copy Markdown
Contributor

@K-Ho K-Ho commented Aug 18, 2020

This PR is to intercept calls to eth_estimateGas and instead return a hardcoded value equal to the Gas Limit. This will drastically decrease load on our Geth node, and solves a bug where eth_estimateGas was reverting because it was going over the block gas limit (we do still need to figure out what was causing this).

Question - does this have any negative downstream effects wrt fees or gas metering?

Comment thread core/vm/state_manager.go
}
copy(methodID[:], input[:4])
ret, err = methodIds[methodID].smFunction(evm, contract, input)
ret, err = methodIds[methodID](evm, contract, input)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since methodID is dynamic, this feels a bit safer:

if method, ok := methodIds[methodID]; ok {
    ret, err = method(evm, contract, input)
    return ret, err
}

return nil, errors.New("Method not found")

I don't love the way that the error case of "method not found" isn't returned immediately. To fix this we would check !ok in the if clause but then method isn't in context outside of the if block.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You could define method above the check so that it is in scope afterwards. What do you think?

Comment thread rpc/handler.go
var result interface{}
var err error
if msg.Method == "eth_estimateGas" {
result = 0xffffffff //Gas Limit
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this may make sense to have in a global variable someplace

Comment thread core/vm/evm.go
return ret, err
ret, err := callStateManager(input, evm, contract)
if err != nil {
log.Error("State manager error!", err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missing the Error key here before the err. It looks like in this case it'll append nil and the formatting will look weird.

See:

if len(ctx)%2 != 0 {

@tynes
Copy link
Copy Markdown
Collaborator

tynes commented Aug 19, 2020

This looks like it closes #12 as well

@tynes
Copy link
Copy Markdown
Collaborator

tynes commented Aug 19, 2020

This also looks like it breaks a test:

--- FAIL: TestGenerateBlockAndImportClique (1.02s)
    worker_test.go:247: failed to insert new mined block:2, error:unknown ancestor

Should be investigated before being ready for merge. Does your fix to eth_estimateGas depend on #12? If not and you need the fix in to progress, we could break this into 2 PRs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants