paritytech / json-rpc-interface-spec

30 stars 3 forks source link

`transaction_broadcast`: Clarify stopping the broadcast process #130

Closed lexnv closed 9 months ago

lexnv commented 9 months ago

The transaction_broadcast is currently propagated over the peer-to-peer network until the transaction_stop is called.

Within this context, the JSON-RPC server might try to broadcast the transaction indefinitely if the user omits or forgets to call transaction_stop.

Lower Bound

It might be beneficial to add a lower bound to the number of broadcasts, after which the server may stop the broadcasting process:

"The JSON-RPC server will try to propagate this transaction over the peer-to-peer network for at least 4 blocks, or until transaction_stop is called". (4 is arbitrary in this case) This allows us to clean up resources and avoid broadcasting indefinitely.

From the transaction_stop method:

A JSON-RPC error is generated if the operationId doesn't correspond to any active transaction_unstable_broadcast operation.

If the server chooses to stop the broadcasting after 4 blocks, the user will receive an error when calling transaction_stop.

Freely stop the broadcast

Similarly, we could assume that the server is free to terminate the transaction_broadcast at any time (may it be that the server chooses to decode blocks and stop the transaction when it is included in a finalized block; or that the server broadcasted the transaction N times and chooses to clean-up resources).

Then we could extend the spec a bit with: "The JSON-RPC server will try to propagate this transaction over the peer-to-peer network until transaction_unstable_stop is called. _However, the server might stop the broadcasting when it deems necessary".

From my perspective, the second option gives the server more freedom (ie can never submit a transaction that fails to decode). This might have been the indent of the transaction_broadcast altogether, just wanted to double check with you

Would love to get your thoughts on this 🙏

cc @paritytech/subxt-team @tomaka @jsdw @josepot

tomaka commented 9 months ago

I don't see the point of specifying a lower bound. Blocks generation and transactions propagation operate completely in parallel. If the time between two blocks was 2 minutes (which is something parachains could realistically do, and could also in theory happen in case of offline validators for example), then the minimum bound would be 8 minutes.

This allows us to clean up resources and avoid broadcasting indefinitely.

What's the problem exactly with broadcasting indefinitely? A transaction occupies at most a few megabytes of memory. These few megabytes "belong" to the JSON-RPC client. When the client disconnects, stop is implicitly called and these few megabytes are freed. This is why there's a limit to the number of broadcasted transactions per JSON-RPC client.

As far as I remember, the Substrate transactions pool gives up watching a transaction after 1024 blocks (which for the sake of security is basically "forever"), but that's because the transaction stays in the pool even after the JSON-RPC client disconnects.

The JSON-RPC server will try to propagate this transaction over the peer-to-peer network until transaction_unstable_stop is called. _However, the server might stop the broadcasting when it deems necessary

Note the wording in the spec. It doesn't say that the JSON-RPC server broadcasts the transaction. The name of the function is overly simplistic. It says that the JSON-RPC server tries to include the transaction in the chain. There are importantly not the same thing. Some transactions are intentionally never broadcasted.

lexnv commented 9 months ago

Thanks Pierre for all the info! 🙏

The wording clarifies things for me! The server will do its best to broadcast, but not guarantee (will try to propagate the transaction does not imply that will propagate any transaction), thanks again!