Skip to content

feat: drain credit lots soonest-expiring first#1123

Open
odesenfans wants to merge 1 commit into
mainfrom
od/credit-lot-expiration-first
Open

feat: drain credit lots soonest-expiring first#1123
odesenfans wants to merge 1 commit into
mainfrom
od/credit-lot-expiration-first

Conversation

@odesenfans
Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #1122 (the eager-write lot cache). Switches the drain policy from emission-order to expiration-order:

  • _consume_address_credits now drains lots ORDER BY expiration_date ASC NULLS LAST, message_timestamp ASC, credit_ref ASC, credit_index ASC. Emission order remains the tiebreaker for replay stability.
  • _rebuild_credit_lots_for_address mirrors the same drain policy when replaying history.

This is the end state #1121 was aiming for, but on top of the per-lot keying we already shipped instead of the bucket-per-expiration shape. Reads (get_credit_balance, get_credit_balance_with_details, paginated listings) are unchanged: they already sum over still-valid lots regardless of order.

Behavioural change

User-facing: a spend now consumes the credit lot that is closest to expiring first, before touching longer-lived (or non-expiring) credits. This matches the "use it or lose it" model end-users expect.

⚠️ Consensus risk to discuss before merging

The switch is unconditional — there is no time gate. After this PR ships:

  • All future expense / transfer messages are processed under expiration-first.
  • The startup repair (_rebuild_credit_lots_for_address) replays the entire credit_history under the new policy, so per-address lot states derived from history may differ from what was actually used to validate past transfers (which were processed under emission-first).

In practice every node converges on the same balances after restart because they all replay the same credit_history with the same code. But any consumer that observed pre-deploy balances and acted on them under the old policy is no longer reproducible from the post-deploy node.

If we want a hard cutover that preserves historical state byte-for-byte, the alternative is to gate on message_timestamp with a new CREDIT_EXPIRATION_FIRST_CUTOFF_TIMESTAMP (mirroring CREDIT_PRECISION_CUTOFF_TIMESTAMP): emission-first for < cutoff, expiration-first for >= cutoff. Happy to add that gate before merge — say the word.

Test plan

  • CI green (postgres-backed tests cover the eager-write drain order)
  • Verify the two renamed scenario tests pass under the new policy
  • Verify the details breakdown tests pass with the updated expectations
  • Manual spot-check: distribute mixed lots, spend, confirm the soonest-expiring lot is drained first

Switch the eager-write drain policy from emission-order to expiration-order:
both _consume_address_credits and the repair replay in
_rebuild_credit_lots_for_address now spend the soonest-expiring lot first,
with emission order as the tiebreaker for replay stability. Non-expiring
lots are drained last (NULLS LAST). This matches the user-facing "use it or
lose it" model: a spend should consume credits that are about to lapse
before consuming longer-lived ones.

Reads are unchanged: get_credit_balance(_with_details) already sum over
still-valid lots regardless of order.

Existing tests that encoded emission-first expectations are updated. The
two FIFO scenario tests are renamed to describe the new policy directly,
and the two details tests now assert the expiration-first breakdown of the
remaining lots.
Copy link
Copy Markdown

@foxpatch-aleph foxpatch-aleph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctly implements expiration-first credit lot draining in both the eager-writer SQL path and the repair replay. The drain order (expiration_date ASC NULLS LAST with emission-order tiebreakers) is consistent between both code paths. Test expectations have been accurately recalculated for the new policy. The consensus risk is acknowledged in the PR description and is a deployment strategy decision, not a code defect.

src/aleph/db/accessors/balances.py (line 314): The SQL ORDER BY correctly implements expiration-first with NULLS LAST. The emission-order tiebreakers ensure deterministic replay-stable ordering for lots sharing the same expiration.

src/aleph/repair.py (line 85): The _drain_order_key correctly mirrors the SQL ordering. Python's False < True ensures non-NULL expiration dates sort before NULL, matching NULLS LAST.

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