Conversation
schnetzlerjoe
requested review from
AdityaSripal,
cezarad and
cwgoes
as code owners
|
IBC packets should not need to be sent in order to conduct a cross-chain state query, that is inefficient. I suggest an approach based on instructing relayers to create a specific cross-chain state proof through some sort of standardised interface instead. |
Hey! Yes, that is the idea. The idea is to utilize ABCI Query already implemented on relayers. So chain submits the transaction to Query module. Relayer detects this query in state via a query event that is sent, the relayer then goes and performs an ABCI Query (includes proof) and takes the results and submits it into state on the querying chain. No packets or IBC work really needed. This also allows querying without other chains having to integrate the module (saves many developer hours). |
Splendid - then I misunderstood, my bad. The approach you propose makes total sense to me :) . |
AdityaSripal
left a comment
AdityaSripal
left a comment
There was a problem hiding this comment.
Nice work. There are two main changes requested.
- Remove dependence on ABCI
- Remove ability of relayer to affect result data
|
|
||
| The Querying Chain starts with the implementation of the Interchain Query Module by adding the module into their chain. | ||
|
|
||
| The general flow for interchain queries starts with a Cross Chain Query Request from the Querying Chain which is listened to by relayers. Upon recognition of a cross chain query, relayers utilize a ABCI Query Request to query data from the Queried Chain. Upon success, the relayer submits a `MsgSubmitQueryResult` to the Querying chain with the success flag as 1. |
There was a problem hiding this comment.
This is making assumptions that both chains are ABCI chains which I believe is unnecessary. All we need is a request that has a ICS23 KeyPath and key. And a response that contains the value and a proof
There was a problem hiding this comment.
This is made even easier with the changes scheduled on the client refactor. See: #687
|
|
||
| ### Data Structures | ||
|
|
||
| A CrossChainABCIQueryRequest data type is used to specify the query. Included in this is the `Path` which is the path field of the query i.e: /custom/auth/account. `Key` is the data key to name the query i.e: `pools` or `stakers`. `Id` is the id of the query with each key to id being unique i.e: stakers-0 or stakers-1275 (this example follows key-id format). `TimeoutHeight` specifies the timeout height on the querying chain to timeout the query. `Bounty` is a bounty that is given to the relayer for participating in the query. `ClientId` is used to identify the chain of interest. |
There was a problem hiding this comment.
I don't understand how the relayer is supposed to construct the final key to query with from Path, Key, and ID.
It doesn't make sense to me to have these constructed by the relayer, when the querying chain can just do this itself and include the final key path in the message.
| type CrossChainABCIQueryRequest struct { | ||
| Path string | ||
| Key string | ||
| Id string | ||
| TimeoutHeight uint64 | ||
| Bounty sdk.Coin | ||
| ClientId string |
There was a problem hiding this comment.
| type CrossChainABCIQueryRequest struct { | |
| Path string | |
| Key string | |
| Id string | |
| TimeoutHeight uint64 | |
| Bounty sdk.Coin | |
| ClientId string | |
| type CrossChainQueryRequest struct { | |
| KeyPath ics23.MerklePath | |
| TimeoutHeight uint64 | |
| Bounty sdk.Coin | |
| ClientId string |
There was a problem hiding this comment.
|
|
||
| The general flow for interchain queries starts with a Cross Chain Query Request from the Querying Chain which is listened to by relayers. Upon recognition of a cross chain query, relayers utilize a ABCI Query Request to query data from the Queried Chain. Upon success, the relayer submits a `MsgSubmitQueryResult` to the Querying chain with the success flag as 1. | ||
|
|
||
| On failure of a query, relayers submit `MsgSubmitQueryResult` with the `success` flag as 0 to the Querying chain. Alternatively on timeout based on the height of the querying chain, the querying chain will submit `SubmitQueryTimeoutResult` with the timeout height specified. |
There was a problem hiding this comment.
How is this failure proved?
The relayer should be able to either submit a proof of existence for that key, or a proof of nonexistence for that key at the requested height.
Otherwise, if there is some error that cannot be proven to querying chain, then we must wait for timeout
| Path string | ||
| Key string | ||
| Id string | ||
| TimeoutHeight uint64 |
There was a problem hiding this comment.
We should very explicitly document that for querying the timeout height is the height on the QUERYING CHAIN. Since this is very different semantics from regular IBC where timeout height is height on the destination chain not sender chain
| ``` | ||
|
|
||
| ```go | ||
| type QueryResult struct { |
There was a problem hiding this comment.
Needs to include the id necessary to retrieve the specific query on the querying chain that this result is referring to.
| - Control Queried Data: The Querying Chain should have ultimate control on how to handle queried data. Like querying for a certain query form/type. | ||
|
|
||
| - Incentivization: In order to incentivize relayers for participating in interchain queries, a bounty is paid. | ||
|
|
There was a problem hiding this comment.
In addition to desired properties, we should also in this case have a section to underscore what the querying module does not guarantee.
Should be clear to any users of querying module what is promised by interchain querying and what is not promised.
For example:
Since there's no prior negotiation, we cannot assume that data coming back from queried chain will be in an expected format.
We also cannot assume that the value stored under the key will be stable, since it may change after the query result has been submitted
| A CrossChainABCIQueryRequest data type is used to specify the query. Included in this is the `Path` which is the path field of the query i.e: /custom/auth/account. `Key` is the data key to name the query i.e: `pools` or `stakers`. `Id` is the id of the query with each key to id being unique i.e: stakers-0 or stakers-1275 (this example follows key-id format). `TimeoutHeight` specifies the timeout height on the querying chain to timeout the query. `Bounty` is a bounty that is given to the relayer for participating in the query. `ClientId` is used to identify the chain of interest. | ||
|
|
||
| ```go | ||
| type CrossChainABCIQueryRequest struct { |
There was a problem hiding this comment.
Currently the relayer has complete control over which height on the queried chain to use.
This is not a problem with regular IBC apps, because the IBC protocol ensures that the data queried (commitments, acks) will never change and thus the relayer can query at any height and is guaranteed by the protocol to get the same value.
However, with interchain querying, the keys that are being queried can have different values at different heights.
Thus, a malicious relayer can choose to query a height that has a value that benefits them the most.
In fact, with the current spec a relayer can choose to query at a height before the query was even posted on the querying chain.
Thus, it changes the security model from relayers being purely liveness facilitators to them being able to actively manipulate what data goes on chain.
We should not have a different security model for this feature, since that is bound to cause mistakes. we should remove the ability for relayers to influence the outcome a query by specifying the desired height to be queried in the request itself.
The relayer then must return the result from a query at that height, and once again we get back to the original security model where the relayer is just able to affect liveness
| Path string | ||
| Key string | ||
| Id string | ||
| TimeoutHeight uint64 |
There was a problem hiding this comment.
| TimeoutHeight uint64 | |
| QueryHeight Height // height on queried chain at which result must be queried from | |
| TimeoutHeight Height // height on querying chain by which point result must be posted |
There was a problem hiding this comment.
| Data []byte | ||
| Key string | ||
| Id string | ||
| Height uint64 |
There was a problem hiding this comment.
| Height uint64 |
Not needed since Height should be specified by querying agent
|
Closing. Replaced by #735 |
7 participants
Draft of interchain query spec to discuss and add to