Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Aug 9, 2023

Close #76

As mentioned in the title, the current transaction namespace is now transactionWatch, and a new transaction namespace is added, allowing just broadcasting transactions.

The new transaction_unstable_broadcast function is very dumb: it just broadcasts the transaction.
Before calling it, it is important for the JSON-RPC client to validate the transaction (using chainHead_call) and check if the propagate field returned by the validation is true. The JSON-RPC server intentionally does not do either of these things.
It is then the responsibility of the JSON-RPC client to download the bodies of blocks and check if the submitted transaction is there.

One drawback is that these new functions might be difficult to implement in Substrate.

cc @josepot @lexnv @jsdw

@tomaka
Copy link
Contributor Author

tomaka commented Aug 9, 2023

I'm conflicted about whether the JSON-RPC server should perform a transaction validation.

Currently, a JSON-RPC client could abuse this function to make a JSON-RPC server broadcast invalid transactions. This will lead to the server losing reputation.

It could be a good idea to force the server to validate the transaction against its current best block (intentionally not saying which one) before broadcasting, but in that case the question is what to do if the transaction is invalid. It feels a bit stupid to just do nothing, but on the other hand reporting the problem to the JSON-RPC client leads to more complications.

@jsdw
Copy link
Collaborator

jsdw commented Aug 9, 2023

I guess the point of this method is just so that if the client is already downloading and checking block bodies anyway, it can avoid calling transactionWatch_submitAndWatch and just rely on this?

(In Subxt's case, since it can talk to RPC nodes too, it's probably always preferable to call transactionWatch_submitAndWatch to avoid needing to download block bodies where possible, I think, so maybe this is more aimed at CAPI/light-client-only APIs?).

Forcing validation and avoiding the client needing to do that step (and being able to broadcast invalid transactions etc) makes sense to me. What are the complications of returning an error if it is/becomes invalid?

@josepot
Copy link
Contributor

josepot commented Aug 9, 2023

Before calling it, it is important for the JSON-RPC client to validate the transaction (using chainHead_call) and check if the propagate field returned by the validation is true.

That's fine because we were already planing to use chainHead_call to call system_dryRun to check for possible errors before submitting the transaction.

I guess the point of this method is just so that if the client is already downloading and checking block bodies anyway, it can avoid calling transactionWatch_submitAndWatch and just rely on this?

(In Subxt's case, since it can talk to RPC nodes too, it's probably always preferable to call transactionWatch_submitAndWatch to avoid needing to download block bodies where possible, I think, so maybe this is more aimed at CAPI/light-client-only APIs?).

Forcing validation and avoiding the client needing to do that step (and being able to broadcast invalid transactions etc) makes sense to me. What are the complications of returning an error if it is/becomes invalid?

I'm of the opinion that for any library that wants to be light-client first (not light client only, but light-client first cough 🙂), the best approach would be to use the body of the finalized blocks as the only reliable source of information for knowing exactly when a transaction is in a finalized block. On top of that, it would also be possible to use the transactionWatch_submitAndWatch events as a "hint" in case that you get lucky and you receive an event before, which would allow you to skip that check. That being said, I'm a bit on the fence on whether it would pay off to add the complexity of leveraging those "hints". I will probably try both approaches, though.

Also, please, let's keep in mind that we would only be examining the body of the latest blocks while there are transactions "pending", which for most use-cases that's not very often.

At the same time, it would make a lot of sense to develop a tiny library for those use-cases where they only need to be sending transactions very often, and they don't really care about when exactly those transactions get finished. In other words: for those use cases where they won't be caring about chainhead_follow, and that therefore they don't have to worry about unpining blocks efficiently, then we could provide a tiny library to fulfil their needs, of course... And that library would be relying solely on the transactionWatch_submitAndWatch events, yeah. So, for those use cases, those events are necessary, yep. Actually, now that I think about it, for those use-cases, there probably isn't a need for a library at all, they could just do that stuff "manually". So, yeah, I can see how those events are useful for those use cases, for sure!

@josepot josepot self-requested a review August 9, 2023 21:43
josepot
josepot previously approved these changes Aug 9, 2023
Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks @tomaka !

@tomaka tomaka marked this pull request as draft August 10, 2023 07:19
@tomaka tomaka marked this pull request as ready for review August 10, 2023 07:31
@tomaka
Copy link
Contributor Author

tomaka commented Aug 10, 2023

I've opened polkadot-fellows/RFCs#19 for reference

@josepot
Copy link
Contributor

josepot commented Aug 10, 2023

The JSON-RPC server might check whether the transaction is valid before broadcasting it. If it does so and if the transaction is invalid, the server should silently do nothing and the JSON-RPC client is not informed of the problem. Invalid transactions should still count towards the limit to the number of simultaneously broadcasted transactions.

This is problematic.

I understand why you added this, but consider the following scenario: the JSON-RPC client is well informed about the latest finalizedBlock, it has properly created and validated the transaction against it and it has immediately broadcasted this transaction. It is possible that the JSON-RPC server that validates the transaction is lagging behind a bit, right? So, in that case it would validate the transaction against the previous block, then the validation would fail, it wouldn't be broadcasted and the JSON-RPC client -despite having done everything correctly- would never know that's what happened.

I propose that if the server decides to validate and the validation fails, that we use the operationId for communicating that to the client via an event, and that this event includes in its payload the block that was used for performing the validation. Either that, or the server shouldn't be allowed to validate the transaction.

In case that it wasn't clear enough, this event that I'm suggesting would only emitted if and only if: the server decides to validate the transaction and the transaction turns out to be invalid. Otherwise, no events would be emitted.

@tomaka
Copy link
Contributor Author

tomaka commented Aug 10, 2023

It is possible that the JSON-RPC server that validates the transaction is lagging behind a bit, right? So, in that case it would validate the transaction against the previous block, then the validation would fail, it wouldn't be broadcasted and the JSON-RPC client -despite having done everything correctly- would never know that's what happened.

This is completely intended.

I propose that if the server decides to validate and the validation fails, that we use the operationId for communicating that to the client via an event, and that this event includes in its payload the block that was used for performing the validation. Either that, or the server shouldn't be allowed to validate the transaction.

How would the client react, then?

@jsdw
Copy link
Collaborator

jsdw commented Aug 10, 2023

If the server may or may not not report back on the invalid state of a transaction, do we care that an invalid transaction submitted as immortal or with a fairly large mortality may require the client to download a lot of block bodies before eventually deciding to give up searching for the tx or that the mortality has expired? Or is this a case of seeing how it goes and we can add another RPC method to ask whether a transaction is in a block if that is an issue?

@josepot
Copy link
Contributor

josepot commented Aug 10, 2023

I propose that if the server decides to validate and the validation fails, that we use the operationId for communicating that to the client via an event, and that this event includes in its payload the block that was used for performing the validation. Either that, or the server shouldn't be allowed to validate the transaction.

How would the client react, then?

It depends on the client needs, I suppose.

In the case of CAPI: we would stop trying to find the transaction in the bodies of the incoming blocks, and we may decide to try the transaction again if we can determine that the validation problem was caused by a "temporary" accident due to the server lagging behind a bit. Basically, if the server is getting in the way and it's not broadcasting the transaction, then we need to know that. Otherwise, we would be under the wrong impression that the transaction is being broadcasted, when it really isn't. So, yep, if the server decides to get in the way, then it needs to let us know that the transaction was not broadcasted.

In the end, the answer to this question is: it would react exactly the same as it would react if we were using transactionWatch_submitAndWatch and we received the invalid event.

@josepot
Copy link
Contributor

josepot commented Aug 11, 2023

@tomaka does my previous comment make sense to you? 🙏 Also, I apologize for the number of edits 😬.

A possible alternative to the conditional invalid event that I suggested could be a broadcasted event that should be emitted if the server has actually started broadcasting the transaction. I mean, I prefer the invalid event TBH, but a broadcasted event would also work.

@jsdw
Copy link
Collaborator

jsdw commented Aug 11, 2023

I guess I still don't don't really see the use cases for this call over transactionWatch_submitAndWatch.

  • transactionWatch_submitAndWatch will tell me if the transaction is broadcasted in case I'm interested in just that (in that case I'd stop paying attention after I saw that)
  • transactionWatch_submitAndWatch will give me a clear end state that I know I will no longer need to do anything after (invalid/dropped/error/finalized). Whether I choose to scan block bodies myself or not, that's really useful to give me a better chance of not just being left scanning bodies for ages when the thing was invalid (or the mortality expires!) a while ago.
  • If transaction_unstable_broadcast were only to "optionally" return an invalid error, that still means that it may not, which feels like a weaker promise than the other.

So then, the only use case I could see for this RPC call is as a "fire and forget" type thing. For me, this isn't useful, but perhaps for others it is. Am I missing something (quite probably this is the case right now :D)?

@josepot
Copy link
Contributor

josepot commented Aug 11, 2023

I guess I still don't don't really see the use cases for this call over transactionWatch_submitAndWatch.

Let me provide some insight.

1. Significance of Latest Finalized Block:
To create a valid transaction, it's imperative to be informed of the latest finalized block. If you're crafting a transaction, you'll either leverage the chainhead group of functions or a non-spec compliant method to obtain what the server considers to be the current finalized block.

2. Dependency between Transaction and Chainhead Functions:
If a server can validate a transaction, it logically follows that the server can support the chainhead functions. The integral relationship between these two function sets shouldn't be overlooked. In essence, if a server offers transactional functionalities, it should also cater to chainhead functionalities, ensuring you construct transactions based on the same block against which they're validated.

3. Argument against Segregation:
One might argue that separating the transaction functions from chainhead allows servers flexibility (some servers may opt-out from the transaction group). But if I had a say, this doesn't offer enough merit for keeping them distinct.

4. Use Cases Illustration:
There's a spectrum of consumers, and understanding their needs underscores the importance of the suggested call:

  • The Casual User: Some primarily aim to initiate transactions and ensure their inclusion in the chain, without an immediate need for the details. They’d immediately unpin every single block, while keeping track of the latest finalized block and would benefit from a method that abstracts away the intricacies. For them, transactionWatch_submitAndWatch is perfect.

  • The Detail-Oriented User: On the contrary, some users must know immediately if their transaction is in the finalized (or best) block. They download block bodies, wanting to pinpoint their transactions instantly. For them, a low-level API makes more sense to eliminate redundancies between client and server actions.

Subxt, for instance, might not fit snugly into these categories, and that's alright. But CAPI aligns with the latter. If this PR doesn't get merged, we'll resort to using transaction_submitAndWatch and we will ignore the events that we don't need. However, we'd rather utilize the more streamlined API which precisely meets our requirements.

Conclusion:
Reflecting on the arguments, I advocate for the integration of the transaction group into chainhead. It appears to be a solution that addresses the outlined concerns.

@josepot
Copy link
Contributor

josepot commented Aug 13, 2023

As @tomaka pointed out on element, we should first have a proper discussion about this topic. I opened #79 as an attempt to have a structured discussion around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants