Skip to content

Problem: no stateful precompiled contract for relayer#1014

Merged
mmsqe merged 8 commits into
crypto-org-chain:mainfrom
mmsqe:ref_relayer
Sep 11, 2023
Merged

Problem: no stateful precompiled contract for relayer#1014
mmsqe merged 8 commits into
crypto-org-chain:mainfrom
mmsqe:ref_relayer

Conversation

@mmsqe

@mmsqe mmsqe commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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! :)

Comment thread integration_tests/ibc_utils.py Outdated
@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
Copy Markdown
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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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

thomas-nguy commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator

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

thomas-nguy commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator

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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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

mmsqe commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

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

thomas-nguy commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator
@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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor
@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

thomas-nguy commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator
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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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

thomas-nguy commented Apr 28, 2023

Copy link
Copy Markdown
Collaborator

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

yihuang commented Apr 28, 2023

Copy link
Copy Markdown
Contributor

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

codecov Bot commented May 16, 2023

Copy link
Copy Markdown

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

Comment thread x/cronos/keeper/precompiles/bank.go Fixed
Comment thread x/cronos/keeper/precompiles/bank.go Fixed
@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

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
Comment thread app/app.go Fixed
Comment thread app/app.go Fixed
Comment thread app/app.go 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 thread app/app.go
Comment on lines +534 to +536
for k, v := range keys {
allKeys[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map

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

Check warning

Code scanning / CodeQL

Iteration over map

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

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
Comment thread x/cronos/keeper/precompiles/relayer.go Outdated
Comment thread x/cronos/keeper/precompiles/relayer.go Outdated
Comment thread x/cronos/keeper/precompiles/relayer.go Outdated
Comment thread nix/sources.json Outdated
Comment thread x/cronos/keeper/precompiles/relayer.go Outdated
Signed-off-by: mmsqe <mavis@crypto.com>
Comment thread x/cronos/keeper/precompiles/relayer.go Outdated
Signed-off-by: yihuang <huang@crypto.com>
@mmsqe
mmsqe enabled auto-merge September 11, 2023 03:52
@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
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.

4 participants