fix(stake): decode v4 (408-byte) StakePool; keep SIZE aliases on deployed v3 - #382
Open
0x-SquidSol wants to merge 1 commit into
Open
fix(stake): decode v4 (408-byte) StakePool; keep SIZE aliases on deployed v3#3820x-SquidSol wants to merge 1 commit into
0x-SquidSol wants to merge 1 commit into
Conversation
…oyed v3 percolator-stake d0c6ecb promoted the dcccrypto#242 cooldown timelock out of `_reserved` (392 -> 408). A 408-byte pool satisfies `length >= STAKE_POOL_SIZE_V3`, so the decoder concluded v3, expected version 3, read 4, and threw `StakePool unsupported version: 4 !== 3`. Every stake read fails at once the moment the stake program redeploys. Add STAKE_POOL_SIZE_V4 = 408 and a v4 arm. The two timelock fields are read version-aware, not simply relocated: on v3 they genuinely live in `_reserved[10..26]` aliased with PERC-313 HWM state, so reading the v4 offsets would misreport all 25 pools live on devnet today. Expose `StakePoolState.version` (previously read and discarded). Field meaning depends on it, not just offsets: on v3 `cooldownProposedAtSlot` can read non-zero where no proposal exists, which is the interface's own documented "live proposal" sentinel, and callers had no way to tell. Deliberately NOT re-pointing STAKE_POOL_SIZE / STAKE_POOL_CURRENT_VERSION. The decoder change is a strict superset; the aliases are a swap. Consumers use STAKE_POOL_SIZE as an exact getProgramAccounts({ dataSize }) filter and as a `data.length < SIZE` gate, so 408 would match zero live pools. They flip with the coordinated stake + wrapper deploy. Also correct specs/stake-parity.json to 408: the fixtures mirror percolator-stake source, not the deployed program, so 392 was drift. The deployed layout is pinned separately as a literal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses items 1 and 1b of #381. Leaves the rest of that issue open — it bundles several other drift items.
Problem
percolator-staked0c6ecb— "promote #242 cooldown timelock out of_reserved(v4, 392 -> 408)" — is a security fix. On the deployed v3 layout,pending_cooldown_slotsandcooldown_proposed_at_slotwere packed into_reserved[10..26], which PERC-313 HWM state already owned. Proposing a cooldown flippedhwm_enabledoff and rewrotehwm_floor_bps; an HWM refresh rewrote the proposal slot so the timelock read as long-elapsed — a bypass of the delay that protects stakers from the admin. v4 gives both fields real struct slots at absolute 392 and 400.The SDK is pinned to v3/392. A 408-byte pool satisfies
data.length >= STAKE_POOL_SIZE_V3, sodecodeStakePoolconcludes v3, expects version 3, reads 4, and throws:Every stake read dies at once. Not broken today — devnet
GCHhcgw…holds 25 pools, all 392 bytes / version 3 — but both programs'mainare already on v4 (percolator-stakestate.rsconst-assertsSTAKE_POOL_SIZE == 408;percolator-progv16_program.rspinsSTAKE_POOL_LEN = 408/STAKE_POOL_VERSION = 4), so it fires on the next stake redeploy.The second half fails silently: reading the cooldown fields at the old
_reservedoffsets on a v4 pool returns HWM state as cooldown state. No throw, just wrong numbers.Fix
The timelock fields are read version-aware, not simply relocated. On v3 those bytes genuinely are the aliased ones — that is what the deployed program writes — so relocating unconditionally would misreport every pool that exists today.
isV4reads 392/400; v3-and-earlier keep reading_reserved[10..18]/[18..26], bug-for-bug.StakePoolState.versionis now exposed. The decoder already read the version byte and threw it away. Field meaning depends on it, not just offsets: on a v3 pool with HWM configured — i.e. what those 25 live pools look like —cooldownProposedAtSlotdecodes non-zero with no proposal ever made, which is this interface's own documented "live proposal" sentinel. A caller had no way to tell. Now: gate onversion >= 4. A test reproduces exactly that scenario.The one deliberate omission
STAKE_POOL_SIZEstays 392 andSTAKE_POOL_CURRENT_VERSIONstays 3.I originally flipped both. Two reviewers independently pushed back, and they were right. The decoder change is a strict superset — it widens what decodes and changes no existing pool's meaning. The aliases are a swap, and they are bare numbers consumers use as an exact
getProgramAccounts({ dataSize })filter and as adata.length < SIZEgate. Inpercolator-launch:app/app/api/stake/pools/route.ts:191—filters: [{ dataSize: STAKE_POOL_SIZE }]; at 408 this matches zero accounts and the endpoint returns{ pools: [] }.:132—if (data.length < STAKE_POOL_SIZE) return null;rejects every deployed pool.app/__tests__/hooks/useStakePool.size-cutover.test.ts:23assertsSTAKE_POOL_SIZE === 392.No consumer breaks today (both pins are
^4.3.0, and a caret cannot cross a major), but the aliases would be wrong about the chain the moment anyone bumped. They flip with the coordinated deploy.STAKE_POOL_SIZE_V4is available for anyone who wants the new layout explicitly.specs/stake-parity.json392 -> 408I had left this alone believing it described the deployed program. It doesn't:
scripts/update-parity-fixtures.mjsgenerates it withcargo run --bin sdk_parity_fixturesagainst the sibling repo, andparity-gate.ymlchecks that repo out with noref:— i.e.main, which isd0c6ecb/408. So392was drift. The deployed layout is now pinned separately as a literal, so it stays guarded. The_reservedoffset assertions need no change:state.rsconst-asserts_reserved == 320, and v4 appended rather than recarving.Heads-up on CI: the Parity Gate fires on PRs touching
src/solana/stake.ts, and it cannot pass —sdk_parity_fixturesdoes not exist inpercolator-stake(no[[bin]], nosrc/bin/, nothing in its history); onlypercolator-matchhas one. This is pre-existing, not caused by this PR, but this PR is what makes it visible. The fixture was hand-edited, asccd50f9also did.Also included
Doc corrections the change made necessary. The large
_reservedbyte-aliasing block described the exact bug v4 fixes as unconditional present-tense reality; it is now scoped "v3 and earlier ONLY; FIXED in v4", with the original report retained because it still describes every pool on devnet.totalRecoveredFromWrapperwas labelled "v3 only" but is v3 and v4. I also removed a dangling docblock my own first draft orphaned above the V4 constant.The v4 timelock reads are expressed as
reservedStart + 72/+ 80rather than bare392/400, so they share a base with every other post-_reservedfield. Anoffbug would then shift both halves together instead of making them disagree — which matters, because this PR adds a comment warning about precisely that failure (omitting a version from thepending_admingate leavesoffat 288 and shifts everything after_reservedby 32). I hit that bug while writing this change; the widenedisV2 || isV3 || isV4gate is the fix.Verification
npx tsc --noEmitclean;npx vitest run-> 29 files, 1011 passed, 31 skipped, 0 failed.origin/main.origin/mainis 408/v4.Sequencing
Merge is safe now —
ci.ymlpublishes only on arelease:-prefixed commit, so a normal merge exposes no consumer. Publish with the deploy, not before:percolator-progpins v4 exclusively and rejects 392-byte pools on length before reading the version byte, andpercolator-stakehas no migration path, so the deploy is a flag day that re-seeds all 25 pools. Worth a follow-up onpercolator-launchforroute.ts:132,191so the flag day is already handled there.🤖 Generated with Claude Code