Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add feature to TXM to detect and purge stuck transactions #12881
Add feature to TXM to detect and purge stuck transactions #12881
Changes from 46 commits
246b04d
d47d88f
8c713dc
05c5a11
360d9fa
0a04fff
d26473c
1541c5a
1d4113d
c71ac14
2e46dde
8185dc8
20bfb9d
82e3a5d
db5264e
0d5b318
25105a1
491f9e3
338ab26
dc17b98
468b73b
736eb89
b2902c6
6eb5dc5
a5663ee
0cea37b
a3cd9b7
02306fe
a13605b
c2deae5
a5ea4aa
1cfb5cc
335033a
a5ba3a0
7513d36
4b41b4b
cc5764d
b1faa07
d67d79b
c4062f5
54b9c7c
d244012
0915400
c95d261
db5e5d9
31d69ba
90ec4a3
d81da52
a33460b
c08504b
88826a7
e400a36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to have
LoadPurgeBlockNumMap
in theNewStuckTxDetector
initialization?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this to avoid another method the confirmer needs to call but this method requires a context. I wouldn't have anything to wire this up to during initialization and didn't want to introduce a context to the TXM builder method just to pass it down to the detector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually need to call this from startInternal(), which can be called multiple times during the lifetime of the Confirmer. So doing it in NewStuckTxDetector() will just be redundant. You still have to call it from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does key refer to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the same disclaimer we have in other places where we process transactions for different
fromAddress
's in separate go routines.key
here refers to a sender key which is just afromAddress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if gas bumping fails due to reaching the max limit? I think this logic assumes there is always room for gas bumping. A tx that gets stuck isn't already likely to hit those limits due to gas bumping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could consider this as a misconfiguration since our attempt threshold config would have to be higher than the amount of bumped attempts it takes to hit the configured max price. I don't think we can really get around this in code since our hands are tied with configs. I can add a note to the
AutoPurge.MinAttempts
config to call this out thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimriou : I assume you mean the max_price limit on our node?
the max_limit set by the caller should be ignored for Purging, in my opinion.
If we are stuck at max_price, then yes, there's no way to recover, except NOP changing the max_price limit on the node. However if we fix the gas bumping bug in BlockHistoryEstimator, I don't think we will ever hit this situation.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like ignoring configs could look like unexpected behavior and maybe cause confusion. Since NOPs wouldn't know the inner workings of this auto-purge feature, I could foresee them noticing a tx post with a higher price than their configs and falsely raise a flag only for us to debug and find it was expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the configs, we shouldn't override them. I guess the NOPs will see the repeated error message and figure out they would have to manually send the txs. I don't see a better alternative.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add comments on what is this interface for.