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

broadcasted field of dropped event of transactionsWatch hard to enforce #132

Closed
tomaka opened this issue Feb 9, 2024 · 3 comments · Fixed by #134
Closed

broadcasted field of dropped event of transactionsWatch hard to enforce #132

tomaka opened this issue Feb 9, 2024 · 3 comments · Fixed by #134

Comments

@tomaka
Copy link
Contributor

tomaka commented Feb 9, 2024

Let's say that there is a load balancer, one of the load balanced servers is watching a transaction using transactionsWatch_watch, but this server crashes or goes down intentionally.

When that happens, the load balancer will naturally send a dropped event to the client to notify it that the transaction isn't being watched anymore.
However, the value that the load balancer should put in the broadcasted field of that dropped event is unclear. The load balancer can't know whether or not the server has broadcasted the transaction before going down. For this reason, it is forced to use true no matter what.

@josepot
Copy link
Contributor

josepot commented Feb 9, 2024

On that same note: what should be the expected behaviour if the server crashes before having emitted the validated/invalidated events?

I mean, based on what we discussed in here, I think that the "broadcasted" concept should be completely removed from transactionWatch_unstable_submitAndWatch.

@lexnv
Copy link
Contributor

lexnv commented Feb 12, 2024

In this case, I would opt for removing the broadcasted field from the Dropped event. It can only accurately describe the broadcast process if the load balancer received the Broadcast event before a restart.

My understanding from #107 (comment), is that the actual number of peers the transaction has been broadcasted doesn't necessarily represent different peers. And the purpose of this event is to check if the transaction could later be included in the chain by some other peers -- the tx has been broadcasted / left the node.

If I got this right, we could then remove the numPeers from the Broadcasted event; and emit the Broadcasted event when numPeers > 0.
However, this doesn't offer any strong guarantees about the tx inclusion, even when multiple peers have received the tx.
Developers that want the tx inclusion guarantee would watch for the InBlock / Finalized events (in combination with announced blocks).

The Broadcasted event might present some insights for the user-experience of some UIs, to tell the user that the transaction has reached other peer(s) while waiting for inclusion.

From my perspective, we could either simplify the Broadcasted event by removing the numPeers; or remove it altogether (I'm not aware of any UIs / developers that makes assumptions on this event)

Would love to hear your thoughts 🙏

@tomaka
Copy link
Contributor Author

tomaka commented Feb 12, 2024

I'd probably be for removing it altogether.

The idea behind this field was simply to be displayed to users to show progress.
End users would see that the transaction is being validated, then has been broadcasted to 1 peer, then 2, then 3, etc. a bit like a loading bar. Just like a loading bar, the idea was to show some movement that something was happening.

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 a pull request may close this issue.

3 participants