search

paritytech / json-rpc-interface-spec

29 stars 3 forks source link

transaction: Change `bestChainBlockIncluded` to `Included` #87

Open bkchr opened 1 year ago

bkchr commented 1 year ago

The server should report the inclusion of a transaction in any block its seeing, not only what it assumes to be the best block. Best block is any way a highly opinionated view of the server. There are several factors that can determine on what is the actual best block (e.g. on going disputes etc). Another reason why the server return Included for every block are parachains. Parachains are determining on what is the best block by checking what is the best block of the relay chain. This means currently it takes at least around 12 seconds to get any kind of feedback on your transaction. While, the transaction is probably already in a block that it just waiting to get included in the relay chain.

The most important event for dapps/wallets should be the finalized one any way, as it is then clear that the changes are not getting reverted. If we in between reported multiple blocks that included the transaction, shouldn't change anything.

tomaka commented 1 year ago

For what it's worth, there's a big ongoing discussion (#79) about the behavior of watching transactions.

I'm advocating for moving to the JSON-RPC client the responsibility of determining when a transaction is included (#77), which would make this a non-issue.

jsdw commented 1 year ago

I'd have no issue with this change since it's providing more information than what we get back at the moment. (fwiw I hope we keep something like transaction_submitAndWatch myself, but yeah, ongoing discussion).

tomaka commented 1 year ago

since it's providing more information than what we get back at the moment.

Well, no, it's not that simple. You're not just providing more information, you're also losing information about what is the best chain.

If you report all the blocks where a transaction is included, then it becomes way more difficult for the JSON-RPC client to determine whether a transaction has a chance of being included in the finalized chain in the future. For example, imagine you try to do a double spend. The transaction you submit gets included in some forks and not others. Suddenly the JSON-RPC client has to track the tree of blocks in order to know whether the transaction is in all forks or only some of them.

The JSON-RPC client already has to track the tree of blocks through chainHead_follow, and if we add included events for all blocks where a transaction has been included then the JSON-RPC client has to track the tree twice. Which is why #77 again makes sense to me.

bkchr commented 1 year ago

Suddenly the JSON-RPC client has to track the tree of blocks in order to know whether the transaction is in all forks or only some of them.

Not sure why it needs to track all forks. The app should only change the view on the transaction being finalized (which is its own event).

tomaka commented 1 year ago

The app should only change the view on the transaction being finalized (which is its own event).

Why do you want to change the included event(s) then, if you're at the same time saying that the app shouldn't care about them?

bkchr commented 1 year ago

Because there are still apps outside that care. For Parachains that will also give you faster feedback on what is the status of your transaction (currently we don't set the best before we know that a block was included in the relay chain).

The app seeing that the transaction was included in different forks, should not be a big problem for the app? I mean what is the best chain is a "local view" of the light client/full node anyway. Some other node could have a completely different view on what is the best.

Currently we only know these apps where you exactly know that this app is using a blockchain and thus sending a transaction. However, for your and my grandmother we probably want to have apps that don't mention "transaction" at all and they are just pressing buttons. These kind of apps will not really need the "included" event, maybe to update some progress bar, but not that much more.

tomaka commented 1 year ago

My take-way from your comment is that you want to make this change for apps that are not end-user apps, that want quick feedback on when a transaction is included because 12 seconds is too long, but also tolerate not being able to precisely know on which forks the transaction is included. I'm not sure such an app exists.

Again, what I think should happen is we leave this function as-is, because it is specifically meant to be easy to use, and if you want something more precise you track the transaction "manually" by following the chain from the JSON-RPC client side.