-
Notifications
You must be signed in to change notification settings - Fork 182
Fix txpool loop reset logic to support setHead #1316
Fix txpool loop reset logic to support setHead #1316
Conversation
Have you tried implementing a unit test for this case? |
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 am not sure this suggestion is proper mitigation for your issue. Can you explain the detail of your issue or share the panic log?
It seems that all behaviors of txpool as well as txpool.reset
should be stopped during the SetHead
. In that case, we can consider locking txpool mutex or other way.
Sorry for the late response. @ehnuje I tried to write test code, but it was difficult to reproduce the situation where only the block header exists and block doesn't. @aidan-kwon What I modified is handling of a special error that occurs only when the block corresponding to oldHead is nil when an event occurs in chainHeadCh. The reason that event does not occur in chainHeadCh during setHead is the downloader function is turned off while SetHead. |
blockchain/tx_pool.go
Outdated
|
||
// If newNum >= oldNum, then it's not a case of setHead. | ||
if newNum >= oldNum { | ||
logger.Warn("Transaction pool reset with missing oldhead", |
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.
If an unexpected situation happens and stops something to do, the log should be ERROR.
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 understand that this PR is not harmful in normal situations. However, I don't think it is the correct mitigation for the setHead panic issue. I think we are supposed to find a way that prevents calling reset
with no existing old blockhead. This change could be a partial solution to the issue, but it could conceal the real problem our code might have.
Can you explain more detail about the situation the empty oldHead is used?
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.
@hqjang-pepper Please, make an issue for the following task
Proposed changes
When setHead function is executed, panic has occured intermittently when syncing chain segments.
This error occured when txpool tries to reinject discarded transactions of blocks, but can't get block because blockchain is rewinded.
This PR is containing this error handling.
I tested enough with private network (4cn) environment.
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...