Skip to content

fix(pm): couple closed-market retention to the commit reveal deadline (validate bound) - #155

Merged
On1x merged 1 commit into
pmfrom
fix/pm-retention-reveal-coupling
Aug 13, 2026
Merged

fix(pm): couple closed-market retention to the commit reveal deadline (validate bound)#155
On1x merged 1 commit into
pmfrom
fix/pm-retention-reveal-coupling

Conversation

@chiliec

@chiliec chiliec commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary

Defensive hardening for the commit-reveal escrow path, found during a money-path re-audit of the dispute / commit-reveal / lazy-pool subsystems.

The gap

A commit's reveal deadline can fall up to (pm_batch_epoch_blocks + pm_reveal_window_blocks) blocks after the commit (pm_commit_bet), and its escrow is only refunded/forfeited by the reveal-forfeit cron at that deadline. gc_market deletes pm_commit rows unconditionally once a market has been finalized for pm_closed_market_retention_sec — with no status-0 refund.

Today this is safe purely by parameter magnitudes: the reveal deadline is ~11 min worst case, vs. a 5-day retention default — three orders of magnitude of slack. But:

  • Nothing in code enforces the ordering (gc_market's commit drop is an unconditional backstop).
  • pm_closed_market_retention_sec had no lower bound in validate().

So a misconfigured median (retention set below the reveal window) combined with a sustained pm_processing_cap_per_block starvation could let a still-unrevealed commit be garbage-collected before its escrow is returned — stranding a bettor's stake. Not attacker-triggerable and can't happen under sane params, but it's an invariant resting on luck rather than construction.

The fix

One validate() assert:

FC_ASSERT(pm_closed_market_retention_sec
              > (uint64_t)(pm_batch_epoch_blocks + pm_reveal_window_blocks) * CHAIN_BLOCK_INTERVAL,
    "pm_closed_market_retention_sec must exceed the worst-case commit reveal deadline ...");

placed right after the existing pm_batch_epoch_blocks > 0 / pm_reveal_window_blocks > 0 asserts (both operands already validated positive). This makes "a commit is always cleared before its market is GC'd" hold by construction.

Safety

  • Defaults satisfy it with wide margin: 432000 > (20 + 200) * 3 = 660.
  • No testnet config overrides these three params; CHAIN_BLOCK_INTERVAL = 3 in both configs.
  • CHAIN_BLOCK_INTERVAL is already used elsewhere in this validate body (no new include).
  • Header syntax-checks clean against the chain build flags. No behavioural change for any valid configuration.

Scope note

This is the root-cause fix (bound the parameter). A belt-and-suspenders alternative would be to also make gc_market refund any lingering status-0 commit before dropping it; I left that out deliberately since it adds a balance mutation to the GC path to guard a case this bound already prevents. Happy to add it if you'd prefer defense-in-depth. Full observation is written up on #124.

… (validate bound)

Defensive hardening for the commit-reveal escrow path. A commit's reveal deadline can
fall up to (pm_batch_epoch_blocks + pm_reveal_window_blocks) blocks after the commit, and
its escrow is only refunded/forfeited by the reveal-forfeit cron at that deadline.
gc_market deletes pm_commit rows unconditionally once a market has been finalized for
pm_closed_market_retention_sec, with no status-0 refund.

Today this is safe purely by parameter magnitudes (5 d retention vs ~11 min worst-case
reveal deadline). Nothing in code enforces the ordering, and pm_closed_market_retention_sec
had no lower bound — a misconfigured median (retention below the reveal window) plus a
sustained cron cap-starvation could let a still-unrevealed commit be garbage-collected
before its escrow is returned, stranding a bettor's stake.

Add a validate() assert requiring
  pm_closed_market_retention_sec > (pm_batch_epoch_blocks + pm_reveal_window_blocks) * CHAIN_BLOCK_INTERVAL
so the "a commit is always cleared before its market is GC'd" invariant holds by
construction. Both operands are already validated positive just above. Defaults satisfy it
with wide margin (432000 > (20 + 200) * 3 = 660); no testnet override changes these.

Header syntax-checks clean against the chain build flags. No behavioural change for any
valid configuration.
@On1x
On1x merged commit 3573108 into pm Aug 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants