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

eth, les: add error when accessing missing block state #18346

Merged
merged 4 commits into from
May 2, 2019

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 20, 2018

Supersedes #15762 . It was not properly goimport-formatted, so I fixed that and refactored it a bit more. Thanks @shbli for the PR!


For release notes:

This change makes getBalance, getCode, getStorageAt, getProof, call, getTransactionCount return an error if the block number in the request doesn't exist. getHeaderByNumber still returns null for missing headers.

@holiman holiman changed the title Pr15762 Return error message on missing block Dec 20, 2018
@holiman holiman mentioned this pull request Dec 20, 2018
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@karalabe
Copy link
Member

karalabe commented Jan 3, 2019

This might break the RPC APIs. I did a similar refactor a while back and it borked a lot of higher level client libraries which relied on missing blocks returning null.

@ryanschneider
Copy link
Contributor

BTW, there is more recent discussion at #18254 since #3483 had been marked deprecated.

@karalabe do you remember which client libraries were impacted? We could look at them and see if a) they still make that assumption and b) any work-arounds that could be implemented.

Ideally we can come up with a clever way forward that allows for an explicit error without breaking these clients too badly.

@xiaods
Copy link

xiaods commented Jan 8, 2019

cloud you please add testing to stub the case. the project missing many testing , please don't miss it anymore.

@danfinlay
Copy link

This might break the RPC APIs. I did a similar refactor a while back and it borked a lot of higher level client libraries which relied on missing blocks returning null.

I agree that API breaking should be a last resort, and maybe querying the future block can retain its current behavior. The concern raised in #18254 is more general than this method: There is currently inconsistent behavior across different methods when querying future blocks, and some of those error responses are indistinguishable from successful responses, which should be the subject of possible breaking changes here.

A good example is the case of calling getCode for an address at a future block. Returning null or 0x in this case is indistinguishable from a contract that has not been published yet, meaning there is no correct way for client software to interpret this behavior, and so breaking current code relying on these flawed responses will universally lead to improvements in that client software.

While returning null in response to a future block request might be acceptable as it's arguably true (there is no block for that number known), there is really no substitute for errors being thrown on a variety of other methods requested at the future block, as presented in issue #18254.

For that reason I hope this PR's concerns aren't blocking a more general "default to error when querying an unknown block" solution which would only be breaking things that are already broken, for the sake of fixing them.

@xiaods
Copy link

xiaods commented Jan 17, 2019

any update? i don't clear howto in process doing on this patch?

@fjl
Copy link
Contributor

fjl commented Mar 5, 2019

This impacts: getBalance, getCode, getStorageAt, getProof, call, getTransactionCount.

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Please change StateAndHeaderByNumber in les/api_backend.go to include the same treatment of header == nil.

@holiman holiman requested a review from zsfelfoldi as a code owner March 5, 2019 11:20
@holiman
Copy link
Contributor Author

holiman commented Mar 5, 2019

done, ptal @fjl

@holiman
Copy link
Contributor Author

holiman commented May 2, 2019

ping @fjl to add tests to this one, and merge it in

@fjl fjl changed the title Return error message on missing block eth, les: add error when accessing missing block state May 2, 2019
@fjl
Copy link
Contributor

fjl commented May 2, 2019

rebased and tests added

@fjl fjl merged commit 5036992 into ethereum:master May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants