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.

Sign up for GitHub

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

Jump to bottom

QGB Use Ecrecover to validate Ethereum signatures #605

Merged
merged 5 commits into from
Aug 17, 2022
Merged

QGB Use Ecrecover to validate Ethereum signatures #605

merged 5 commits into from
Aug 17, 2022

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Aug 10, 2022 •
edited
Loading

Updates the implementation of the Ethereum signer.

Contributes to #604

@rach-id rach-id added bug Something isn't working C: QGB labels Aug 10, 2022
@rach-id rach-id self-assigned this Aug 10, 2022
@rach-id rach-id changed the base branch from main to qgb-integration August 10, 2022 17:54
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2022 •
edited
Loading

Codecov Report

Merging #605 (17a1463) into qgb-integration (2fef6fd) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

@@                 Coverage Diff                 @@
##           qgb-integration     #605      +/-   ##
===================================================
- Coverage            10.96%   10.93%   -0.04%     
===================================================
  Files                   57       57              
  Lines                 9240     9240              
===================================================
- Hits                  1013     1010       -3     
- Misses                8125     8127       +2     
- Partials               102      103       +1
Impacted Files Coverage Δ
x/qgb/types/ethereum_signer.go 60.00% <50.00%> (-10.00%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rach-id
Copy link
Member Author

rach-id commented Aug 10, 2022

reverting after 179 attestation. Converting to draft.

@rach-id rach-id marked this pull request as draft August 10, 2022 18:45
@evan-forbes
Copy link
Member

we still might want to merge this even if it isn't the (sole) source of the issue described in #604. there's no rush tho, we can wait until we figure it out.

@rach-id rach-id marked this pull request as ready for review August 12, 2022 14:24
@rach-id
Copy link
Member Author

rach-id commented Aug 12, 2022

The next issue found is that the commitments queried from Celestia-core are not deterministic. Will investigate that separately.
Aside from that, we need the changes we have in this PR. So, we can merge if approved.

rootulp
rootulp reviewed Aug 12, 2022
View reviewed changes
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

For my learning: what was wrong with the previous implementation?
Related: Do we want a unit test that prevents a future EthAddressFromSignature regression?

x/qgb/types/ethereum_signer.go Outdated Show resolved Hide resolved
x/qgb/types/ethereum_signer.go
Comment on lines -43 to -44
if signature[64] == 27 || signature[64] == 28 {
signature[64] -= 27
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why is this no longer necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://twitter.com/alexberegszaszi/status/1534461421454606336?t=6hUf4_GjhEuFBVdj_Kqzew&s=19

Copy link
Collaborator

Choose a reason for hiding this comment

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

wow so a bunch of smart contracts include this check even though it's unnecessary

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. And also in our case we were adding this check while it's not needed.

@rach-id
Copy link
Member Author

rach-id commented Aug 12, 2022 •
edited
Loading

@rootulp Nothing is wrong with the implementation, it was alright. I was suspecting it to be the reason why the relayer was failing. But, turned out to be the commitments.
However, I wanted to be the same as what we are using on the smart contract side:

https://github.com/celestiaorg/quantum-gravity-bridge/blob/37b613843c7a0681fe2a8ae82d3422e1f1cfb311/src/QuantumGravityBridge.sol#L140-L148

Also, I don't think we need a test because the two implementations give the same results.

Edit: changed the title of the PR as it was misleading

@rach-id @rootulp
Update x/qgb/types/ethereum_signer.go
17a1463 
Co-authored-by: Rootul Patel <rootulp@gmail.com>
@rach-id rach-id changed the title QGB Ethereum signature fix QGB Use Ecrecover to validate Ethereum signatures Aug 12, 2022
@rootulp
Copy link
Collaborator

rootulp commented Aug 12, 2022 •
edited
Loading

Nothing is wrong with the implementation, it was alright

Thanks for explaining! So no changes were strictly needed in x/qgb/types/ethereum_signer_test.go? On first pass, I thought the implementation output different results b/c the changed values in the test.

@rach-id
Copy link
Member Author

rach-id commented Aug 12, 2022

@rootulp The different values in tests are because we removed the signatures whos v is either 27 or 28.
The reason is that we no longer need to support that legacy change:
https://ethereum.stackexchange.com/a/113505/65649

adlerjohn
adlerjohn approved these changes Aug 13, 2022
View reviewed changes
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

evan-forbes
evan-forbes approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

👍

@rach-id
Copy link
Member Author

rach-id commented Aug 16, 2022

Will not merge until I get to the bottom of the commitments issue.

@rach-id rach-id merged commit 0ee5a45 into celestiaorg:qgb-integration Aug 17, 2022
@rach-id rach-id deleted the relayer_signature_fix branch August 17, 2022 11:00
rach-id added a commit to rach-id/celestia-app that referenced this pull request Aug 17, 2022
@rach-id
QGB Use Ecrecover to validate Ethereum signatures (celestiaorg#605)
ea1619b 
* updates ethereum signer implementation

* update test for correct signature

* go.sum

* Update x/qgb/types/ethereum_signer.go

* Update x/qgb/types/ethereum_signer.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rootulp rootulp rootulp left review comments

@adlerjohn adlerjohn adlerjohn approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

Assignees

@rach-id rach-id

Labels
bug Something isn't working
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate the QGB relayer signatures issue
5 participants
@rach-id @codecov-commenter @evan-forbes @rootulp @adlerjohn