harden(v16): derive the array-length constants from the asset count (M-1, #160) - #185
Conversation
…M-1, dcccrypto#160) M-1 warns that the vendored portfolio mirror is only checked against itself, so engine drift can mask itself as a compile pass. I compared the mirror to the live engine field-for-field and it is currently CORRECT -- but the comparison surfaced one concrete instance of exactly that vector. V16_ACTIVE_BITMAP_WORDS was the literal `1`, with the formula only in its doc comment, while the engine derives the same quantity as `(V16_MAX_PORTFOLIO_ASSETS_N + 63) / 64`. Both are 1 today at 16 assets. If the engine ever raises the asset count above 64, its bitmap grows to two words while a literal `1` here stays put -- silently shifting `legs` and every field after it, including the stale_state / b_stale_state / liquidation_lock flags the transfer gate reads. That is M-1's stated failure mode, reachable by a single upstream constant change. The size assertions would not catch it, which is the point of the issue: they compare the mirror against itself, so whoever raised the count would update EXPECTED_PORTFOLIO_ACCOUNT_SIZE to whatever the wrong layout computed and every assertion would pass. PORTFOLIO_SOURCE_DOMAIN_CAP was already derived (`2 * V16_MAX_PORTFOLIO_ASSETS_N`); this closes the asymmetry. Added as const assertions rather than #[test]s so every build checks them -- including cargo build-sbf, and any environment where the dev-dependency graph cannot be built. Verified they bite: hardcoding the bitmap back to a literal fails the build on the new assertion. `div_ceil` rather than the engine's `(N + 63) / 64` because clippy rejects the manual form under -D warnings; the comment records that the engine writes it the other way so the two can still be diffed by eye. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesV16 bitmap capacity
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change preserves the current account layout and transfer-control behavior while deriving the bitmap length from the asset capacity and adding compile-time safeguards against future layout drift. No actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution timed out 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 |
…ill exists (#187) README deferred runtime layout validation to "#110H". #110 is CLOSED — it was a batch of LOW/INFO findings — so the deferral was documented as tracked somewhere that no longer tracks it. Now points at #160, which is open and is the issue that raised this. Small, but this is the failure mode where a known gap stops being known: the doc says "deferred, see X", a reader checks X, finds it closed, and concludes the work was done. Also states plainly what the const_assert!s cannot do, because the reason is sharper than "they only check internal consistency". Under the exact change they exist to guard against — raising the asset count — whoever made it would update EXPECTED_PORTFOLIO_ACCOUNT_SIZE to whatever the wrong struct computed, and the assertion would pass. It is not weak evidence; in that scenario it is guaranteed to pass. #185 (merged today) removed one concrete instance of that vector by deriving V16_ACTIVE_BITMAP_WORDS from the asset count. The general case still needs a real portfolio account decoded at runtime, which is what #160 stays open for. The three false LiteSVM/compile-time claims #160 reported are already corrected in both README.md and src/slab_types_v16.rs — verified, no LiteSVM reference remains anywhere in the repo. nft suite: 11 + 6 + 5 + 5 + 4 passing, 0 failed. Refs: #160 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>
Addresses M-1 (#160).
What I checked, and the result
M-1's core point is that the
const_assert!s prove the mirror is internally consistent and prove nothing about correspondence to the live engine — so drift "can mask itself as compile-pass". Nobody had actually done that correspondence check, so I did it against the engine source (percolator/src/v16.rs):PortfolioAccountV16Accounttop-level fieldsProvenanceHeaderV16AccountPortfolioLegV16AccountPortfolioSourceDomainV16AccountHealthCertV16AccountCloseProgressLedgerV16AccountResolvedPayoutReceiptV16AccountThe mirror is currently correct. The risk M-1 describes is real but not presently realised.
One near-miss worth recording: my first pass appeared to show
PORTFOLIO_SOURCE_DOMAIN_CAPas engine=4 vs mirror=2. That was wrong twice over — the engine's4is#[cfg(kani)]-only, and my pattern had truncated2 * V16_MAX_PORTFOLIO_ASSETS_Nat the2. Both sides use the identical expression. Flagging it because a careless version of that check would have produced a false CRITICAL.The one real instance of M-1's failure mode
V16_ACTIVE_BITMAP_WORDSwas the literal1, with the formula only in its doc comment. The engine derives the same quantity as(V16_MAX_PORTFOLIO_ASSETS_N + 63) / 64.Both are
1today at 16 assets. If the engine ever raises the asset count above 64, its bitmap grows to two words while a literal1here stays put — silently shiftinglegsand every field after it, including thestale_state/b_stale_state/liquidation_lockflags the transfer gate reads. That is precisely M-1's "wrong lock/stale flags → bypass transfer restrictions", reachable by a single upstream constant change.And the size assertion would not save us, which is M-1's argument exactly: whoever raised the count would update
EXPECTED_PORTFOLIO_ACCOUNT_SIZEto whatever the wrong layout computed, and every assertion would pass.PORTFOLIO_SOURCE_DOMAIN_CAPwas already derived — this closes the asymmetry.Fix
Derive it, and assert the derivation:
Const assertions rather than
#[test]s so every build checks them — includingcargo build-sbf, and any environment where the dev-dependency graph cannot be built. That last point is not hypothetical:cargo testcannot run on my machine at all (openssl-sysfinds no OpenSSL on Windows), whilecargo build --libchecks these fine.div_ceilrather than the engine's(N + 63) / 64because clippy rejects the manual form under-D warnings; the comment records that the engine writes it the other way, so the two can still be diffed by eye.Verification
cargo build --libclean;cargo clippy --lib -- -D warningsclean.2fails the build on exactly this new assertion, rather than passing silently.cargo test(OpenSSL, above), which is why these are const assertions; the compile-time checks they replace are strictly stronger than the test would have been.On the rest of M-1
The false-claim half is already fixed —
cf56ba5("M-1 docs") removed the LiteSVM assertions fromcpi_v16.rsandslab_types_v16.rs, andREADME.md:128-130now states the honest position, naming the deferred runtime validation and pointing at #110H. So option 1 is done; this PR narrows the residual risk that option 2 was meant to cover, without pretending to replace it.🤖 Generated with Claude Code
Summary by CodeRabbit