test: pin the ledger-entry ceiling that makes MAX_HISTORY_ENTRIES unreachable - #848
Conversation
…eachable MAX_HISTORY_ENTRIES is documented as preventing storage DoS by capping the history vector, but it cannot be reached. A SpendingLimitData holding 816 entries serializes to 65,592 bytes, past the mainnet contractDataEntrySizeBytes limit of 65,536, so enforce fails with an untyped host budget error and HistoryCapacityExceeded is never returned. The measured ceiling is 815. This is visible in the existing suite: enforce_history_capacity_exceeded reaches 1000 only because it calls disable_resource_limits(). That call is a normal idiom here, used in eight places across four files for tests that deliberately exceed mainnet limits, so this is not a claim that the test is wrong. It does mean no test currently covers the interaction between the constant and the platform limit. Adds two tests that pin both sides of the boundary under mainnet defaults: filling to 815 succeeds and leaves the guard untouched, and 816 fails with Error(Budget, ExceededLimit). Documents the ceiling on the constant. The constant itself is left at 1000. Lowering it would be a behavioural change for accounts currently holding more than the new value in-window, and the right value is a judgement about headroom that belongs to the maintainers. The open question is in the linked issue. Note the second test asserts a host budget error rather than a contract error code, so it does not use the usual Error(Contract, #NNNN) form. stellar-accounts: 183 passed.
|
Warning Review limit reached
Next review available in: 56 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
1 participant
Fixes #847
What this does
Documents the ledger-entry ceiling that makes
MAX_HISTORY_ENTRIESunreachable, and adds two tests that pin both sides of it.It does not change the constant. The issue explains why: lowering it is a behavioural change for accounts holding more than the new value in-window, and on an audited security module the headroom judgement is yours. The open question is in the issue.
The measurement
A
SpendingLimitDataholding 816 entries serializes to 65,592 bytes, past the mainnetcontractDataEntrySizeBytesof 65,536. Measured ceiling: 815. Soenforcefails withError(Budget, ExceededLimit)185 entries beforeHistoryCapacityExceededcould be returned.The tests
spending_history_ledger_entry_ceilingfills to 815 through the realenforcepath under mainnet defaults and asserts the history holds 815.spending_history_one_past_the_ceiling_exceeds_the_ledger_entry_limitgoes one further and expects the host rejection.The second one asserts
Error(Budget, ExceededLimit)rather than anError(Contract, #NNNN)code, so it does not follow the usual panic-test form incode-quality.md. That is deliberate: the failure genuinely is a host budget error, and that is the point being pinned. Say the word if you would rather it were shaped differently.Neither test touches
enforce_history_capacity_exceeded. Itsdisable_resource_limits()call is a normal idiom here, used in eight places across four files, and I am not claiming it is wrong.Checks
cargo +nightly fmt --all -- --checkcleancargo +stable clippy --release --locked --all-targets -- -D warningsexit 0cargo test -p stellar-accountspasses: 183 tests, 181 before plus these two