params: fix V2 compare issue #1926
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
and removed request for
Copilot
gzliudan
force-pushed
the
fix-v2-compare
branch
from
3371920 to
6dbaeee
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a V2 comparison issue in XDPoS configuration validation. The main fix removes the comparison of configIndex (which are internal keys for AllConfigs maps) that was causing false mismatches after upgrades. The changes also refactor the equality functions to provide better debugging through detailed log warnings for each field mismatch.
Key Changes:
- Removed
configIndexcomparison from V2Equal function to fix upgrade compatibility issue - Added detailed logging for each field comparison to aid debugging
- Refactored nil-checking logic in all three equality functions
- Modified SwitchEpoch comparison to skip checks when either value is 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
params/config.go
Outdated
| return true | ||
| } | ||
| if a.SwitchEpoch != b.SwitchEpoch || !configNumEqual(a.SwitchBlock, b.SwitchBlock) || !slices.Equal(a.configIndex, b.configIndex) { | ||
| if a.SwitchEpoch != b.SwitchEpoch && a.SwitchEpoch != 0 && b.SwitchEpoch != 0 { |
There was a problem hiding this comment.
The SwitchEpoch comparison logic has a special case where if either SwitchEpoch is 0, the comparison is skipped. However, this creates an asymmetric equality check. For example, if a.SwitchEpoch=0 and b.SwitchEpoch=100, they would be considered equal, but if a.SwitchEpoch=100 and b.SwitchEpoch=200, they would not be equal. This breaks the reflexive property of equality and could lead to unexpected behavior.
| if a.SwitchEpoch != b.SwitchEpoch && a.SwitchEpoch != 0 && b.SwitchEpoch != 0 { | |
| if a.SwitchEpoch != b.SwitchEpoch { |
| func V2ConfigEqual(a, b *V2Config) bool { | ||
| if a == nil || b == nil { | ||
| return a == b | ||
| if a != b { | ||
| log.Warn("[V2ConfigEqual] One of the configs is nil", "a", a, "b", b) | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
| return a.MaxMasternodes == b.MaxMasternodes && a.SwitchRound == b.SwitchRound && a.CertThreshold == b.CertThreshold | ||
| if a.MaxMasternodes != b.MaxMasternodes { | ||
| log.Warn("[V2ConfigEqual] MaxMasternodes mismatch", "a.MaxMasternodes", a.MaxMasternodes, "b.MaxMasternodes", b.MaxMasternodes) | ||
| return false | ||
| } | ||
| if a.SwitchRound != b.SwitchRound { | ||
| log.Warn("[V2ConfigEqual] SwitchRound mismatch", "a.SwitchRound", a.SwitchRound, "b.SwitchRound", b.SwitchRound) | ||
| return false | ||
| } | ||
| if a.CertThreshold != b.CertThreshold { | ||
| log.Warn("[V2ConfigEqual] CertThreshold mismatch", "a.CertThreshold", a.CertThreshold, "b.CertThreshold", b.CertThreshold) | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The V2ConfigEqual function is incomplete. It only compares MaxMasternodes, SwitchRound, and CertThreshold, but the V2Config struct has many more fields that should be compared including MaxProtectorNodes, MaxObverserNodes, MinePeriod, TimeoutSyncThreshold, TimeoutPeriod, MasternodeReward, ProtectorReward, ObserverReward, MinimumMinerBlockPerEpoch, LimitPenaltyEpoch, MinimumSigningTx, and ExpTimeoutConfig. This could lead to configurations being considered equal when they are actually different.
| // Only check configs in both of AllConfigs | ||
| for k1, cfg1 := range a.AllConfigs { | ||
| if cfg2, ok := b.AllConfigs[k1]; ok { | ||
| if !V2ConfigEqual(cfg1, cfg2) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The V2Equal function only checks configs that exist in both AllConfigs maps (intersection). This means if one config has additional entries that the other doesn't have, they will still be considered equal. This asymmetric comparison could lead to incorrect equality results. Consider whether configs should be considered equal if they have different sets of keys in AllConfigs.
| // Only check configs in both of AllConfigs | |
| for k1, cfg1 := range a.AllConfigs { | |
| if cfg2, ok := b.AllConfigs[k1]; ok { | |
| if !V2ConfigEqual(cfg1, cfg2) { | |
| return false | |
| } | |
| } | |
| // Ensure AllConfigs have the same number of entries | |
| if len(a.AllConfigs) != len(b.AllConfigs) { | |
| log.Warn("[V2Equal] AllConfigs length mismatch", "len(a.AllConfigs)", len(a.AllConfigs), "len(b.AllConfigs)", len(b.AllConfigs)) | |
| return false | |
| } | |
| // Ensure every config in a.AllConfigs exists in b.AllConfigs and is equal | |
| for k1, cfg1 := range a.AllConfigs { | |
| cfg2, ok := b.AllConfigs[k1] | |
| if !ok { | |
| log.Warn("[V2Equal] Missing config in b.AllConfigs", "key", k1) | |
| return false | |
| } | |
| if !V2ConfigEqual(cfg1, cfg2) { | |
| return false | |
| } |
gzliudan
force-pushed
the
fix-v2-compare
branch
from
6dbaeee to
b417adf
Compare
5 participants
Proposed changes
Ref:
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