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

accept retracted XtStatus #759

Open
brenzi opened this issue Apr 8, 2024 · 7 comments · May be fixed by #807
Open

accept retracted XtStatus #759

brenzi opened this issue Apr 8, 2024 · 7 comments · May be fixed by #807
Assignees

Comments

@brenzi
Copy link
Contributor

brenzi commented Apr 8, 2024

Retracted tx status means that it had been included in a block which happens to be orphaned on a non-finalized fork.
We saw this happening more frequently recently and actually its not a problem in the usual flow because finality will still happen eventually as the nodes inject retracted into tx pool again automatically.

I'd suggest to accept retracted as a non-error

It may still be worth a warning in the logs

@brenzi
Copy link
Contributor Author

brenzi commented Apr 8, 2024

example commits (no PR because based on old release):
brenzi@480823e
brenzi@d01bc07

@brenzi brenzi changed the title accept retracted status accept retracted XtStatus Apr 8, 2024
@Niederb
Copy link
Contributor

Niederb commented Apr 9, 2024

@brenzi I will have a look at it.
Just to clarify: The two commits from your comment would basically implement this? Or am I missing somthing?

@brenzi
Copy link
Contributor Author

brenzi commented Apr 10, 2024

Yes they would. Happy to submit a PR, but I first wanted to raise the discussion to see if you agree and would merge rhis

@Niederb
Copy link
Contributor

Niederb commented Apr 10, 2024

I also gave it some more thought. In general I find the current separation into expected and unexpected status quite arbitrary. Whether they are expected or not depends on the use case and I feel that by making this separation we make a decision that actually the user should take.
In polkadot there is a is_final method: https://github.com/paritytech/polkadot-sdk/blob/74a42cebc1a9fd4e4a7713d5e41caba77a0fa172/substrate/client/transaction-pool/api/src/lib.rs#L161
Maybe we should also use this in our methods. You can wait for some status but once we receive a final status we stop waiting. What do you think?

@brenzi
Copy link
Contributor Author

brenzi commented Apr 16, 2024

That sounds better than my hack commits

@Niederb
Copy link
Contributor

Niederb commented Apr 16, 2024

Ok, I will try to come up with a PR. Still need to do some thinking on how to do this. This change could lead to some very subtle breaks of backward compatibility for clients.

@Niederb Niederb self-assigned this Apr 16, 2024
@haerdib
Copy link
Contributor

haerdib commented Sep 3, 2024

When we change to is_final instead of the current expected one, we shouldn't forget about the future status. So it probably makes sense to do #295 in the same swoop.

@Niederb Niederb linked a pull request Oct 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants