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

Need to throw error when methods are called at future blocks #18254

Closed
danfinlay opened this issue Dec 6, 2018 · 19 comments
Closed

Need to throw error when methods are called at future blocks #18254

danfinlay opened this issue Dec 6, 2018 · 19 comments
Assignees
Milestone

Comments

@danfinlay
Copy link

danfinlay commented Dec 6, 2018

System information

Geth version: 1.8.15 (Infura)
OS & Version: Linux
Commit hash :

Expected behaviour

Geth should throw an error when making a request on a future block.

Actual behaviour

Many types of request simply return null or 0x when the block parameter is an integer for a future unknown block.

This is especially troublesome for clients that hit load-balanced geth clusters like Infura, where one request for latest block could return one number, and then suddenly requests at that block height could start returning null and 0x, resulting in inconsistent client app state.

In short: This is the cause of many bugs in MetaMask and Dapps building on MetaMask, and probably other similar clients as well, and there is no way for us to detect or work around this issue without a fix in Geth.

Steps to reproduce the behaviour

This bug can be reproduced with many methods that we're aware of, and probably many more are possible.

getStorageAt of a known contract (mkr) on a future block:

curl 'https://mainnet.infura.io/' -H 'pragma: no-cache' -H 'origin: null' -H 'accept-encoding: gzip, deflate, br' -H 'accept-language: en,fr;q=0.9,en-US;q=0.8,es;q=0.7' -H 'user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36' -H 'content-type: application/json' -H 'accept: application/json' -H 'cache-control: no-cache' -H 'authority: mainnet.infura.io' --data-binary '{"jsonrpc":"2.0","id":914762274,"method":"eth_getStorageAt","params":["0x9f8f72aa9304c8b593d555f12ef6589cc3a579a2","0x0","0x962124"]}' --compressed

Returns {"jsonrpc":"2.0","id":914762274,"result":"0x"}, as if the contract were never published.

eth_getTransactionCount of an account with some mined transactions, on a future block:

curl 'https://mainnet.infura.io/' -H 'pragma: no-cache' -H 'origin: null' -H 'accept-encoding: gzip, deflate, br' -H 'accept-language: en,fr;q=0.9,en-US;q=0.8,es;q=0.7' -H 'user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36' -H 'content-type: application/json' -H 'accept: application/json' -H 'cache-control: no-cache' -H 'authority: mainnet.infura.io' --data-binary '{"id":6972925150872393,"jsonrpc":"2.0","params":["0xfdea65c8e26263f6d9a1b5de9555d2931a33b825","0x885ac5"],"method":"eth_getTransactionCount"}' --compressed

Returns {"jsonrpc":"2.0","id":6972925150872393,"result":null}, as if there were no error. At least this value can possibly be detected as invalid.

eth_getBalance on an account with some ether, on a future block:

curl 'https://mainnet.infura.io/' -H 'pragma: no-cache' -H 'origin: null' -H 'accept-encoding: gzip, deflate, br' -H 'accept-language: en,fr;q=0.9,en-US;q=0.8,es;q=0.7' -H 'user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36' -H 'content-type: application/json' -H 'accept: application/json' -H 'cache-control: no-cache' -H 'authority: mainnet.infura.io' --data-binary '{"id":2366105135518150,"jsonrpc":"2.0","params":["0xfdea65c8e26263f6d9a1b5de9555d2931a33b825","0x885ab5"],"method":"eth_getBalance"}' --compressed

Returns {"jsonrpc":"2.0","id":2366105135518150,"result":null}, as if there were no error. At least this value can possibly be detected as invalid.

Related to #3483.

@danfinlay danfinlay changed the title Need to throw error when getStorage is called at future block Need to throw error when methods are called at future blocks Dec 6, 2018
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 0.404 ETH (39.03 USD @ $96.62/ETH) attached to it as part of the MetaMask fund.

@gitcoinbot
Copy link

💰 A crowdfund contribution worth 20.00000 DAI (20.0 USD @ $1.0/DAI) has been attached to this funded issue from @.💰

Want to chip in also? Add your own contribution here.

@giltotherescue
Copy link

Thank you Dan for the excellent writeup of the problem. We pitched in on the Gitcoin.

@ryanschneider
Copy link
Contributor

As I mentioned at ethereum/EIPs#1474 (comment) I'd be happy to work on PRs to address issues like this if a standard on the response is finalized (which is why I raised it as part of that EIP discussion since I think the clients should agree on the error returned).

@danfinlay
Copy link
Author

@ryanschneider That proposal in its current state is simply a reformatting of the existing wiki, but has become overloaded with a number of changes to the provider.

Because this is an urgent issue that is breaking many dapp dev experiences, I think we should definitely implement a fix before waiting for all methods to be "finalized" (whatever that would mean).

@xiaods
Copy link

xiaods commented Dec 14, 2018

i have reach the code below

## ethclient/ethclient.go
## 335:    err := ec.c.CallContext(ctx, &result, "eth_getBalance", account, toBlockNumArg(blockNumber))                                                                             

@danfinlay can you guide me to indicate where is rpc api implement code. new comer to go-ethereum codebase. could you do me favor?

@danfinlay
Copy link
Author

@xiaods I am not actually a regular contributor (nor do I know any go), or I'd submit the change myself.

@xiaods
Copy link

xiaods commented Dec 18, 2018

@danfinlay got it. let me have a try.

@xiaods
Copy link

xiaods commented Jan 6, 2019

sorry for the late active response. i will follow this issue now. i will update it asap. 😄

@danfinlay
Copy link
Author

This issue has a much larger impact than I think anyone is appreciating.

Here's another confused person today:
https://twitter.com/Jami_eCrypto/status/1082323704279908352

@xiaods
Copy link

xiaods commented Jan 8, 2019

@danfinlay i found currently the project missing many more testing for this apis. i can't easily to handle the logic. so i try to add more testing to reproduce the bug, the we can easily to fix it asap.

@xiaods
Copy link

xiaods commented Jan 15, 2019

i have merged #18346 to my patch, it look good first.

@xiaods
Copy link

xiaods commented Feb 1, 2019

any update?

xiaods added a commit to xiaods/go-ethereum that referenced this issue Feb 1, 2019
…ture blocks

-[x] add unit test for StorageAt rpc call
-[x] Return error message on future block
-[x] add TransactionCount rpc call unit testing

Signed-off-by: Deshi Xiao <xiaods@gmail.com>
@xiaods
Copy link

xiaods commented Feb 10, 2019

@adamschmideg any update on it?

@fjl
Copy link
Contributor

fjl commented Mar 5, 2019

Fixed in #18346

@fjl fjl added this to the 1.9.0 milestone Mar 5, 2019
@xiaods
Copy link

xiaods commented Mar 10, 2019

@fjl got it, see this like promise patch

@holiman
Copy link
Contributor

holiman commented Apr 9, 2019

Not sure how @gitcoinbot works, but the bounty for the #18346 PR should go to @shibli , not me

@holiman
Copy link
Contributor

holiman commented May 3, 2019

This is now implemented, credits to @shibli

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 0.404 ETH (59.81 USD @ $148.06/ETH) attached to this issue has been cancelled by the bounty submitter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants