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

Formatter has a large number of overflow bugs #3227

Closed
usd-yamazaki opened this issue Nov 21, 2019 · 7 comments
Closed

Formatter has a large number of overflow bugs #3227

usd-yamazaki opened this issue Nov 21, 2019 · 7 comments
Labels
1.x 1.0 related issues

Comments

@usd-yamazaki
Copy link

usd-yamazaki commented Nov 21, 2019

outputBlockFormatter currently has a bug where multiple parameters overflow.
For example, the theoretical condition value of block.gasLimit is 0x7fffffffffffffff. Since this exceeds 53 bits that can be handled by javascript Number, it is necessary to use BigNumber.
https://github.com/ethereum/web3.js/blob/ca9869fba7a87087079c24195ebb522b587e148f/packages/web3-core-helpers/src/formatters.js#L263
Incorrect

block.gasLimit = utils.hexToNumber(block.gasLimit);

Correct

block.gasLimit = outputBigNumberFormatter(block.gasLimit);

There are other reports of similar problems.

#1905 block.timestamp
#1960 block.number

Probably the same problem will occur with block.gasUsed.
In addition, similar potential bugs are expected for outputTransactionFormatter, outputTransactionReceiptFormatter, and outputLogFormatter.

@nivida
Copy link
Contributor

nivida commented Nov 21, 2019

Thanks for opening this issue! Those properties are known to us and we will update the handling of them as soon as possible. This would be a bigger breaking change for all of us. Because of this will we do the mentioned improvements at the earliest for the next minor release of web3.js.

@nivida nivida added 1.x 1.0 related issues Bug Addressing a bug labels Nov 21, 2019
This was referenced Nov 21, 2019
@cgewecke
Copy link
Collaborator

@usd-yamazaki

Could you provide more information about your execution context? Why do you require these values be so large / have such precision?

@usd-yamazaki
Copy link
Author

@usd-yamazaki

Could you provide more information about your execution context? Why do you require these values be so large / have such precision?

I continue to execute a lot of gas consuming contracts on the private chain.
Since this process should not be limited by gasLimit, the upper limit value 0x7fffffffffffffff is set for the gasLimit parameter.
For the upper limit of each parameter, refer to https://eips.ethereum.org/EIPS/eip-1985.

  • “Theoretical condition value” was a translation error and was correctly “theoretical upper limit value”.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 26, 2019

@usd-yamazaki Thank you for providing the link to the EIP.

@nivida discussed this in a meeting yesterday and I argued these changes should not be made to the 1.x branch as a bug fix because:

  • all real limits are well within the JS MAX_INT and will remain so indefinitely
  • changing the data format would break a lot of existing code using JS API
  • the change would benefit a smaller number of people who are running forks

One way to address this is for someone to publish an extension lib that serves high performance forks and private chains which Web3 could link to in the docs. @nivida has highlighted the changes that would be necessary in #3234.

An existing example can be seen at jpmorganchase/quorum.js, which supports a nanosecond precision timestamp by applying a web3.extend to eth_getBlockByNumber and rewriting the output block formatter.

@usd-yamazaki What is your opinion of this approach?

@usd-yamazaki
Copy link
Author

@cgewecke
Of course you should avoid breaking existing code.
So the idea of ​​using web3.extend is great.
However, since it is inconsistent with EIP, I think that it should be corrected after 2.x etc.

Another idea could be to optionally choose whether the parameter supports BigNumber.
This is very simple and easy to understand.

  • Example
web3.setBigNumberSupport({
  blockGasLimit: true,
  transactionGas: true,
  receiptBlockNumber: false  // default
});

@cgewecke
Copy link
Collaborator

Thanks @usd-yamazaki, I like the interface you've proposed there. Will think about this further...

@ryanio ryanio mentioned this issue Apr 15, 2020
13 tasks
@cgewecke cgewecke removed the Bug Addressing a bug label May 20, 2020
@cgewecke
Copy link
Collaborator

Closing in favor of #3442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants