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

Problem: no stateful precompiled contract for relayer #1014

Merged
merged 8 commits into from
Sep 11, 2023

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Apr 28, 2023

Updates: cosmos/relayer#1187

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

@mmsqe mmsqe force-pushed the ref_relayer branch 2 times, most recently from 75631b9 to bd7ae10 Compare April 28, 2023 02:51
@thomas-nguy
Copy link
Collaborator

I guess this PR is to be able to test the ibc middleware that will convert cosmos msgs to ethermint msgs?

would it be possible to write an ADR for the design? @yihuang

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

I guess this PR itself is just try go-relayer in integration test, so it'll be more convenient for @mmsqe to further experiment using the golang relayer

@thomas-nguy
Copy link
Collaborator

thomas-nguy commented Apr 28, 2023

yes I understand that this PR is only to integrate with the go-relayer but I just using this opportunity to raise the need for an ADR before we are getting too deep in the implementation so that we can have a better visibility on what has to be done
(not related to the PR itself though)

@thomas-nguy
Copy link
Collaborator

thomas-nguy commented Apr 28, 2023

Ideally the IBC protocol is relayer agnostic so using hermes or gorelayer implementation should not matter unless we change the semantic for cronos which may raise some concerns (which could be discussed in the ADR)

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

Ideally the IBC protocol is relayer agnostic so using hermes or gorelayer implementation should not matter unless we change the semantic for cronos which may raise some concerns (which could be discussed in the ADR)

@mmsqe need to patch the relayer to let it send evm tx instead of cosmos tx.

@mmsqe
Copy link
Collaborator Author

mmsqe commented Apr 28, 2023

Ideally the IBC protocol is relayer agnostic so using hermes or gorelayer implementation should not matter unless we change the semantic for cronos which may raise some concerns (which could be discussed in the ADR)

@mmsqe need to patch the relayer to let it send evm tx instead of cosmos tx.

sorry that I am not so familiar with rust.

@thomas-nguy
Copy link
Collaborator

thomas-nguy commented Apr 28, 2023

@mmsqe need to patch the relayer to let it send evm tx instead of cosmos tx.

Could it be done without changing the relayer implementation?

something like

Relayer send cosmos txs -> Cronos middleware catch the cosmos txs and wrap it to an evm txs using module private key

in a general way, if the concern is to generate the receipt for cosmos txs, I think execution is not needed but we could refactor ethermint (https://github.com/evmos/ethermint/blob/main/x/evm/keeper/msg_server.go#L92) to be able to generate more easily receipts out of an eth txs

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

@mmsqe need to patch the relayer to let it send evm tx instead of cosmos tx.

Could it be done without changing the relayer implementation?

something like

Relayer send cosmos txs -> Cronos middleware catch the cosmos txs and wrap it in an evm txs using module private key

can you elaborate? by middleware do you mean a rpc service? it can convert the original tx to an evm tx, but can't re-sign the evm tx using the relayer's key, right?

But we might can develop a standalone convert and resign cli, which can runs together with relayer and access the same keystore, so we don't have to modify rust and golang version twice. but still need to make some changes to the relayer.

in a general way, if the concern is to generate the receipt for cosmos txs, I think execution is not needed but we could refactor ethermint (https://github.com/evmos/ethermint/blob/main/x/evm/keeper/msg_server.go#L92) to be able to generate more easily receipts out of an eth txs

there are many things to emulate: tx list in eth_getBlock, tx body in eth_getTransaction, and in the end, we can't emulate the v,r,s signature in tx body.

@thomas-nguy
Copy link
Collaborator

thomas-nguy commented Apr 28, 2023

can you elaborate? by middleware do you mean a rpc service? it can convert the original tx to an evm tx, but can't re-sign the evm tx using the relayer's key, right?

I mean ibc middleware, or it can even be done in the ante handler if we want to generalise to all cosmos txs

Yes, the evm txs will be signed by an arbitrary key (module key in genesis config) not the relayer key
but I think it is not too important (maybe?) because :

  • In case of ibc, what trigger the "change of state" is the relayer, not the original ibc sender and in most of the case, we don't care about the relayer identity (the from field would be irrelevant in most explorer)
  • If we need to get the relayer address, we can add an ethereum event that will specify it so that it will still be possible to get the original cosmos signer from the explorer, as well as the original ibc sender if needed
there are many things to emulate: tx list in eth_getBlock, tx body in eth_getTransaction, and in the end, we can't emulate the v,r,s signature in tx body.

I think we can wrap the cosmos txs in the evm txs like putting the cosmos txs encoded in the call data
v,r,s signature in tx body will be generated by the module private key

But in some case, we may need to also have the ability to generate "custom" ethereum events which make a bit complex.

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

I mean ibc middleware

do you mean broadcast a new tx in the ibc middleware? that is part of the state machine.

@thomas-nguy
Copy link
Collaborator

do you mean broadcast a new tx in the ibc middleware? that is part of the state machine.

One step is to refactor ethermint to make it easier to generate ethereum receipt.

I understand that it is a bit complicated now because its all across many different place. But from my understanding the ethereum receipt information is stored in the cosmos events in many places and reconstructed in the rpc endpoints.

It may be possible to refactor the code to have an utility function to generate receipt.
(its basically the same as executing ethmsg except that we can ignore the applymsg part)

but maybe there is more complexity (?), I havent look too deep into the code

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

do you mean broadcast a new tx in the ibc middleware? that is part of the state machine.

One step is to refactor ethermint to make it easier to generate ethereum receipt.

I understand that it is a bit complicated now because its all across many different place. But from my understanding the ethereum receipt information is stored in the cosmos events in many places and reconstructed in the rpc endpoints.

It may be possible to refactor the code to have an utility function to generate receipt. (its basically the same as executing ethmsg except that we can ignore the applymsg part)

but maybe there is more complexity (?), I havent look too deep into the code

even if we emulated the events, there are still some difficulties, for example getBlock filter evm txs by msg type and result code, now it need to look into the event list of every tx, and in the end we still can't make the signature of the emulated tx.

@thomas-nguy
Copy link
Collaborator

thomas-nguy commented Apr 28, 2023

another possible flow is

cosmos tx -> middleware convert to eth tx signed with module private key -> execute the eth tx as a specific precompile (we may be able to bypass signature verification check and set direction From to original cosmos sender)

at the end we can get the correct tx result code and gas used in the receipt.

but not sure if its possible to change to bypass the signature verification without any risks?

@yihuang
Copy link
Collaborator

yihuang commented Apr 28, 2023

another possible flow is

cosmos tx -> middleware convert to eth tx signed with module private key -> execute the eth tx as a specific precompile (we may be able to bypass signature verification check and set direction From to original cosmos sender)

at the end we can get the correct tx result code and gas used in the receipt.

but not sure if its possible to change to bypass the signature verification without any risks?

I guess ibc incentivization feature rely on correct signer of the tx.

@mmsqe mmsqe changed the title Problem: golang relayer is not supported Problem: no stateful precompiled contract for relayer May 8, 2023
@mmsqe mmsqe force-pushed the ref_relayer branch 2 times, most recently from 0ed2e95 to 7fe73e8 Compare May 16, 2023 02:01
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #1014 (69c3355) into main (2e10beb) will increase coverage by 1.82%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1014      +/-   ##
==========================================
+ Coverage   46.60%   48.42%   +1.82%     
==========================================
  Files         105       73      -32     
  Lines        7884     6125    -1759     
==========================================
- Hits         3674     2966     -708     
+ Misses       3843     2846     -997     
+ Partials      367      313      -54     

see 43 files with indirect coverage changes

@mmsqe mmsqe force-pushed the ref_relayer branch 3 times, most recently from 1d77749 to 49c2810 Compare May 18, 2023 07:51
if len(contract.Input) < int(prefixSize4Bytes) {
return nil, errors.New("data too short to contain prefix")
}
prefix := int(binary.LittleEndian.Uint32(contract.Input[:prefixSize4Bytes]))

Check failure

Code scanning / gosec

Potential integer overflow by integer type conversion Error

Potential integer overflow by integer type conversion
@mmsqe mmsqe force-pushed the ref_relayer branch 2 times, most recently from 050408c to a70ce2e Compare May 23, 2023 07:25
app/app.go Fixed Show fixed Hide fixed
app/app.go Fixed Show fixed Hide fixed
app/app.go Fixed Show fixed Hide fixed
@mmsqe mmsqe marked this pull request as ready for review September 9, 2023 11:06
@mmsqe mmsqe requested a review from a team as a code owner September 9, 2023 11:06
@mmsqe mmsqe requested review from JayT106 and devashishdxt and removed request for a team September 9, 2023 11:06
Comment on lines +534 to +536
for k, v := range keys {
allKeys[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +537 to +539
for k, v := range tkeys {
allKeys[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
Comment on lines +540 to +542
for k, v := range memKeys {
allKeys[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map Warning

Iteration over map may be a possible source of non-determinism
nix/sources.json Outdated Show resolved Hide resolved
@mmsqe mmsqe added this pull request to the merge queue Sep 11, 2023
Merged via the queue into crypto-org-chain:main with commit 1f215fb Sep 11, 2023
31 of 32 checks passed
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.

3 participants