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

Ecrecover index error #357

Merged
merged 3 commits into from Jul 19, 2019

Conversation

@kdeme
Copy link
Contributor

commented Jul 19, 2019

When syncing Nimbus there was a segfault occuring (release mode) at block number 1352922.

The problem was invalid access to data[63], in getSignature used by ecRecover.
The data provided there does not necessarily have 64 bytes, and can in fact be completely empty.

The reason as to why this happens at block 1352922, is because it is the first TX to 0x0...01 (ecrecover precompile) with empty input data, see:
https://etherscan.io/txs?a=0x0000000000000000000000000000000000000001&p=166
https://etherscan.io/tx/0x4127eb13b273120f81267673dc89a158cae570215eb06e795ab0636ee7f7f015

The first commit fixes this. The second commit is a rework of the code in hope to make it more clear + do certain actions in a more reasonable order.

Additionally, this bug would only be noticed in release mode, without much debug data to work from.
IndexError would be raised in debug mode, but quickly catched by the catch all. The last commit should make debug mode also crash on this type of errors. This could create some new crashes, I haven't seen any yet but I have not yet tested to sync again from start.

I did run the general state tests, and have same results, skipping the 2 known failed and slow tests (slow tests seem to not give reliable results, with or without this change)

kdeme added some commits Jul 19, 2019

@zah zah closed this Jul 19, 2019

@zah zah reopened this Jul 19, 2019

@zah

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Something went wrong for the CI. Closed/reopened to retrigger it.

@zah

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

The CI fails with:

nimbus.nim(143, 19) Error: undeclared identifier: 'peerGauge'

Probably something is wrong with the pinned modules.

@kdeme

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Yes, I see nim-metrics / nim-eth got bumped, but #352 isn't merged yet.

@zah zah merged commit 1a3a29c into devel Jul 19, 2019

0 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

@delete-merged-branch delete-merged-branch bot deleted the ecrecover-index-error branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.