confio / ts-relayer

IBC Relayer in TypeScript
MIT License
109 stars 63 forks source link

Sending multiple packets from one tansaction (or msg?) not supported #255

Closed webmaster128 closed 2 years ago

webmaster128 commented 2 years ago

I recognized issues when multiple packets are sent from one transaction. Turns out the log string we get from Tendermint is buggy (see https://github.com/tendermint/tendermint/issues/9595). So most likely the code here is correct, but I want to bring this up to help other users trying to debug their issues.

Some ideas to improve the situation:

webmaster128 commented 2 years ago

The place where this pops up is:

  private async getPacketsFromTxs({
    minHeight,
    maxHeight,
  }: QueryOpts = {}): Promise<PacketWithMetadata[]> {
    let query = `send_packet.packet_connection='${this.connectionID}'`;
    if (minHeight) {
      query = `${query} AND tx.height>=${minHeight}`;
    }
    if (maxHeight) {
      query = `${query} AND tx.height<=${maxHeight}`;
    }

    const search = await this.client.tm.txSearchAll({ query });
    const resultsNested = search.txs.map(({ hash, height, result }) => {
      const parsedLogs = logs.parseRawLog(result.log);
      // we accept message.sender (cosmos-sdk) and message.signer (x/wasm)
      let sender = '';
      try {
        sender = logs.findAttribute(parsedLogs, 'message', 'sender').value;
      } catch {
        try {
          sender = logs.findAttribute(parsedLogs, 'message', 'signer').value;
        } catch {
          this.client.logger.warn(
            `No message.sender nor message.signer in tx ${toHex(hash)}`
          );
        }
      }
      return parsePacketsFromLogs(parsedLogs).map((packet) => ({
        packet,
        height,
        sender,
      }));
    });
    return ([] as PacketWithMetadata[]).concat(...resultsNested);
  }
ethanfrey commented 2 years ago

Use the events array instead of the raw log to work around it

This would make it worse most likely, as it would merge the multiple packet events of the same type into one packet. Using raw_log json should keep them as independent events.

Can you attach the JSON out of a raw_log where two ibc packets were emitted in the same tx? It may be a bug in parsing. Having a fixed json to test against would allow us to debug this much quicker

webmaster128 commented 2 years ago

This would make it worse most likely, as it would merge the multiple packet events of the same type into one packet. Using raw_log json should keep them as independent events.

In the events they are separate. In the logs they are merged. See https://github.com/tendermint/tendermint/issues/9595. There you also find all the example outputs.

ethanfrey commented 2 years ago

Good to discuss with tendermint/sdk teams. Not sure the proper way to get anything out of that data.

NB: what is the events field you refer to? Can you link to eg mintscan to show how this is different than log or raw_log?

webmaster128 commented 2 years ago

In https://gist.github.com/webmaster128/5efcb9de025f05a1cd2d55c22e25eb00 you see the full Tendermint RPC response of this transaction: https://testnet.mintscan.io/juno-testnet/txs/502E6F4AEA3FB185DD894D0DC14E013C45E6F52AC00A0B5224F6876A1CA107DB. There we get .result.tx_result.log which IMO contains buggy events. This is what we use for packets from tx search. Then there is .result.tx_result.events, which contains the events rolled out and complete. Since the events are binary in Tendeermint 0.34, they are base64 encoded here.

The term "raw log" does not exist in Tendermint. It was introduced at a higher level (the old LCD API?) there Tendermint's log was parsed into log and copied into raw_log. We inherited that in CosmJS when propagating Tendermint data to higher levels: rawLog: tx.result.log || "",.

ethanfrey commented 2 years ago

Yes, we should use (and properly parse) tendermint events. This seems to have improved since 2 years ago, when those were not very reliable.

Are these returned / processed properly in CosmJS? If they are, let's use them. If not, let's add those to next CosmJS release and then add them. I think we only support modern tendermint rpc endpoints now, so this should be a safe change. We support tendermint 0.34 (and soon 0.37?)

ethanfrey commented 2 years ago

Okay, it seems that BroadcastTxCommitResponse.deliverTx includes [TxData.events] (https://github.com/cosmos/cosmjs/blob/51a7214d0117351529bed7342078be066d489d5e/packages/tendermint-rpc/src/tendermint34/responses.ts#L200)

It would be good to update ts-relayer to use this, if this is now most reliable source of data.

I never really likes raw_log parsing, but that was the best source a while ago

webmaster128 commented 2 years ago

I think that can be done. I'll check it out.