paritytech / json-rpc-interface-spec

29 stars 3 forks source link

Rename the existing `transaction` to `transactionWatch` and add a new different `transaction` namespace #107

Closed tomaka closed 8 months ago

tomaka commented 12 months ago

I can't reopen https://github.com/paritytech/json-rpc-interface-spec/pull/77 due to missing rights, so I created a new PR.

jsdw commented 8 months ago

This has lingered a while and @lexnv pointed me at it, so wanted to try and progress it.

To check my understanding (I haven't read all of the many previous messages about this, just skimmed), I think the way that it's intended for you to use transaction_broadcast is as so:

  1. Call chainHead_call to validate your TX.
  2. Follow finalized (or new) blocks via chainHead_follow.
  3. Call transaction_broadcast to broadcast your validated TX.
  4. Download each block body to look for your TX; when you see it in a finalized block you're done!

And as I understand it, the big advantage over transactionWatch_submitAndWatch is:

And an implementation detail that's important is:

Is my understanding correct? Are there any other caveats to submitting a tx with transaction_broadcast that it's worth being aware of (ie is there chance the TX becomes invalid later, and I have no indication that I should stop looking out for it?)? (It makes me think that showing example usage for some of these methods might be nice)

Otherwise, I have no issue with merging this! It'd be good to get @josepot's review though since he likely has more thoughts/concerns, and I haven't read enough to know whether this resolves them or not.

tomaka commented 8 months ago

Your understanding is mostly correct!

transaction_broadcast does not validate the TX. This is to avoid a node validating it against an older block than expected and silently dropping it. Instead, the client must validate it.

transaction_broadcast might or might not validate the transaction, it's left at the discretion of the implementer. This is mentioned in the text.

The main motivation is that these two situations are equivalent:

Therefore, the implementation is allowed to do the latter (finding out that the transaction is invalid and discarding it) as "an optimization".

Instead, the client must validate it.

Also note that, besides validating a transaction, the client most likely wants to check the fees of the transaction. Additionally, there are some transactions that are considered valid but that will fail to do anything once included in a block, and this is something that the client most likely wants to query as well.

Because the client wants to query these two things, it might as well validate the transaction as well.

(ie is there chance the TX becomes invalid later, and I have no indication that I should stop looking out for it?)

Yes, actually, a transaction might become invalid later, and so should be re-validated from time to time. Validating a transaction returns a struct that contains a field named longevity, and the transaction should be revalidated after longevity blocks.

lexnv commented 8 months ago

From the perspective of submitting a transaction to the peers without the server validating it first; would it be possible that this behavior will lead to the p2p reputation decrease? And the node to not be able to broadcast valid transactions for some time? For example, in cases where a malicious user crafts an invalid transaction; then the invalid transaction is broadcasted to peers repeatedly. The malicious user could then fail to call transaction_stop, letting the reputation of the node decrease over time 🤔

jsdw commented 8 months ago

@jsdw have you changed your mind with regards of this API? I'm pretty sure that a while back you stated that Subxt wouldn't be using this API.

I'm not sure yet!

For the time being things generally work well with transaction_submitAndWatch, but the whole "if its slow to emit a Finalized event, we'll buffer lots of blocks up" thing might prompt me to experiment at some point.

In that case, a halfway step for me might be to use transaction_submitAndWatch plus downloading blocks bodies to avoid needing to buffer any, but if I took that step then it seems almost natural to go the whole way and emit my own events as I scan block bodies.

This is also why I wanted your input on this PR; I can def see the utility of having a lower level API but I think you are much more interested in using it than me, so we need it to make sense for you to be worth merging, else it might not actually get used.

josepot commented 8 months ago

As I revisit the specifics of this API, I'm reminded of several concerns I initially had 😅:

Firstly, I believe it's risky to delegate the responsibility of halting the transaction broadcast to the consumer. This approach is problematic because failing to stop the broadcast in a timely manner could lead to the propagation of an invalid transaction. The same could be said about the validation of the transaction. In my opinion, the API should be design to prevent the consumer from such scenarios.

Additionally, I have reservations about the potential for ambiguous and inconsistent behavior. Consistency is key, and the server should either always validate the transaction or never do so. The current notion that the server:

might or might not validate the transaction

is, to me, unacceptable. We need uniform behavior across different implementations to ensure reliability and predictability.

Moreover, effective utilization of this API appears to hinge on access to the chainHead group of functions. This is the only practical method to validate transactions and determine the right moment to cease broadcasting. This implicit coupling on chainHead is suboptimal, especially since no guarantees can be offered with regards of a different group of functions.

Given these considerations, I'd strongly advocate for an API like chainHead_unstable_submitTx, instead. This would likely provide a more robust and clear-cut solution.

tomaka commented 8 months ago

and the server should either always validate the transaction or never do so

Whether the server validates the transaction or not has, and I stress this, absolutely zero visible consequence for the client.

If the server finds out that the transaction is invalid and doesn't broadcast it, then the effects are exactly the same as broadcasting it anyway.

This is purely an implementation detail of the server. The JSON-RPC API has tons and tons of implementation details that have absolutely zero visible consequences.

tomaka commented 8 months ago

I also want to add that this is to me completely a non-problem.

In which situation would you ever call this JSON-RPC function with an invalid transaction? The client builds the transaction, and if this transaction is invalid it indicates a bug in the client implementation.

The only scenario that I can see where a transaction is invalid is if by coincidence a runtime upgrade happens and removes the logic of that transaction from the runtime altogether. For example if you send a treasury vote, and right at this moment the runtime upgrades and removes the treasury from the chain altogether. If that happens, then we likely have a broader UI problem than just a specific treasury vote.

josepot commented 8 months ago

and the server should either always validate the transaction or never do so

Whether the server validates the transaction or not has, and I stress this, absolutely zero visible consequence for the client.

If the server finds out that the transaction is invalid and doesn't broadcast it, then the effects are exactly the same as broadcasting it anyway.

This is purely an implementation detail of the server. The JSON-RPC API has tons and tons of implementation details that have absolutely zero visible consequences.

I understand that, of course.

The point that I'm trying to make is that the spec wouldn't have to talk about these kind of implementation details if we had an operation like chainHead_unstable_submitTx that received a followSubscription opaque string as an argument.

josepot commented 8 months ago

If the server finds out that the transaction is invalid and doesn't broadcast it, then the effects are exactly the same as broadcasting it anyway.

This is purely an implementation detail of the server. The JSON-RPC API has tons and tons of implementation details that have absolutely zero visible consequences.

What I'm trying to explain is that this lack of visibility is quite annoying, because there is no way for the consumer to know whether the server is actually broadcasting the transaction or not.

In reality, this API is going to make different servers apply different strategies to ensure that they are not doing something silly, the issue is that there is no way for the consumer to know about what's going on with that transaction, which IMO is quite undesirable.

A client could be under the wrong impression that a transaction is being broadcasted (when in reality it isn't), which means that the client would start searching the transaction within the incoming blocks "indefinitely" for no good reason. On top of that, the user would also be under the impression that the transaction is being broadcasted, when in reality that may not be the case, so the user won't know whether to try to submit the transaction again or wait until the transaction makes it into the blockchain. All in all, giving the server the ability to silently drop a transaction creates a really bad API for both the client and the end user.

I think that the alternative that I'm proposing is a lot better for the client, the server and the user: with a chainHead_unstable_submitTx the server could produce the following events:

I wouldn't expect the server to search the transaction within the block bodies, the client can do that on their own or just use transaction_unstable_submitAndWatch.

tomaka commented 8 months ago

What I'm trying to explain is that this lack of visibility is quite annoying, because there is no way for the consumer to know whether the server is actually broadcasting the transaction or not.

I'm repeating my point above because you're ignoring it: even if the server has broadcasted the transaction, the transaction might still be invalid and silently discarded by other nodes.

A transaction being broadcasted is in no way a guarantee that it will be included in a block.

In reality, this API is going to make different servers apply different strategies to ensure that they are not doing something silly, the issue is that there is no way for the consumer to know about what's going on with that transaction, which IMO is quite undesirable.

The consumer knows that the server is trying to make the transaction go to the chain.

so the user won't know whether to try to submit the transaction again

There's absolutely no point in submitting the transaction again. Either the transaction makes it into the chain at some point or it won't. Cancelling a broadcast then restarting it has zero advantage.

josepot commented 8 months ago

I'm repeating my point above because you're ignoring it: even if the server has broadcasted the transaction, the transaction might still be invalid and silently discarded by other nodes.

A transaction being broadcasted is in no way a guarantee that it will be included in a block.

Fair enough!

There's absolutely no point in submitting the transaction again.

I didn't mean submitting the same transaction again. I meant to try to create a new transaction for the same call with the same arguments. Meaning that sometimes a transaction could be invalid because of the way that it was created in a certain moment, but re-creating the transaction just a few moments later could make the newly created transaction valid (for instance: you may have queried a stale nonce value due to a race condition).

tomaka commented 8 months ago

I still think that it would be nice to have a way to get the broadcasted events, but that is something that can be added later on top of the current API.

I forgot to point this out above, but a transaction might be included in the chain even if it's not broadcasted, in case the JSON-RPC server is a validator node.

The "broadcasted" event has always been a bit shitty. You can have 50 broadcasted peers, which are actually 50 times the same peer that we keep disconnecting and reconnecting from. Only "broadcasted == 0" and "broadcasted != 0" are actually useful to know. The actual number isn't useful. But even "broadcasted == 0" and "broadcasted != 0" can't be relied upon because, as I've said, the server might be validator.

josepot commented 8 months ago

The "broadcasted" event has always been a bit shitty. You can have 50 broadcasted peers, which are actually 50 times the same peer that we keep disconnecting and reconnecting from. Only "broadcasted == 0" and "broadcasted != 0" are actually useful to know. The actual number isn't useful. But even "broadcasted == 0" and "broadcasted != 0" can't be relied upon because, as I've said, the server might be validator.

Half-baked idea:

What if transaction_unstable_broadcast only returns the opaque string representing the operation if and only if broadcasted != 0 and then we have a list of possible errors for cases like:

All that while we explain that the server has no obligation to validate the transaction.

EDIT

After having put a bit more thought into that ^ half-baked idea, I no longer think that the server should only return the "operationId" if -and only if- the operation has started.

The reason being that as @tomaka previously pointed out: that would be misleading, because the server could decide not to validate the transaction and broadcast it (so the operation would appear to have "started"). However, that would have the exact same effect as having silently dropped the transaction as other nodes would just ignore that transaction. Therefore, there is no value whatsoever into stating that "the server should only return the "operationId" if -and only if- the operation has started", because that would be completely misleading.