fix(rpc): better error handling for transaction submission

[bowenwang1996]

Add two experimental rpcs:

• EXPERIMENTAL_broadcast_tx_sync that sends the transaction and waits for its validity to be checked before returning, but doesn't wait for the transaction to be processed.
• EXPERIMENTAL_check_tx that checks whether a transaction is still valid. For the rpc to work, the node that the request is sent to must track the shard of the sender, otherwise it will return Cannot determine whether the transaction is valid.

Fixes #2039.

Test plan

• test_check_invalid_tx that does a sanity check on EXPERIMENTAL_check_tx.
• pytest rpc_tx_submission that checks transactions can be submitted in all three ways and work well. Also check that invalid transactions can be caught by EXPERIMENTAL_check_tx.

[gitpod-io]

[SkidanovAlex]

EXPERIMENTAL_broadcast_tx_sync is not a correct name. sync implies that you won't return until the transaction is processed. In other words broadcast_tx_sync and broadcast_tx_commit must be synonyms.

We should either repurpose the existing boardcast_tx_async to do what your new method does, or call it differently (e.g. validate_and_broadcast_tx_async?)

[frol]

1. The return value should be an object (JSON key-value). Consider the RPC client perspective, you will need to guess if the returned string an error or a transaction hash; that is going to feel unreliable. We should always return an object (a structure / JSON key-value) instead of a plain string, this way it is also going to help us to extend the API response without breaking changes in the future (if needed).
2. This whole check_only looks a bit ugly to me, but I don't have better suggestions at the moment.

[bowenwang1996]

you will need to guess if the returned string an error or a transaction hash; that is going to feel unreliable

You don't need to guess. They are two different rpcs

[bowenwang1996]

We should always return an object (a structure / JSON key-value) instead of a plain string

What do you suggest here?

[frol]

They are two different rpcs

I was referring to the fact that the RPC can either return "some-tx-hash" or "Node does't track this...", so from the type perspective they are both strings. To distinguish them you need to either hardcode the error text messages or try to parse that string as hash (check base58 validity).

Currently, the response is:

{
  "jsonrpc": "2.0",
  "result": "some-tx-hash"
}

or

{
  "jsonrpc": "2.0",
  "result": "Node doesn't track ..."
}

This is really hard to work with, and also it is not extendible without a breaking change.

I suggest to have:

{
  "jsonrpc": "2.0",
  "result": {
    "transaction_hash": "some-tx-hash",
  }
}

and

{
  "jsonrpc": "2.0",
  "error": {
    "code": ...
    "data": "Node doesn't track...",
    "message": "Server error"
  }
}

[frol]

Well, I just noticed that the OK response is null value. Can we provide anything helpful in the response? (e.g. block height, block hash, chunk hash the transaction landed, if that is trivial to get)

[bowenwang1996]

That is for checking the transaction. If you just want to check whether the transaction is valid I don't know what else you want to include in the response.

[bowenwang1996]

@SkidanovAlex the naming follows tendermint API https://docs.tendermint.com/master/rpc/#/Tx/broadcast_tx_sync. It waits until the transaction gets into mempool before returning.

[SkidanovAlex]

Please wait for @frol's approval before merging

[frol]

We seem to miss a structured error handling here (as per similar discussion #2518 (comment)) /cc @fckt

[bowenwang1996]

@frol I created a separate issue #2536 for this.

[lexfrl]

@bowenwang1996
What does this error mean for user?

[frol]

@fckt Good catch.

We should help users to understand that they are expected to reach another node that tracks the shard.

[bowenwang1996]

@frol how to better phrase it? Should we include something like "try another node"?

[lexfrl]

I was thinking the node must be found automatically? (maybe the request should contain a sender_id?) How to use this method?