Fix potential mem-leak from hotstuff #1514
Conversation
| @@ -835,6 +835,27 @@ func (as *AggregatorSuite) TestNonePruneAfterBlock() { | |||
| require.Equal(as.T(), 3, votingStatusLen) | |||
| } | |||
|
|
|||
| // receive the block for view 2,3,4,5 | |||
| // prune by view 5, should be all pruned | |||
| func (as *AggregatorSuite) TestPruneAll() { | |||
There was a problem hiding this comment.
This test case was able to reproduce the issue. Without the fix, this test will fail
| @@ -153,6 +153,15 @@ func (va *VoteAggregator) StoreProposerVote(vote *model.Vote) bool { | |||
| return false | |||
| } | |||
| va.proposerVotes[vote.BlockID] = vote | |||
| // update viewToBlockIDSet | |||
There was a problem hiding this comment.
We can't use updateState here, even though that will that also update the viewToBlockIDSet which is used for pruning.
The reason is that the proposer Vote might be invalid, which we don't want to update the state with.
However, the viewToBlockIDSet is safe to update alone, since it's purely used for pruning.
There was a problem hiding this comment.
proposer Vote might be invalid
Good observation. From the perspective of VoteAggregator, the proposer vote is still unchecked. A double voting attempt cannot be attributed without checking the vote first.
I was thinking that the EventHander always validates the proposer vote first:
Nevertheless, this happens outside of the
VoteAggregator and we should probably not rely on external components for critical validity checks.
zhangchiqing
requested review from
AlexHentschel,
durkmurder,
tarakby,
simonhf and
synzhu
| @@ -153,6 +153,15 @@ func (va *VoteAggregator) StoreProposerVote(vote *model.Vote) bool { | |||
| return false | |||
| } | |||
| va.proposerVotes[vote.BlockID] = vote | |||
| // update viewToBlockIDSet | |||
There was a problem hiding this comment.
proposer Vote might be invalid
Good observation. From the perspective of VoteAggregator, the proposer vote is still unchecked. A double voting attempt cannot be attributed without checking the vote first.
I was thinking that the EventHander always validates the proposer vote first:
Nevertheless, this happens outside of the
VoteAggregator and we should probably not rely on external components for critical validity checks.
zhangchiqing
force-pushed
the
leo/fix-potential-mem-leak-from-hotstuff
branch
from
5df7142
to
f6218fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #1514 +/- ##
==========================================
- Coverage 55.09% 55.09% -0.01%
==========================================
Files 521 521
Lines 32514 32521 +7
==========================================
+ Hits 17914 17917 +3
- Misses 12192 12195 +3
- Partials 2408 2409 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* fix potential mem-leak from hotstuff (#1514) * Kan/clean up secondary result index (#1515) * Remove from secondary index * Add tests for caches * add concurrency test * Apply suggestions from code review Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com> Co-authored-by: Leo Zhang (zhangchiqing) <zhangchiqing@gmail.com> Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com> * disable cache metrics Co-authored-by: Kan Zhang <kan@axiomzen.co> Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com>
This PR fixed a potential mem leak in hotstuff, where when a proposerVote was stored, the index for pruning was not updated, which leads to a potential memory leak.