fix(security,#433): reconcile pre-existing ledgerless backing on top-up - #456
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5a79e60a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| write_or_init_backing_domain_ledger(data, ledger, *initialized)?; | ||
| } | ||
| } | ||
| if amount == 0 { |
There was a problem hiding this comment.
When a legacy ledgerless domain is already in Resolved mode, this migration branch is unreachable because the shared preflight rejects every non-Live market at line 10537. WithdrawBackingBucket explicitly supports mode 1 but now requires the ledger account, so backing in such a market cannot be reconciled or withdrawn. Permit this zero-capital initialization in the resolved wind-down state, or provide an equivalent ledger-creation path during withdrawal.
Useful? React with 👍 / 👎.
| ledger.total_principal_atoms = gross_principal_atoms; | ||
| ledger.cumulative_loss_atoms = unavailable_atoms; | ||
| ledger.cumulative_recovery_atoms = 0; |
There was a problem hiding this comment.
When a legacy ledgerless bucket already has nonzero utilization_fee_earnings, this migration reconstructs principal and impairment but leaves total_earnings_atoms at zero while read_or_new_backing_domain_ledger has already pinned the earnings watermark to the bucket's current value. Subsequent syncs therefore never recognize those earnings; withdrawing them instead increments total_earnings_withdrawn_atoms above total earnings and leaves the canonical ledger inconsistent. Seed the currently outstanding earnings as part of the migration baseline.
Useful? React with 👍 / 👎.
|
Merged. Reviewed against the merge result rather than the diff, and the tests pass once the environment is right. The fix
Two things in it are the reason this merged without changes: Seeding principal and impairment together, not principal alone:
Pinning the watermark to the same snapshot, so the first normal sync cannot book the migration baseline as a newly observed loss — and then re-baselining after the top-up, because a refill that satisfies provider receivable would otherwise read as a recovery. That is a double-count in each direction, and both are closed. Keeping Two things I checked that looked wrong and were not1. The diff appears to delete three tests — 2. Both of this PR's new tests FAILED locally — After That is worth flagging for anyone else running these locally — a stale Suite
CI's Merged, not deployed. Pending release. |
2 participants
Summary
Follow-up for #433.
This PR reconciles backing-domain accounting for pre-existing funded domains whose canonical
BackingDomainLedgerdid not exist yet whenTopUpBackingBucketfirst creates it.The creation/existence half is already handled on the top-up path. The remaining accounting gap was that a newly-created ledger started with:
even when the engine bucket already contained historical backing.
That made the canonical ledger authoritative only for deposits observed after its creation, rather than for the provider's actual outstanding principal.
This PR reconstructs the migration baseline from the existing engine bucket before applying the new top-up.
Problem
For a legacy funded domain with no canonical backing ledger, the bucket can already contain principal across:
Creating the canonical ledger alone is therefore not sufficient.
Example:
Without reconciliation, the newly-created ledger would account for only the new
20, even though the engine bucket already represents historical principal.There is also an accounting constraint:
Because of that, historical principal cannot safely be seeded by itself.
Historical consumed/impaired backing must also be represented as unavailable principal. Otherwise, creating the ledger late could incorrectly convert previously consumed or impaired backing into withdrawable principal.
Implementation
The reconciliation is intentionally migration-specific to
TopUpBackingBucket.It does not change the generic behavior of:
That helper is also used by LP-vault accounting paths where an uninitialized ledger can legitimately represent zero contribution for a domain that has never funded the vault.
For a newly-created/uninitialized ledger on the top-up path, the migration baseline is reconstructed from the pre-top-up bucket snapshot:
using the existing
BOUND_SCALEconversion semantics.The migrated ledger is seeded as:
This preserves:
as the bucket's currently recoverable principal at migration time.
Consumed/impaired backing therefore does not become newly available simply because the canonical ledger was initialized late.
Existing Ledgers Stay on the Normal Sync Path
The migration applies only when the canonical ledger is still uninitialized.
Existing initialized ledgers continue through the normal synchronization path:
This keeps the reconciliation narrowly scoped to legacy ledger initialization and avoids changing accounting semantics for already-established ledgers.
Non-Zero Top-Up Handling
For a legacy ledgerless domain receiving new capital, reconciliation occurs against the bucket before the engine top-up mutation.
The sequence is:
The post-top-up watermark adjustment is important.
A top-up can refill
provider_receivableand reduce consumed backing. That reduction represents new capital, not recovery of historical loss.Without re-baselining the unavailable-principal watermark after the refill, the next normal synchronization could incorrectly book that reduction a second time as:
This PR prevents that double-booking.
Zero-Capital Migration Path
TopUpBackingBucket(amount = 0)can also reconcile a still-ledgerless funded domain without requiring the provider to contribute additional capital solely to trigger migration.The zero-amount path performs:
No engine backing-deposit mutation is performed for
amount == 0.This provides an explicit migration trigger for historical backing without requiring a non-zero deposit.
Regression Coverage
Two compiled-BPF regressions were added for the legacy ledgerless state.
1. Zero-Top-Up Migration + Withdrawal
Fixture:
The test verifies that a zero-amount top-up:
100;40unavailable amount as the loss baseline;60principal to be withdrawn;2. Non-Zero Top-Up Does Not Become Recovery
Starting from:
a new top-up of
20can refill provider receivable.The regression verifies that after reconciliation:
and that a subsequent normal
SyncBackingDomainLedgerdoes not book the new top-up again as historical recovery.Non-Vacuous Harness Coverage
The legacy fixtures deliberately avoid the
V16CuEnvbacking helpers that route through:Those helpers
set_accountthe canonical ledger into existence and would make the migration proof vacuous.Instead, the legacy backing state is injected directly while keeping the related engine consumption counters coherent, including:
The production
TopUpBackingBucketpath then performs the normal engine validation before committing the migrated state.The relevant instructions are exercised against compiled program bytes.
Watched Negative Control
A watched negative control was performed to verify that the regression actually observes the reconciliation logic.
Procedure:
The negative-control build failed exactly at:
The known-good source was then restored byte-for-byte, rebuilt, and the positive regression passed again.
This confirms that the test is observing the migration itself rather than passing because the harness silently pre-created the canonical ledger.
Verification
Compiled SBF / LiteSVM verification:
The existing no-LP-vault top-up → withdrawal proof-of-life remains green.
Full
v16_curun after building the siblingpercolator-matchBPF:The only failure was:
with the transaction exhausting the
1,400,000CU meter.The same test was reproduced against a clean
upstream/mainworktree at the PR base using the same SBF toolchain and matcher binary.The clean upstream baseline failed identically.
Therefore, that CU-bound
TradeNoCpifailure is pre-existing baseline behavior and is not introduced by this backing-ledger reconciliation change.Scope
Changed files:
This PR does not change:
read_or_new_backing_domain_ledgersemantics;The patch is intentionally limited to the legacy backing-ledger reconciliation lifecycle and its regression coverage.
Deployment-Window Note
There is one state this PR intentionally does not attempt to infer.
If a domain:
the current bucket alone does not safely identify how much of the initialized ledger is historical versus legitimately observed after initialization.
This PR therefore reconstructs the accounting baseline only for still-uninitialized legacy ledgers.
It does not heuristically rewrite an already-initialized ledger.
If any initialized-under-seeded domains exist in that deployment window, they should be identified explicitly and handled through a bounded one-time repair/migration path rather than inferred from current bucket state.
Relation to #433
This implements the legacy-reconciliation follow-up discussed in #433:
I am intentionally using
Refs #433rather than automatically closing the issue, pending maintainer/ops confirmation on the initialized-under-seeded deployment-window case described above.Refs #433