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

[tomaka]

I can't reopen #77 due to missing rights, so I created a new PR.

[jsdw]

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:

• No need to buffer finalized blocks in chainHead_follow while you're waiting for transactionWatch_submitAndWatch to return a Finalized event telling you which block it made it into.

And an implementation detail that's important is:

• 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.

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]

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:

• transaction_broadcast doesn't try to validate the transaction and sends it out to the other peers, but all the other peers find that the transaction is invalid and silently discard it.
• transaction_broadcast finds out that the transaction is invalid and silently discards it.

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]

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]

Thanksfor the explanation @tomaka, that all makes sense! So by re-validating the transaction periodically, we will always know when to stop checking block bodies for it (ie we will either find it in a block, or we'll see that it's become invalid).

All good from my side then, and I can see the utility of having a more "lighter weight and lower level do-it-yourself" style way to submit transactions.

@josepot if you're ok with this then I'm happy to merge it, and if not then we can iron out any niggles.

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

Ah yup, sorry! Perhaps I got muddled because I went skimming over various background bits after reading this PR!

[josepot]

Overall I'm on board with this change. However, there are a few things that I would like to discuss before the PR gets merged, which is why I'm flagging the PR as "Request changes". Not because I'm actually "demanding" these changes, but because I want to make sure that we have properly discussed it before the PR gets merged.

It would be nice if it was possible to use this new API, in combination with the other APIs of chainHead_unstable_follow to emulate the same behaviour that one would get by using: transactionWatch_unstable_submitAndWatch.

Currently, that's almost possible. The consumer could perform a dry-run, validate the transaction, calculate the fees, etc and then broadcast it and search it within the new blocks to see if the transaction made into list of best-blocks and/or whether it gets finalized. Amazing!

However, there is one event that you can not get with the current API: the number of peers to which the transaction is being broadcasted to. The thing is that transactionWatch_unstable_submitAndWatch lets you know to how many peers the transaction has been broadcasted, which can be an interesting piece of information for some UIs.

I'm well aware of the fact that this value can not be used as a way of inferring whether it's possible to "cancel" the transaction... I understand that once the transaction has been "broadcasted" it, then there is no going back. Even if you try to immediately stop it, it's very possible that some peers have already received it and therefore the transaction is already being gossiped. I know that. I mean, sure, maybe you got lucky and it turns out that the server decided to silently validate the transaction behind the scenes, and you managed to stop it before the server finished the validation. However, I understand that once the transaction_unstable_broadcast method has been invoked, then there is no way to guarantee that the transaction won't be gossiped.

Nevertheless, I think that it would be nice to be able to have the equivalent of the broadcasted events that you would get if you happened to use transactionWatch_unstable_submitAndWatch.

Thoughts?

PS: @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.

[jsdw]

@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]

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]

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]

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]

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]

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:

• valid / invalid
• If the previous was valid it would then emit a series of broadcasted events until either the user stops the operation or the server decides to emit a:
• dropped event (if for some reason the server has decided to stop broadcasting the transaction).

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]

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]

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).

[josepot]

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.

Also, taking into account what @tomaka said: we can consider calling the transaction_unstable_broadcast as the transaction being broadcasted, and most users won't care to know to how many peers it's being broadcasted anyways.

I actually prefer using this API than what we currently have. I mean, it's quite annoying to have to search the transactions on the client because it can actually be quite CPU intensive, but you can't have it all in this life, so... 🙂

In the end, I'm starting to realize that there are a bunch of things that Polkadot-API is doing that shouldn't be done in the main-thread anyways. So, since we will have to find a solution to that problem regardless, then searching for the transactions on the block bodies is not the end of the world.

Thanks for this @tomaka !

[tomaka]

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]

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:

• the maximum number of broadcasted transactions has been reached.
• the server won't broadcast it because it's a validator.
• the server won't broadcast it because it's invalid.

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.

[josepot]

Thanks!

(I just merged main in order to resolve the conflicts)