refactor(consensus): fix config object not reference to same one and concurrency issues and refactor access pattern by benjamin202410 · Pull Request #2146 · XinFinOrg/XDPoSChain

benjamin202410 · 2026-03-08T18:06:34Z

refactor(consensus): fix config concurrency issues and refactor access pattern

Return defensive copies of V2Config to prevent concurrent modification
Add Config() method to XDPoS_v2 engine for centralized config access
Update hooks to retrieve config through engine instead of chain config
Use json.Decoder with UseNumber() in deepCloneJSON for safer JSON parsing
Remove unnecessary return statement in saveRewardToFile

These changes prevent race conditions when accessing V2 configs from multiple
goroutines and establish a clearer separation of concerns by accessing
configuration through the consensus engine.

Changes:

Added bytes import for bytes.NewReader
Replaced json.Unmarshal with json.NewDecoder + UseNumber()
Ensures numeric data integrity in consensus engine operations

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

Impacted Components

Which parts of the codebase does this PR touch?
Put an ✅ in the boxes that apply

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A

coderabbitai · 2026-03-08T18:06:52Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8f17de50-d70a-4fb1-8d9c-fd4f929dd1f3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix(consensus)-preserve-numeric-precision-in-deepCloneJSON-using-json.Number

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This PR refactors the XDPoS consensus engine's configuration access patterns to improve thread safety and reduce coupling. It prevents potential race conditions when reading V2Config from multiple goroutines by returning defensive copies, centralizes config access through a new Config() method on XDPoS_v2, and uses json.Decoder with UseNumber() in deepCloneJSON to preserve numeric precision.

Changes:

params/config.go: Both GetCurrentConfig() and Config(round) now return a value copy of the config struct instead of the original pointer, preventing callers from mutating shared config.
consensus/XDPoS/engines/engine_v2/engine.go: Adds Config(r uint64) method as a centralized accessor, uses UseNumber() in deepCloneJSON, and removes an unnecessary return statement.
eth/hooks/engine_v2_hooks.go: Updates HookPenalty and HookReward to retrieve config through the engine (via bc.Engine()) rather than directly from the chain config.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
`params/config.go`	Returns defensive copies of `V2Config` in `Config()` and `GetCurrentConfig()` to prevent shared mutation
`consensus/XDPoS/engines/engine_v2/engine.go`	Adds `Config()` method, uses `json.Decoder`+`UseNumber()` in `deepCloneJSON`, removes bare `return`
`eth/hooks/engine_v2_hooks.go`	Hooks now resolve config via `bc.Engine()` cast to `*XDPoS.XDPoS` rather than the chain config

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T03:05:05Z

params/config.go

@@ -855,7 +855,10 @@ func (v2 *V2) Config(round uint64) *V2Config {
 			break
 		}
 	}
-	return v2.AllConfigs[index]
+
+	// avoid config is changed by other goroutines
+	cfg := *v2.AllConfigs[index]
+	return &cfg


The V2.Config(round uint64) function accesses v2.configIndex and v2.AllConfigs without holding v2.lock, while the similar GetCurrentConfig() correctly acquires v2.lock.RLock() before accessing v2.CurrentConfig.

Since v2.configIndex is written by BuildConfigIndex() and v2.AllConfigs is a shared map (read at line 860), concurrent calls to Config(round) while another goroutine runs BuildConfigIndex() — or any code that modifies the underlying maps — would be a data race. The comment on line 859 ("avoid config is changed by other goroutines") acknowledges the intent to fix this, but the lock is not actually acquired.

The fix should add v2.lock.RLock() / v2.lock.RUnlock() at the start of Config(round uint64), consistent with how GetCurrentConfig() and UpdateConfig() protect accesses to shared fields.

good suggustion

Copilot · 2026-03-10T03:05:05Z

eth/hooks/engine_v2_hooks.go

+		XDPoSEngine, ok := bc.Engine().(*XDPoS.XDPoS)
+		if XDPoSEngine == nil || !ok {
+			return []common.Address{}, fmt.Errorf("[HookPenalty] XDPoS not found")
+		}
+		currentConfig := XDPoSEngine.EngineV2.Config(uint64(round))


In both HookPenalty and HookReward, the new code performs a redundant bc.Engine().(*XDPoS.XDPoS) type assertion. The function's outer parameter adaptor is already the *XDPoS.XDPoS engine — this is verified in eth/backend.go:357 where c := eth.engine.(*XDPoS.XDPoS) is passed directly as adaptor. bc.Engine() returns the same eth.engine. The added type assertion is therefore unnecessary, and calling adaptor.EngineV2.Config(uint64(round)) directly would be simpler and eliminate the extra runtime check.

Suggested change

XDPoSEngine, ok := bc.Engine().(*XDPoS.XDPoS)

if XDPoSEngine == nil || !ok {

return []common.Address{}, fmt.Errorf("[HookPenalty] XDPoS not found")

}

currentConfig := XDPoSEngine.EngineV2.Config(uint64(round))

currentConfig := adaptor.EngineV2.Config(uint64(round))

Copilot · 2026-03-10T03:05:06Z

eth/hooks/engine_v2_hooks.go

+		XDPoSEngine, ok := bc.Engine().(*XDPoS.XDPoS)
+		if XDPoSEngine == nil || !ok {
+			return nil, fmt.Errorf("[HookReward] XDPoS not found")
+		}
+		currentConfig := XDPoSEngine.EngineV2.Config(uint64(round))


Same redundant bc.Engine().(*XDPoS.XDPoS) assertion as in the HookPenalty closure (see comment on lines 102–106). The adaptor parameter already holds the *XDPoS.XDPoS engine, so adaptor.EngineV2.Config(uint64(round)) should be used directly.

Suggested change

XDPoSEngine, ok := bc.Engine().(*XDPoS.XDPoS)

if XDPoSEngine == nil || !ok {

return nil, fmt.Errorf("[HookReward] XDPoS not found")

}

currentConfig := XDPoSEngine.EngineV2.Config(uint64(round))

currentConfig := adaptor.EngineV2.Config(uint64(round))

gzliudan changed the title ~~fix(consensus): preserve numeric precision in deepCloneJSON using jso…~~ fix(consensus): preserve numeric precision in deepCloneJSON using json.Number Mar 10, 2026

bug fix for using same config object

109c33b

benjamin202410 force-pushed the fix(consensus)-preserve-numeric-precision-in-deepCloneJSON-using-json.Number branch from c4430e6 to 109c33b Compare March 10, 2026 02:57

benjamin202410 changed the title ~~fix(consensus): preserve numeric precision in deepCloneJSON using json.Number~~ refactor(consensus): fix config object not reference to same one and concurrency issues and refactor access pattern Mar 10, 2026

gzliudan requested a review from Copilot March 10, 2026 03:00

Copilot started reviewing on behalf of gzliudan March 10, 2026 03:01 View session

wanwiset25 approved these changes Mar 10, 2026

View reviewed changes

Copilot AI reviewed Mar 10, 2026

View reviewed changes

update

ab8a14a

gzliudan approved these changes Mar 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(consensus): fix config object not reference to same one and concurrency issues and refactor access pattern#2146

refactor(consensus): fix config object not reference to same one and concurrency issues and refactor access pattern#2146
benjamin202410 wants to merge 2 commits intodev-upgradefrom
fix(consensus)-preserve-numeric-precision-in-deepCloneJSON-using-json.Number

benjamin202410 commented Mar 8, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Mar 8, 2026 •

edited

Loading

Review skipped

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

benjamin202410 Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

benjamin202410 commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Impacted Components

Checklist

Uh oh!

coderabbitai bot commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

benjamin202410 Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benjamin202410 commented Mar 8, 2026 •

edited

Loading

coderabbitai bot commented Mar 8, 2026 •

edited

Loading