[ovitrif]

Refs: #29, pubky/paykit-rs#213, pubky/paykit-rs#251

Summary

#29 added Allowance intake but kept it out of production. Each read derived a reader's whole Allowance history in the SDK, and nothing in the server could bound that work. pubky/paykit-rs#213 adds accept_pending_allowance_proposals, which bounds it. This PR adopts it and lifts the block:

• Paykit pin: paykit-lib and paykit-sdk come from the paykit-rs git tag v0.1.1-rc5, which includes feat: accept a link's pending allowance proposals with bounded work paykit-rs#213. It's pinned the same way as Locks in chore: pin locks-core to Locks v0.1.1-rc1 #118.
• Intake: each read is now one SDK call. It accepts up to four of a reader's oldest pending proposals in one transaction, under the Creator mutation lock.
  • This replaces list_allowances plus one accept_allowance per proposal, each of which derived the link's whole history.
  • The SDK applies the same selection the server used: received, local identity as Allowee, Proposed, Consistent, not sent locally. So the server's own filter is gone.
  • more_pending from the SDK decides whether the reader stays due. Cooldown, spacing, read budget, Creator turns and pass deadline are unchanged.
• Payment attribution: rc5 requires a payee not to count payment evidence earlier than its own proposal record time, less a payment-method tolerance, as paying a request. The server now applies that rule to USDT:
  • Migration 0023 stores the SDK's proposal_recorded_at on payment_request_lifecycles. A row stored before the upgrade gets it from its next projection.
  • The invoice window (usdt_payment) and the Marketplace window (status_usdt, which also decides first_qualifying_at) now start at the later of their existing start and the earliest recorded proposal time less PROPOSAL_TIME_TOLERANCE, rounded down to the whole second.
  • PROPOSAL_TIME_TOLERANCE is 24 h 5 min. Arbitrum allows block timestamps up to 24 hours behind the current time, and 5 min covers server clock skew.
  • Until the proposal time is recorded, as for a row from before the upgrade, no USDT evidence pays the request. The receipt is still kept, and the next projection records the time and opens the window. Nothing is credited from such a row, so the backfill has nothing to retract.
• Unknown acceptance outcome: a future PendingAllowanceAcceptance variant fails the read with a retryable error, so the link isn't marked as read.
• Record cap: each read accepts no more than the link's headroom under the SDK's record cap, reserving each Acceptance's Delivery Confirmation, so the server's own Acceptances never take a link past it. The headroom is counted again under the Creator lock, just before the call. A link with no headroom isn't read.
• Cancellation: a running SDK call is never dropped. The pass deadline stops new reads, and shutdown waits for running passes. Intake runs outside the abortable worker set, so the drain deadline doesn't abort it either. While serving, an intake root that returns or panics stops the server like any owned worker. The server's Pubky client bounds each request at 30 s.
• Pre-upgrade payment markers: migration 0023 flags invoices whose payment marker came from USDT alone. Once an invoice's proposal time is recorded, its marker is recomputed under the new rule. Expiry and drain classification follow, and a marker backed by a Bitcoin outpoint is kept. The payment-drain store reconciles before every read of those markers, whatever the USDT configuration, and a marker not yet reconciled never counts as payment, so a drain waits for it.
• Buyer contacts: a contact backed by USDT alone is published only while its payment qualifies under the current window, so a contact queued before the upgrade waits. When the same buyer's later invoice qualifies and the contact's own invoice no longer does, the open contact moves to it. Bitcoin-backed contacts are unaffected.
• Over the SDK's record cap: a link with more than 16,384 records gets LinkHistoryTooLarge, visits no record, and logs link_history_too_large. It counts as read until new items arrive. Its proposals can still be accepted manually.
• Config: paykit.allowance_intake = true is accepted in every build, not only test-utils. The default stays off, so production turns it on in its deploy config.
• The new PaymentRequestRecord timestamp fields are added to two test fixtures, the only 0.1.0 → rc5 API change the server meets.
• The local Docker source script and its test follow the git pin. docs/local-locks-demo.md builds from the tag's commit, ebb8b08, so a moved tag can't change the build. README.md names the pin and the attribution rule.

Tests

• a_large_allowance_history_does_not_hold_the_creator_lock_while_it_is_read is replaced by one_bounded_call_holds_the_creator_lock_and_frees_it_after.
  • The old design kept the unbounded history scan outside the lock and took it once per acceptance.
  • Now one call bounded by the record cap does both, so it runs under the lock and frees it after.
• a_link_over_the_record_cap_is_read_and_left_alone covers the over-cap outcome.
• The config test now asserts that intake is off by default and can be enabled.
• PostgreSQL regressions on both USDT paths:
  • A transfer one second before the bound doesn't pay the request, and one at the bound does.
  • With a legacy row that has no proposal time, nothing pays and no buyer contact is queued.
• A unit test covers payment_window_start.
• On Rust 1.91.1: cargo fmt --check, cargo clippy --workspace --all-targets --all-features -D warnings and cargo test -p paykit-server --all-features pass.
• scripts/test-prepare-local-docker-sources.sh passes on Linux, and the prep script runs cleanly on the real Cargo.toml and Cargo.lock.
• The PostgreSQL e2e suite passes locally on postgres:16-bookworm, run the way CI's postgres-e2e job runs it.

Out of scope

• Reopening payment drains completed before migration 0023, and recording each USDT observation's Payment Request attempt: bug: usdt window ignores the request attempt and old drains stay completed #123.

• Choosing which readers are due still reads each eligible reader's stream items once per pass (newest_stream_items), as before this PR.

• Turning intake on in the production deploy config, which lives outside this repository.

[ovitrif]

@dzdidi heads-up for planning: until this PR is released and deployed with paykit.allowance_intake = true, allowances that Bitkit 3.1 readers send to Locks creators stay unanswered. Allowances between Bitkit users aren't affected. It comes out of draft once pubky/paykit-rs#251 (Paykit v0.1.1-rc5) merges and the pin moves to its tag.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T16:02:06.075003Z	a52a202	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[dzdidi]

Internal exact-head review

Verdict: Changes requested. Reviewed commit a52a202ee433f914ca5d998537fa978f38f31aec; all four exact-head CI jobs pass.

Blocking issue: the v0.1.1-rc5 pin introduces the payee-side proposal_recorded_at attribution bound, but this server drops that value and still accepts USDT using earlier invoice/activation times. See inline comment.

Also fix release surfaces in this dependency-pin PR:

• README.md:35-44 still says Paykit crates.io 0.1.0; document the exact v0.1.1-rc5 Git tag/lock commit and its Payment Request integration requirement.
• Docker source URLs use mutable tag refs while docs claim reproducible, fail-closed source matching; see inline comment.

Non-blocking forward-compatibility note on unknown allowance outcomes is inline.

CI verified: Formatting, Clippy, Server tests, PostgreSQL E2E — all successful on this exact head.

[ovitrif]

@dzdidi every point is addressed in 6e105ac and 167d80d:

• Attribution bound: both USDT windows now open no earlier than the recorded proposal time less a 24 h 5 min Arbitrum tolerance, with PostgreSQL regressions on both paths.
• README: names the v0.1.1-rc5 tag, its lock commit and the attribution rule.
• Docker contexts: use the tag's peeled commit.
• Unknown acceptance outcome: fails the read so it's retried.

The full PostgreSQL e2e suite passes locally.

Addressed in 6e105ac and 167d80d

[dzdidi]

Internal exact-head review

Verdict: Changes requested. Reviewed commit 167d80dedb6c57174f864d5cc302327f54e21276 against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631.

Two migration-path blockers remain: legacy lifecycle rows with NULL proposal_recorded_at still fail open for pre-proposal USDT attribution, and a later timestamp backfill does not retract invoice/drain/buyer-contact authority already derived during that window. See inline comments.

Exact-head hosted CI is green: Formatting, Clippy, Server tests, and PostgreSQL E2E all completed successfully. Current tests cover populated timestamps, not the legacy NULL upgrade path.

Addressed in 5b236d7

[dzdidi]

Exact-head review

Verdict: changes requested. Reviewed 5b236d72d2f0e8319690b09f9661515fd05c9273 against 1c0a923c0df3e4a54cbcdf50b47f8675842b6631.

Three blockers remain:

1. Bounded Allowance intake can append its own link past rc5's record cap, then classify the next over-cap call as drained and strand remaining proposals. See inline.
2. Production enablement exposes existing pass-deadline and shutdown abort paths that can drop a mutating shared-state SDK future. with_write_lock unlocks only after the operation settles, and the adapter marks state unconfirmed before its awaited commit. Enforce deadlines between SDK mutations, await a started mutation during shutdown, and bound transport requests instead of dropping the stateful future.
3. Migration backfills lifecycle time but does not fence pre-v23 pending buyer-contact work, so upgraded code can publish contact from USDT evidence its current eligibility rule rejects. See inline.

Also fix migration comment: exact-head behavior keeps the window closed while proposal_recorded_at is NULL, contrary to its text.

Exact-head hosted CI is green: Formatting, Clippy, Server tests, PostgreSQL E2E. Focused pinned-SDK cap test passed (1 test; 928 filtered), confirming the first cap-minus-one call appends four while reporting more pending. Hosted CI does not cover the second-call cap crossing, cancellation after shared-state mutation starts, or populated-v22 pending-contact upgrade.

[ovitrif]

@dzdidi all three blockers and the migration comment are addressed in f2910c6, d34f195 and 8050e30. The record-cap and buyer-contact replies are in their threads. The cancellation blocker had no inline thread, so here is its fix (d34f195):

• Pass deadline: no pass is wrapped in a timeout anymore. The 10-minute deadline only stops new reads from starting, and each read is a single SDK mutation, so it's checked between mutations.
• Shutdown: finish_intake_passes waits for running passes instead of aborting them, and abort_and_join is removed. A test holds a pass mid-call and checks that shutdown waits for it.
• Transport bound: the server's Pubky client now sets a 30 s request_timeout, so a call on a stalled homeserver fails and settles its write lock instead of being dropped. The companion relay sets its own per-request timeout on each poll, which overrides this.
• Process drain timeout: past shutdown.drain_timeout, the process still aborts all workers as a last resort. That applies to the transport worker in the same way.

The full PostgreSQL e2e suite passes locally on the new head.

Addressed in f2910c6, d34f195 and 8050e30

[dzdidi]

Review summary

Recommended verdict: request changes. Exact-head hosted CI is green, but three production correctness defects remain: stale record-cap headroom can cross the Allowance cap; outer shutdown can still cancel a started SDK mutation; and a held legacy buyer-contact row can suppress a later valid invoice for the same buyer. See inline comments for smallest fixes.

Reviewed exact head 8050e308b78a567177c26ef2fa76f86391fa7441 against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631.

Addressed in 330158e, 0acd104 and ea6c4f3

[dzdidi]

Reviewed exact head 97117738da07bb697adc3c7b0e59d259ab0f89b2 against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631. One blocking migration defect remains: pre-upgrade USDT settlement authority can survive even when current proposal-time attribution rejects that evidence, preventing expiry and allowing drains to complete. See inline comment.

Exact-head hosted CI completed successfully: Formatting, Clippy, Server tests, and PostgreSQL E2E. Current tests do not exercise a populated-v22 stale marker through v23 expiry and drain classification.

Addressed in a14b739, 9711773 and 95c1a7c

[dzdidi]

Reviewed exact head 95c1a7c7b9a36dadaee89ed0eaef2d080b77398d against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631. One blocking migration/drain defect remains: lifecycle refresh can establish the proposal-time attribution bound, then drain creation/replay can still consume stale pre-upgrade USDT payment authority before its deferred reconciliation runs. See inline comment.\n\nExact-head hosted CI completed successfully: Formatting, Clippy, Server tests, and PostgreSQL E2E. Current E2E directly invokes the USDT observer and does not cover lifecycle backfill followed immediately by drain snapshot.

Addressed in 1dfc045

[dzdidi]

Internal exact-head review

Recommended human verdict: request changes.

One verified blocking migration/data-integrity defect remains. Exact-head hosted CI is green (Formatting, Clippy, Server tests, PostgreSQL E2E), but current regression does not cover disabled [usdt] production composition. See inline finding.

Reviewed commit: 1dfc0455cea96606f7b5a5adb81185cd106ec834.

[dzdidi]

Changes requested on exact head 1dfc0455cea96606f7b5a5adb81185cd106ec834 against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631. This supplies the formal verdict omitted by review 5743231815; the existing independently verified inline finding remains at #122 (comment) (not duplicated here).

Blocking: when optional [usdt] is absent, PaykitAdapter skips required post-refresh legacy-credit reconciliation, allowing stale pre-upgrade USDT authority into a drain snapshot. Retain InvoiceStore independently from the optional verifier and reconcile before snapshot/replay; add a production-path PostgreSQL regression with [usdt] disabled.

Exact-head hosted CI passed, but does not cover this configuration path. No source changes or local PostgreSQL reproduction were performed.

Addressed in 0a8f554

[dzdidi]

Blocking data-integrity defect: an unresolved migration-flagged USDT marker can complete and clean up a drain before later proposal-time attribution rejects the transfer. Exact-head hosted CI is green, but the current undecided migration test encodes this completion and does not cover stale backfill after replay. Keep flagged invoices nonterminal and cleanup-blocked until attribution resolves, then add the migration regression described inline.

Addressed in 56a0099

[dzdidi]

Internal exact-head review

Verdict: changes requested. Reviewed 56a0099ba7882f837c4a6b64b338651914fe1268 against 1c0a923c0df3e4a54cbcdf50b47f8675842b6631.

Three blocking persistence/attribution defects remain:

1. Migration 0023 can strand already-completed retained drains when later proposal attribution reopens their invoice.
2. A late amount-matched Bitcoin outpoint suppresses migration reconciliation despite not being timely Bitcoin settlement authority.
3. USDT observations lose request identity, then use the earliest proposal time across multiple supported attempts, allowing wrong-attempt attribution on both Locks invoices and Marketplace preparations.

Exact-head hosted CI completed successfully: Formatting, Clippy, Server tests, and PostgreSQL E2E. CI tested synthetic merge 1c920a01d2c88e05215d51e70476b5e9dd04f61e, whose parents are the pinned base/head and whose tree equals head tree 80a2d0830f5d5d118c8ff5bc646904df902871b6. Current tests do not exercise the three states above. See inline findings for failure scenarios and smallest fixes.

[dzdidi]

Blocking — reopen affected retained drains during migration. This flags the invoice but leaves any retained v22 payment_drains summary unchanged. A drain already stored as accepted_count = 0, terminal_count = 1, status = 'completed' must become accepted/nonterminal if later proposal attribution clears the old USDT marker. Runtime cannot make that reversal: exact_replay only permits accepted count to fall and terminal count to rise, while cleanup_completed rejects the now-stale canonical summary. Its active_drain_id then fences this lock generation permanently. Recompute/reopen affected retained drain summaries here, and add an upgrade regression that completes and retains a v22 drain before applying 0023, records a proposal time that makes the transfer stale, then verifies replay and eventual cleanup.

[ovitrif]

Proposed to move to #123, per the split in #122 (comment). I'm leaving this thread open for you.

[dzdidi]

Blocking — compare evidence with its own request attempt, not MIN across attempts. Verification above binds the proof to record.payment_request_id, but the persisted USDT observation stores only transfer facts/sender. This query then discards request identity and applies the earliest proposal time from every lifecycle row owned by the invoice. Multiple ambiguous retry attempts per invoice are supported, so evidence verified only for a later attempt can inherit an earlier attempt's window; a known attempt can likewise open the window for evidence whose matching legacy attempt still has NULL time. Persist request-level attribution for each verified observation (or an equivalent transfer/request attestation), compare against the matching lifecycle row, and fail closed/reverify legacy observations whose request cannot be reconstructed. The identical MIN(proposal_recorded_at) pattern in Marketplace status_usdt needs the same fix and two-attempt regression.

[ovitrif]

Proposed to move to #123, per the split in #122 (comment). I'm leaving this thread open for you.

[ovitrif]

@dzdidi round 10's timely-outpoint point is fixed in 959d7a7. For the other two, I propose splitting the scope:

• This PR lands as:
  • the v0.1.1-rc5 pin
  • the bounded Allowance intake
  • the paykit.allowance_intake config lift
  • the USDT attribution work from the earlier rounds:
    • the proposal-time bound on both windows
    • fail-closed while the proposal time is unknown
    • pre-upgrade marker reconciliation before every drain read
    • drains kept open while a flagged marker is unresolved
• Moved to bug: usdt window ignores the request attempt and old drains stay completed #123, to work through with you:
  • reopening payment drains completed before migration 0023 (thread)
  • recording, per USDT observation, the Payment Request attempt it was verified for, on both Locks invoices and Marketplace (thread)

Why the split: version 0.1.0 had no proposal-time bound at all, so this PR doesn't regress either case, and the intake doesn't depend on them. Until this PR is released and deployed, allowances that Bitkit 3.1 readers send to Locks creators stay unanswered.

Could you re-review on that basis?

[dzdidi]

Internal exact-head review

Verdict: changes requested. Reviewed exact head 959d7a7a404758795a5baa74439287d56c9cc43d against base 1c0a923c0df3e4a54cbcdf50b47f8675842b6631. Full exact-tree hosted CI is green (Formatting, Clippy, Server tests, PostgreSQL E2E), but two blocking persistence/attribution defects remain: migration 0023 can strand retained completed drains, and USDT evidence is bounded by the earliest invoice-wide proposal rather than the verified Payment Request attempt. See inline comments for failure paths and smallest fixes.

[dzdidi]

Blocking — migration/persistence: This flags legacy USDT-paid invoices without reopening any retained drain that already summarized one as terminal/completed. After upgrade, the canonical snapshot treats a flagged accepted item as nonterminal, so accepted_count must rise from 0 to 1 and terminal_count fall. exact_replay rejects that reverse transition, while cleanup_completed rejects the stale summary; active_drain_id then keeps the generation fenced. Recompute/reopen affected retained drains in this release migration (or a same-release forward migration), then add a populated-v22 regression that completes a drain, applies 0023, records a stale proposal time, and proves replay plus eventual cleanup work.

[dzdidi]

Blocking — attribution/data integrity: observe_usdt_request verifies evidence under one exact record.payment_request_id, but TransferFacts does not retain that request ID and this query uses the earliest proposal across every attempt for the invoice. With ambiguous attempts A then B, evidence verified only for B can inherit A's earlier window; a known A can also open the window while B's proposal time is still NULL. That can mark a request paid before its own permitted window. Persist authenticated request-attempt identity with each observation, resolve the exact lifecycle row, and fail closed for legacy observations that cannot be attributed. Add two-attempt PostgreSQL regressions here and for the equivalent Marketplace MIN query in marketplace_preparations/usdt.rs:463.

[ovitrif]

@dzdidi restating the open question, since it got buried under the latest automated review: can this PR land without the two remaining items, with those moved to #123?

• Your latest review (959d7a7) repeats exactly the two items I proposed to split off in feat: allowance intake in production #122 (comment):
  • reopening drains completed before migration 0023 (thread)
  • recording the Payment Request attempt per USDT observation (thread)
• Why split: 0.1.0 had no proposal-time bound at all, so this PR doesn't regress either case, and the allowance intake doesn't depend on them.
• Why it matters now: Bitkit 3.1 is in store review with allowances on. Until this PR is released and deployed with paykit.allowance_intake = true, allowances that Bitkit readers send to Locks creators get no answer.

If you'd rather have both items in this PR, say so and I'll build them here instead.