fix: prevent backdated expense from draining future-dated grant#1124
fix: prevent backdated expense from draining future-dated grant#1124odesenfans wants to merge 2 commits into
Conversation
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Click Re-request review to retry.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Click Re-request review to retry.
The eager-write drain in _consume_address_credits filtered lots by the expense's expiration bound but not by its lower-bound message_timestamp. Messages can arrive out of order in the P2P pipeline, so an older expense could land after a future-dated grant has already been cached as a lot, at which point the eager writer would happily deduct from that lot. The repair replay enforces the lower bound by walking credit_history chronologically, so the next restart would silently correct the eager state. Add the missing ``AlephCreditBalanceDb.message_timestamp <= message_timestamp`` predicate so the eager writer matches what the replay derives, and cover it with a regression test that processes a backdated expense against a future-dated grant.
test_credit_transfer_non_whitelisted_sender seeded the sender with a distribution dated 2023-01-01 but processed a transfer whose embedded ``time`` field is 1651050219.0 (2022-04-27). With the new timestamp lower-bound filter on the eager-write drain, a transfer dated 2022 cannot deduct from a lot whose message_timestamp is 2023 — that is the bug the filter is supposed to catch. Move the seed back to 2022-01-01 so the lot genuinely precedes the transfer in time.
2e097c6 to
9899c70
Compare
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR correctly addresses a gap where backdated expenses could drain future-dated grants in the eager-write credit lot cache. The one-line filter addition (AlephCreditBalanceDb.message_timestamp <= message_timestamp) in _consume_address_credits brings the eager writer's eligibility logic to parity with the chronologically-walked repair replay. The regression test directly validates the fix, and the existing transfer test was adjusted to maintain correctness under the new constraint. The code is well-documented and clean.
src/aleph/db/accessors/balances.py (line 308): Correctly adds the lower bound on message_timestamp. The <= is the correct operator since grants at exactly the same timestamp as the expense should be drainable (the repair replay, which processes entries chronologically, would see the grant before the expense at the same timestamp).
src/aleph/db/accessors/balances.py (line 283): The updated docstring is thorough and clearly explains the rationale for both bounds. Good documentation.
tests/message_processing/test_process_posts.py (line 224): The adjusted timestamp (2022-01-01) correctly seeds the grant before the transfer's time field (2022-04-27), matching the new invariant. The explanatory comment is helpful.
Summary
The eager-write drain in `_consume_address_credits` was filtering lots by the expense's expiration bound only, not by the lower bound on the lot's own `message_timestamp`. Messages can arrive out of order in the P2P pipeline, so a future-dated grant may already exist as a lot in the cache when an older expense lands. Without the lower bound the eager writer happily deducts from a lot that didn't yet exist at the expense's instant.
This is a gap that was introduced in #1122 (the eager-write lot cache PR) and hadn't been spotted before. `_rebuild_credit_lots_for_address` already enforces both bounds because it walks `credit_history` chronologically, so the next restart silently heals any divergence the eager writer produces. The fix brings the eager-write filter to parity with what the replay derives.
Change
One-line filter addition in `_consume_address_credits`:
```python
AlephCreditBalanceDb.message_timestamp <= message_timestamp,
```
Docstring updated to spell out both bounds.
Test plan