consensus/XDPoS: guard decodeMasternodesFromHeaderExtra against short extra - #2477
consensus/XDPoS: guard decodeMasternodesFromHeaderExtra against short extra#2477SAY-5 wants to merge 5 commits into
Conversation
2026 Summer Release v2.8.0 dev upgrade merge
…thereum#2377) * quick fix to return old signingTX behavior * fix test
… extra Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe XDPoS masternode decoder now handles short header ChangesMasternode Extra decoding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens XDPoS consensus header parsing by preventing a runtime panic in decodeMasternodesFromHeaderExtra when header.Extra is shorter than the required ExtraVanity + ExtraSeal prefix, in both engine v1 and engine v2.
Changes:
- Add a length precondition in
decodeMasternodesFromHeaderExtra(v1 + v2) to returnnilinstead of panicking on shortExtra. - Refactor decoding to reuse a local
extraslice variable for clarity. - Add unit tests in
engine_v1covering short-Extra(no panic, returns nil) and valid-Extra(decodes expected masternodes).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| consensus/XDPoS/engines/engine_v2/utils.go | Add short-Extra guard to prevent makeslice panic when decoding masternodes at the v1→v2 switch block. |
| consensus/XDPoS/engines/engine_v1/utils.go | Add the same short-Extra guard to prevent makeslice panic when decoding masternodes from checkpoint headers. |
| consensus/XDPoS/engines/engine_v1/utils_test.go | Add tests for short and valid Extra decoding behavior in v1. |
| func decodeMasternodesFromHeaderExtra(checkpointHeader *types.Header) []common.Address { | ||
| masternodes := make([]common.Address, (len(checkpointHeader.Extra)-utils.ExtraVanity-utils.ExtraSeal)/common.AddressLength) | ||
| extra := checkpointHeader.Extra | ||
| if len(extra) < utils.ExtraVanity+utils.ExtraSeal { | ||
| return nil | ||
| } | ||
| masternodes := make([]common.Address, (len(extra)-utils.ExtraVanity-utils.ExtraSeal)/common.AddressLength) | ||
| for i := 0; i < len(masternodes); i++ { | ||
| copy(masternodes[i][:], checkpointHeader.Extra[utils.ExtraVanity+i*common.AddressLength:]) | ||
| copy(masternodes[i][:], extra[utils.ExtraVanity+i*common.AddressLength:]) | ||
| } |
|
Good finding ! How about this version ? func decodeMasternodesFromHeaderExtra(checkpointHeader *types.Header) []common.Address {
extra := checkpointHeader.Extra
minRequiredLen := ExtraVanity + ExtraSeal
// Skip processing if extra length is below minimum threshold
if len(extra) < minRequiredLen {
return nil
}
// Simplify calculation with pre-defined minRequiredLen
addressTotalPayloadLen := len(extra) - minRequiredLen
// Verify total payload length is exactly aligned to single address size
if addressTotalPayloadLen%common.AddressLength != 0 {
return nil
}
masternodeCount := addressTotalPayloadLen / common.AddressLength
masternodes := make([]common.Address, masternodeCount)
// Use incremental offset to avoid repeated multiplication calculation
srcOffset := ExtraVanity
for i := range masternodes {
copy(masternodes[i][:], extra[srcOffset:])
srcOffset += common.AddressLength
}
return masternodes
} |
|
please change merge target branch to dev-upgrade |
…FromHeaderExtra Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
|
Adopted your version in 09a4e9f across both engine_v1 and engine_v2, including the payload-alignment check so a non-multiple-of-AddressLength extra returns nil rather than truncating. Also retargeted the PR to dev-upgrade. The short-extra and valid-extra tests still pass. |
# Conflicts: # contracts/utils_test.go
Proposed changes
decodeMasternodesFromHeaderExtrain bothengine_v1andengine_v2computes a slice length fromheader.Extrawith no length precondition:make([]common.Address, (len(Extra)-ExtraVanity-ExtraSeal)/AddressLength). Whenlen(Extra) <= 77the length is negative andmakepanics withruntime: makeslice: len out of range. The v1verifyHeaderalready validates this length before decoding, but the shared helper does not, so a shortExtrareaching the helper crashes instead of returning no masternodes.This adds the missing length guard to both engines: when
Extrais shorter thanExtraVanity+ExtraSeal, the helper returnsnil(no masternodes) rather than panicking. A well-formedExtrastill decodes exactly as before. Fixes #2362.Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
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) thatAdded
engine_v1unit tests covering a shortExtra(no panic, returns nil) and a well-formedExtra(correct masternodes). Behavior is unchanged for valid inputs, so this is backwards compatible; the private-network / co-existence / docs items are N/A for a defensive nil guard on an internal decode helper.Summary by CodeRabbit
Bug Fixes
Tests