-
Notifications
You must be signed in to change notification settings - Fork 61
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: race condition of x.signer in v1 consensus #445
Conversation
parent := chain.GetHeader(header.ParentHash, number-1) | ||
if parent == nil { | ||
return consensus.ErrUnknownAncestor | ||
} | ||
// Set the correct difficulty | ||
header.Difficulty = x.calcDifficulty(chain, parent, x.signer) | ||
header.Difficulty = x.calcDifficulty(chain, parent, signer) |
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.
How about this version ?
if number%x.config.Epoch != 0 {
// Gather all the proposals that make sense voting on
addresses := make([]common.Address, 0, len(x.proposals))
for address, authorize := range x.proposals {
if snap.validVote(address, authorize) {
addresses = append(addresses, address)
}
}
// If there's pending proposals, cast a vote on them
if len(addresses) > 0 {
header.Coinbase = addresses[rand.Intn(len(addresses))]
if x.proposals[header.Coinbase] {
copy(header.Nonce[:], utils.NonceAuthVote)
} else {
copy(header.Nonce[:], utils.NonceDropVote)
}
}
}
parent := chain.GetHeader(header.ParentHash, number-1)
if parent == nil {
return consensus.ErrUnknownAncestor
}
// Set the correct difficulty
x.lock.RLock()
signer := x.signer
x.lock.RUnlock()
header.Difficulty = x.calcDifficulty(chain, parent, signer)
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.
The locks are already in the code before, i'm not sure what values it's locking, probably x.proposals. I prefer not to change the original logic as it would involve unnecessary riskks
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 can refer the struct and other codes:
lock sync.RWMutex // Protects the signer fields
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'm not sure i understand what you mean above.
What's i'm referring to is that the lock is already within the original if statement.
I do not intend to change the behaviour of the original code hence move the signer := x.signer
right before the Runlock in the original code. This would be the smallest change that would be required to achieve the same goal
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.
The lock
field protects signer field.
Proposed changes
Fix a small bug where race condition could happen when concurrently update x.signer
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅
in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅
in the boxes that applyChecklist
Put an
✅
in the boxes once you have confirmed below actions (or provide reasons on not doing so) that