# PR #350 Validation Report
**Branch**: `fix/snapshot-restore-pbss-v38`  
**Base**: `origin/xdc-network`  
**Date**: 2026-04-20  
**Validator**: CLI Agent

---

## Executive Summary

PR #350 contains **3 changes** addressing issues #308, #311, and #312.  
**Change 1 (blockchain.go) is correct and ready.**  
**Changes 2 and 3 (sync_xdc.go) contain a compile-blocking bug and two logic bugs that must be fixed before merge.**

---

## Change 1: core/blockchain.go — P1 checkpoint trie commit (#308)

### Validation: ✅ CORRECT

**What it does**: During bulk sync checkpoint commits (every 900 blocks on XDC chains), the code now uses `XdcGetCachedStateRoot(current)` instead of the header `root` from the triegc queue. This ensures trie nodes stored under GP5's computed root are actually persisted.

**Why it's correct**:
1. **Pattern consistency**: The codebase already uses `XdcGetCachedStateRoot` in 4 other places in `blockchain.go`:
   - Line ~482: `Stop()` — head state validation on restart
   - Line ~1520: `writeBlockWithState` GC loop — recent trie commits
   - Line ~1907: `writeBlockWithState` memory limit flush
   - Line ~2200: `insertChain` — parent root for block processing
   
2. **Root cause match**: GP5 computes different state roots than v2.6.8 due to uint256/BigBalance encoding. Trie nodes are stored under GP5's root, so committing the header root causes `hashdb.Commit` to silently no-op.

3. **Upstream alignment**: The change is guarded by `chainID == 50 || chainID == 51`, keeping it XDC-specific. The `triedb.Commit(root, false)` API matches upstream go-ethereum v1.17.

4. **Build verified**: `go build ./core/` passes cleanly.

---

## Change 2: eth/sync_xdc.go — Peer TD comparison fix (#312)

### Validation: ❌ BLOCKING BUGS

**Intent**: Replace the incorrect TD-vs-block-number comparison with a proper block-number-vs-block-number comparison.

**Bugs found**:

#### Bug 2A: Compile error — `currentBlock` used before declaration
```
vet: eth/sync_xdc.go:206:17: undefined: currentBlock
```

The PR places `ourBlockNum := currentBlock.Number.Uint64()` **before** `currentBlock := s.handler.chain.CurrentBlock()`.

**Fix**: Move the `currentBlock` fetch and nil-check to the top of the function, before `ourBlockNum`.

#### Bug 2B: Logic error — peers with unknown heads are incorrectly skipped

```go
peerHeadNum := uint64(0)
if peerHeadHeader := s.handler.chain.GetHeaderByHash(peerHead); peerHeadHeader != nil {
    peerHeadNum = peerHeadHeader.Number.Uint64()
}
// ... later ...
if peerHeadNum <= ourBlockNum {
    return  // SKIPS PEER
}
```

If `GetHeaderByHash(peerHead)` returns `nil` (we don't have the peer's head block locally), `peerHeadNum` stays `0`. The comparison `0 <= ourBlockNum` is always true, so **the peer is skipped even though it is ahead of us and we need to sync from it**.

**Fix**: Only skip when `peerHeadNum > 0 && peerHeadNum <= ourBlockNum`. When the peer's head is unknown, fall through to allow sync.

---

## Change 3: eth/sync_xdc.go — Synced flag fix (#311)

### Validation: ❌ LOGIC BUG

**Intent**: Only mark the node as `synced` when sync actually makes progress.

**Bug**: The new code only sets `synced` when `bestPeer()` returns a peer AND `ourHead+32 >= bestHeadNum`. If `bestPeer()` returns `nil` (e.g., all peers' heads are now known, or only one peer exists), `synced` is **never** set even after successful progress.

**Impact**: After syncing with the only available peer, the node would remain "unsynced" forever, rejecting transactions and not enabling post-sync features.

**Recommended fix**: Simplify to the core intent of #311 — only avoid marking synced when zero progress was made. When progress > 0, mark synced (matching original behavior and upstream downloader completion semantics).

---

## Upstream Alignment Assessment

### go-ethereum v1.17 baseline
The repo is based on **v1.17.0** (`Major=1, Minor=17, Patch=0`).

### Upstream v1.17.0 → v1.17.2 changes
Reviewed upstream commits via GitHub API. The majority of v1.17.1/v1.17.2 commits are **post-merge/EIP-7928 (Amsterdam fork) features** (blobpool, beacon blsync, SLOTNUM opcode, precompile touching) which are **not applicable** to XDC (pre-merge chain).

**Potentially relevant upstream fixes** (should be evaluated for cherry-pick in a separate PR):
1. `eth/protocols/snap: restore peers to idle pool on request revert` — fixes snap-sync peer leak under low-peer conditions.
2. `eth: check for tx on chain as well` — prevents re-fetching already-on-chain transactions.
3. `core/types: fix transaction pool price-heap comparison` — txpool edge-case fix.
4. `trie/bintrie: fix endianness in code chunk key computation` — verkle trie fix (only if verkle is enabled).

**Not recommended to merge**:
- `eth/protocols/eth: drop protocol version eth/68` — XDC still uses legacy eth/66 or eth/67 protocols; dropping eth/68 could break compatibility.
- Most EIP-7928/Amsterdam fork commits — XDC does not implement this fork.

### Recommendation on upstream
Do **not** bulk-merge upstream v1.17.1/v1.17.2. Instead, create a tracking issue to audit and cherry-pick only the snap-sync and txpool bugfixes that are fork-agnostic.

---

## Proposed Fixes

See `PR350_fixes.patch` for the complete corrected diff. The patch:
1. Fixes the `currentBlock` compile error in `eth/sync_xdc.go`
2. Guards the peer-skip logic with `peerHeadNum > 0`
3. Simplifies the synced flag logic to only depend on `totalProgress > 0`

---

## Test Results

| Package | Result | Notes |
|---------|--------|-------|
| `go build ./core/` | ✅ PASS | Change 1 compiles cleanly |
| `go build ./eth/` | ❌ FAIL | `sync_xdc.go:206: undefined: currentBlock` |
| `go vet ./eth/` | ❌ FAIL | Same compile error |
| `go test ./eth/ -run TestXDC` | ❌ FAIL | Build failure blocks tests |

---

## Checklist

- [x] Change 1 technically correct
- [x] Change 1 follows existing XDC patterns
- [x] Change 1 geth 1.17 API compatible
- [ ] Change 2 compiles (BLOCKING)
- [ ] Change 2 logic correct (BLOCKING)
- [ ] Change 3 logic correct (BLOCKING)
- [x] Upstream v1.17.x reviewed
- [x] No unintended upstream divergence introduced

---

## Action Items

1. **Apply `PR350_fixes.patch`** to resolve compile and logic bugs in `eth/sync_xdc.go`
2. **Re-run tests**: `go build ./...` and `go test ./eth/ -run TestXDC`
3. **Create follow-up issue** to audit upstream v1.17.1/v1.17.2 for XDC-relevant bugfixes (snap peer idle pool, txpool price-heap)
4. **Merge PR** after patch application and CI pass