fix(#456): cover remaining legacy backing migration edge cases - #458
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
|
Your PR is not the cause of this red check. Clearing that up before anything else, since the failure looks damning and isn't yours.
The cause is in CI, not in anyone's PR: Once #459 lands, please rebase (or I'll re-run it) and the check should reflect your actual change. On the substance of #458 itself: it's a program |
… red since 15eb8b0 (#459) `main` has failed CI for three days, from 15eb8b0 (2026-08-31 13:31) through be8c8dd, on nine tag87_* tests. Fourteen commits have landed on a red trunk, and contributor PR #458 inherited the red and looks like it caused it. It did not. Neither did any of the fourteen. ROOT CAUSE `devnet` is not a default feature — `default = ["anchor-v2"]`. Without it, tag 87 (WithdrawInsuranceReserveToStake) compiles to an unconditional #[cfg(not(feature = "devnet"))] { return Err(PercolatorError::StakeProgramNotPinned.into()); } // Custom(60) because a non-devnet build has no pinned percolator-stake id to send tokens to. v17 percolator-stake has no mainnet deployment, so this is correct fail-closed behaviour of the program — not a bug in it. CI built the wrapper `--no-default-features`. So every tag87_* test failed at Custom(60) before reaching anything it asserts about, regardless of the stake pin. #441 removed those nine from tests/KNOWN_FAILING.txt as "they now pass". That was true where it was measured: a local `cargo build-sbf` takes the default features, and anyone testing tag 87 builds devnet. It could never be true in CI, and those nine had never once passed here. Note this is NOT the failure deployed-refs.env predicts for a wrapper/stake pin skew ("Custom(9) at tag 87"). It is Custom(60), and it is independent of the pins: the pins were advanced correctly by 78c2005 and main stayed red. WHY devnet IS THE RIGHT BUILD, not an allowlist restore The suite's sibling pins are all deployed-DEVNET refs, and the deployed devnet wrapper is itself built `--features devnet` — that is what reproduces DhSkE7u byte-for-byte. A non-default build tests a configuration deployed nowhere. It also meant the entire tag-87 security surface was never exercised, including `tag87_blocks_the_creator_forged_stake_pool_exploit` and `tag87_owner_pin_fires_before_any_pool_byte_is_read` — a test that exists precisely to prove the owner pin fires before any pool byte is read. Restoring the allowlist entries would have gone green while keeping that blind. MEASURED on be8c8dd, siblings at the pinned refs (stake d0c6ecb, nft 215842e, matcher d4d4f1c): --no-default-features passed=596 failed=30 9 NEW failures vs the allowlist --features devnet passed=603 failed=21 exact match: 0 new, 0 stale `./scripts/ci-test.sh` now exits 0 with its own verdict: "OK: failing set matches the allowlist exactly". Upstream aeyakovenko/percolator-prog carries neither scripts/ci-test.sh nor tests/KNOWN_FAILING.txt — both are ours, so there is nothing to coordinate. No test is edited, skipped or re-scoped, and no program source changes. Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D Co-authored-by: dcccrypto <dcccrypto@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
b3541b6 to
9ef25f9
Compare
|
Rebased onto Local verification after rebuilding the wrapper with
The previous CU-bound failure no longer reproduces with the correctly built devnet wrapper. No source changes were made as part of the rebase beyond the existing #458 follow-up. |
|
Merged — thank you, this was careful work and the writeup made it reviewable. Both changes were verified against source rather than accepted from the description, and each got an independent negative control run on current
Two notes for the record: Your change 1 is more severe than the PR describes. You framed the missing earnings baseline as a reconstruction gap. What made change 2 acceptable wasn't the new match arms on their own — it's that the
One habit worth changing: the PR body measures its diff against |
2 participants
Summary
Follow-up to #456.
This PR covers two remaining legacy backing-ledger migration edge cases identified during post-merge validation of #456.
It does not replace, reopen, or redesign the main #433 / #456 fix. The canonical backing-ledger creation and reconciliation path introduced by #456 remains intact.
This follow-up is intentionally limited to two migration cases:
utilization_fee_earningswhen its canonicalBackingDomainLedgeris first materialized;Resolvedmarket may still require a zero-capital reconciliation pass to materialize a missing canonical backing ledger before the remaining legacy backing can be withdrawn.Both cases were reproduced against the clean merged #456 baseline before applying this follow-up.
Context
PR #456 added migration-specific reconciliation for backing domains that were funded before their canonical
BackingDomainLedgerexisted.That migration correctly reconstructs historical principal and impairment from the existing backing bucket while preserving the generic zero-principal semantics of:
for unrelated callers such as LP-vault accounting.
Post-merge validation exposed two remaining lifecycle boundaries:
Resolvedstate.These are narrow follow-up cases around the migration lifecycle introduced by #456, not changes to its core accounting model.
1. Preserve Outstanding Pre-Ledger Backing Earnings
Problem
A legacy ledgerless backing bucket may already contain non-zero:
before its canonical ledger exists.
The #456 migration reconstructed historical principal and unavailable principal, but an already-existing earnings balance was not part of that reconstructed baseline.
At the same time, the newly materialized ledger could begin with its earnings observation watermark already aligned to the bucket's current earnings value.
That could produce a state equivalent to:
A subsequent normal sync would then observe no new earnings delta.
The pre-ledger earnings would remain present in the bucket, but would never have been represented in
total_earnings_atoms.Fix
The migration-specific baseline now snapshots outstanding provider earnings from the same bucket state used to reconstruct the rest of the legacy accounting baseline:
This behavior is deliberately contained inside:
rather than changing the generic semantics of:
The reconstructed baseline therefore starts from one coherent snapshot containing:
This prevents outstanding pre-ledger earnings from being skipped by an already-advanced watermark and prevents the first normal sync from re-booking the migrated snapshot.
2. Keep Zero-Capital Migration Reachable After Full Resolution
Problem
The migration path introduced by #456 still passed through the normal Live-only
TopUpBackingBucketlifecycle gate.A legacy domain could therefore reach:
with no remaining path to materialize the canonical ledger.
This matters because the canonical backing ledger is now authoritative for the backing-withdrawal path.
A fully wound-down market could still contain legitimate backing that predates its ledger, while reconciliation itself was no longer reachable.
The required operation in this state is not another backing deposit.
It is an accounting-only migration with:
Fix
TopUpBackingBucketnow distinguishes normal backing funding from the narrow migration-only terminal case.For normal Live markets, existing behavior remains unchanged and continues through:
For a
Resolvedmarket, zero-capital reconciliation is allowed only when all of the following are true:This represents an already wound-down market with no remaining materialized user portfolios or user capital.
The
amount == 0requirement makes the operation accounting-only.Non-zero backing deposits after resolution remain rejected.
In particular:
still fails closed.
Safety Boundaries
The resolved migration exception is intentionally narrow.
It does not:
read_or_new_backing_domain_ledger(...)behavior.Migration is allowed only when:
and the market is already fully wound down:
Its only purpose is to materialize canonical accounting for backing that already existed before the ledger became mandatory.
Accounting Invariants
The follow-up keeps the accounting rules established by #456 intact.
Principal and impairment remain seeded together
Historical gross principal is reconstructed from:
while historical unavailable principal is reflected in the migration loss baseline.
This preserves:
and avoids making previously consumed or impaired backing appear newly withdrawable.
Migration watermarks remain pinned to the reconstructed snapshot
The unavailable-principal watermark remains aligned with the same migration snapshot so the first normal sync does not re-book historical impairment as a newly observed loss.
The earnings watermark now follows the same rule for outstanding pre-ledger earnings.
Refill remains distinct from recovery
The #456 re-baselining behavior after top-up remains intact.
A top-up that satisfies provider receivable may reduce consumed backing, but that new capital is not treated as recovery of historical impairment.
Generic ledger initialization remains unchanged
The migration-specific reconstruction remains isolated from callers for which an uninitialized backing ledger legitimately means the vault has never funded the domain.
Regression Coverage
Two targeted regressions cover the remaining post-merge cases.
Outstanding backing earnings
The fixture recreates a historically funded ledgerless domain with:
The test verifies that migration materializes the canonical ledger with the expected historical accounting baseline, including the pre-existing earnings balance and matching earnings watermark.
It then exercises an earnings withdrawal and verifies that the ledger, backing bucket, watermark, vault balance, and withdrawn accounting remain coherent.
Fully resolved migration
The fixture recreates:
The regression first exercises the negative control:
and verifies that it remains rejected.
The rejected attempt does not:
The test then executes:
and verifies that canonical-ledger migration succeeds without introducing new capital.
The resulting ledger reflects the historical backing baseline rather than a newly fabricated deposit.
Existing legacy backing can then proceed through the canonical withdrawal path.
Baseline Reproduction
Both new regressions were reproduced against the clean merged #456 baseline before the follow-up was applied.
Baseline:
Before the follow-up:
With this follow-up:
Existing #456 migration regressions remain green, including:
Full
v16_cuVerificationFull
v16_cuon the follow-up branch:The remaining failure is:
The failure reaches the existing transaction compute-unit ceiling:
For comparison, the same test was rebuilt and run independently against the clean merged #456 baseline (
be8c8dd2).It fails there with the same failure class and the same 1,400,000-CU ceiling.
The remaining failure is therefore baseline-equivalent and is not introduced by this follow-up.
No changes to the unrelated TradeNoCPI / compute-budget path are included here.
Relationship to #456
This PR is intentionally a post-merge follow-up.
#456 remains the primary reconciliation fix.
This follow-up only completes two remaining boundaries around it:
The core #456 design remains unchanged.
Scope
Production code:
Regression coverage:
Final diff against
upstream/main:git diff --check upstream/main...HEADis clean.No unrelated source files are included.
No ABI / Layout Change
This follow-up does not change:
BackingDomainLedgerlayout;WithdrawBackingBucketABI;read_or_new_backing_domain_ledger(...)semantics;The production change is limited to migration-baseline initialization and the migration-specific lifecycle gate.
Branch State
Branch:
Head:
Commit:
The working tree is clean and the branch contains only:
relative to
upstream/main.Result
After this follow-up:
This completes the remaining post-merge migration edge cases discovered while validating #456 without widening normal backing-deposit behavior or changing the core reconciliation design.