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

fix: Tx incorrect status #759

Closed
wants to merge 4 commits into from
Closed

fix: Tx incorrect status #759

wants to merge 4 commits into from

Conversation

boecklim
Copy link
Collaborator

Description of Changes

Provide a brief description of the changes you've made.

Testing Procedure

  • I have added new unit tests
  • All tests pass locally
  • I have tested manually in my local environment

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have updated CHANGELOG.md with my changes

@boecklim boecklim changed the title Fix/tx incorrect status fix: Tx incorrect status Jan 27, 2025
if err != nil {
return err
}
if txInfo.BlockHash != "" || txInfo.BlockHeight > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, as I was saying there still is case where we assign rejected status but transaction gets mined eventually.

Imagine we submit chained transaction and for some transactions they don't see parent yet in mempool so we receive ORPHANED status.

Now we do this logic ⬆️ and before we started this check those parent transactions got mined in a new block. So those input transactions now have txInfo.BlockHash != "" || txInfo.BlockHeight > 0 so we assign REJECT status to this one but in fact it will be mined (probably in the next block).

I admit this case is pretty rare but that's why I didn't like the approach because there is the case when we tell user that transaction is rejected but it gets mined eventually which is very bad.

@boecklim boecklim self-assigned this Jan 28, 2025
@boecklim boecklim closed this Jan 28, 2025
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.

2 participants