# PR #350 Final Validation Report

**Branch:** fix/snapshot-restore-pbss-v38
**Target:** origin/xdc-network
**Commit Range:** 5e6699d..4ded6a7
**Date:** 2026-04-20
**Validator:** Senior Ethereum/go-ethereum Engineer (Claude Opus 4.7)

---

## 1. Executive Summary

**VERDICT: READY TO MERGE**

PR #350 correctly fixes two independent issues:
1. **PBSS snapshot restore failure (#308)** -- GP5 computes a divergent state root from
   the block header; the fix ensures `triedb.Commit()` uses the cached GP5 root
   during checkpoint commits so trie nodes are actually persisted to disk.
2. **Sync correctness (#311, #312)** -- Fixes a category error where peer
   total difficulty (TD, a `*big.Int`) was compared against local block number.
   Also gates `synced.Store(true)` on actual download progress.

Both changes are minimal, well-scoped, and align with upstream geth v1.17
patterns.

---

## 2. Build and Static Analysis

| Check | Result |
|-------|--------|
| `go build ./core/` | PASS (clean compilation) |
| `go build ./eth/` | PASS (clean compilation) |
| `go vet ./core/ ./eth/` | **FAIL** -- pre-existing, NOT from this PR |

`go vet` failure:
```
core/xdc_state_root_cache_test.go:16: cannot use xdcStateRootCache (variable of type map[uint64]common.Hash) as *lru.Cache[uint64, common.Hash] value
```

This reproduces on `origin/xdc-network` and is **not touched** by PR #350.
The two modified files (`blockchain.go`, `sync_xdc.go`) vet cleanly.

---

## 3. Diff Scope Confirmation

```
git diff origin/xdc-network..fix/snapshot-restore-pbss-v38 --stat
 core/blockchain.go | 13 +++++++++----
 eth/sync_xdc.go    | 36 ++++++++++++++++--------------------
 2 files changed, 25 insertions(+), 24 deletionsions(+)
```

Exactly 2 files changed, matching the intended fix set precisely.
No scope creep.

---

## 4. Fix #308 -- core/blockchain.go (P1 Checkpoint Trie Commit)

### Before (bug)
```go
if err := bc.triedb.Commit(root, false); err != nil {
```

Where `root` is the block header root. On XDC/GP5, the computed state root
**differs** from `header.Root`, so trie nodes are committed under the wrong
hash and are not findable later by `XdcGetCachedStateRoot()`.

### After (fix)
```go
commitRoot := root
if cachedRoot, ok := XdcGetCachedStateRoot(current); ok {
    commitRoot = cachedRoot
}
if err := bc.triedb.Commit(commitRoot, false); err != nil {
```

### Upstream Alignment
- `triedb.Commit(root, bool)` API call is **identical** to upstream v1.17.2
- Upstream passes `header.Root` because on Ethereum `computed == header`.
- Our divergence is **necessary and bounded**: only for chainID 50/51, and
  follows the identical fallback pattern already used at lines 482, 1520,
  1907, and 2200 in the same file (Stop(), writeBlockWithState GC paths).

### Risk Assessment
- **Minimal**: additive fallback; if `XdcGetCachedStateRoot()` returns `ok==false`,
  behavior reverts to original (broken-but-known state).
- **No regression for non-XDC chains**: entire block is guarded by `chainID == 50 || 51`.

---

## 5. Fix #311/#312 -- eth/sync_xdc.go (Sync Correctness)

### Before (bug #312): Category Error
```go
ourBlockNum := new(big.Int).SetUint64(currentBlock.Number.Uint64())
if peerTd.Cmp(ourBlockNum) <= 0 {
    peerTd = new(big.Int).SetInt64(1 << 62)
}
```

Comparing peer TD (cumulative difficulty, `*big.Int`) against local block
number (arithmetic `uint64` promoted to `*big.Int`) is semantically meaningless.

### After (fix #312): Block Number Comparison
```go
peerHeadNum := uint64(0)
if peerHeadHeader := s.handler.chain.GetHeaderByHash(peerHead); peerHeadHeader != nil {
    peerHeadNum = peerHeadHeader.Number.Uint64()
}
if peerHeadNum > 0 && peerHeadNum <= ourBlockNum {
    log.Debug("XDC sync: peer head <= local head, skipping", ...)
    return
}
```

This resolves `peerHead` hash to its block number via `GetHeaderByHash` and
compares block numbers to block numbers -- the correct metric for sync progress.

### Before (bug #311): Unconditional Synced Flag
```go
s.handler.synced.Store(true)  // ALWAYS set, even on failure
```

### After (fix #311): Progress-Gated Synced Flag
```go
if totalProgress > 0 {
    // ... update lastSyncBlock ...
    s.handler.synced.Store(true)
} else {
    s.recordPeerBackoff(peer.ID())
}
```

### Upstream Alignment
- Upstream v1.17.2 sets `synced` via `enableSyncedFeatures()` callback only
  when the downloader genuinely completes. Our fix now matches that semantics.
- The `peerHeadNum > 0` guard handles the case where a peer advertises a head
  hash we don't have in our local header chain (e.g., during initial startup).

### Risk Assessment
- **Minimal**: fixes a clear bug; no other codepath depends on the old
  meaningless TD/num comparison.
- `totalProgress > 0` is a conservative gate: even 1 block of progress means
  the downloader connected and ran, so `synced` is reasonable.

---

## 6. Pattern Consistency Audit

Searched all `XdcGetCachedStateRoot` call sites in the repo:

| File | Line | Pattern |
|------|------|---------|
| core/blockchain.go | ~482 | `if root, ok := XdcGetCachedStateRoot(...); ok { ... }` |
| core/blockchain.go | ~1520 | `if root, ok := XdcGetCachedStateRoot(...); ok { ... }` |
| core/blockchain.go | ~1907 | `if root, ok := XdcGetCachedStateRoot(...); ok { ... }` |
| core/blockchain.go | ~2200 | `if root, ok := XdcGetCachedStateRoot(...); ok { ... }` |
| **core/blockchain.go** | **~1936** | **NEW -- identical pattern** |

The new call site is consistent with all existing usages in the codebase.

---

## 7. Upstream Changelog Review (v1.17.0 to v1.17.2)

No PBSS, trie, or snapshot-related fixes were introduced in this repo between
those tags. The v1.17.2 release consists of a version bump only.

No additional cherry-picks are required for this merge window.

---

## 8. Known Pre-existing Issues (NOT blocking)

1. `core/xdc_state_root_cache_test.go` has a compilation error (`map[uint64]common.Hash`
   vs `*lru.Cache[uint64, common.Hash]`). This exists on `origin/xdc-network` and
   should be fixed in a follow-up PR.

---

## 9. Merge Recommendation

**MERGE PR #350 into `xdc-network` branch.**

After merge:
1. Build Docker image v39 from merged `xdc-network`
2. Deploy to xdc07 production node
3. Restart with fixed binary
4. Let node run to take a fresh cold snapshot
5. Test restore of fresh snapshot to validate end-to-end
6. Once validated, clean up corrupted v37 snapshots from `/mnt/data/snapshots/`

---

*Report generated by Claude Opus 4.7 / 2026-04-20*