params: fix V2 compare issue #1923
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
force-pushed
the
fix-v2-compare
branch
from
3ad0bcf to
71894f2
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a V2 configuration comparison issue in the XDPoS consensus params package. The changes improve nil handling logic, enhance logging by switching from Info to Warn level for mismatches, and restructure comparison functions to provide more granular field-by-field validation with detailed logging.
Key changes include:
- Improved nil pointer handling in comparison functions (XDPoSConfigEqual, V2Equal, V2ConfigEqual)
- Enhanced logging from Info to Warn level for better visibility of configuration mismatches
- Refactored V2Equal to simplify AllConfigs comparison by only checking overlapping keys
- Expanded V2ConfigEqual to compare fields individually with detailed logging
Comments suppressed due to low confidence (1)
params/config.go:583
- The V2ConfigEqual function is missing comparisons for several important fields of V2Config: MinePeriod, TimeoutSyncThreshold, TimeoutPeriod, and ExpTimeoutConfig (which includes Base and MaxExponent). These fields are part of the V2Config struct and should be compared to ensure configuration equality. Without these comparisons, two V2Config instances with different values for these fields would incorrectly be considered equal.
return true
}
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
}
func (c *XDPoSConfig) String() string {
return "XDPoS"
}
func (c *XDPoSConfig) Description(indent int) string {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func V2ConfigEqual(a, b *V2Config) bool { | ||
| if a == nil || b == nil { | ||
| log.Info("[V2ConfigEqual] Nil check", "a", a, "b", b, "equal", a == b) | ||
| return a == b | ||
| if a != b { |
There was a problem hiding this comment.
The V2Equal function now only checks configs that exist in both AllConfigs maps (keys that overlap). This means if config A has keys [1, 2, 3] and config B has keys [1, 2, 4], the function would not detect that they are different - it would only compare keys 1 and 2. The comparison should verify that both maps have the same set of keys before comparing individual configs. Additionally, the comparison of configIndex was removed from line 543 (originally checked with slices.Equal), but configIndex is a derived field from AllConfigs that's used for config lookups. If the maps have different keys, the configIndex values would differ, which could cause runtime issues.
gzliudan
force-pushed
the
fix-v2-compare
branch
from
71894f2 to
37c65e6
Compare
4 participants
Proposed changes
configIndexis the keys ofAllConfigs, we should skip compare a.configIndex and b.configIndex after upgrade.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