Unify mailbox TTL to a single value by Arowolokehinde · Pull Request #1457 · payjoin/rust-payjoin

Arowolokehinde · 2026-03-31T18:12:27Z

This Pr addresses the issue in #1421 which replace three separate TTL constants (read: 10 min, unread at capacity: 1 day, unread below capacity: 7 days) with a single DEFAULT_TTL of 24 hours based on creation time only. Removes read_order, read_mailbox_ids, mark_read(), and the read-TTL pruning pass. Capacity eviction remains as a storage pressure mechanism.

I brainstormed the implementation approach with Claude while working through this fix.

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

nothingmuch · 2026-03-31T18:48:13Z

i'm still not convinced this change is desired, per discussion in #1421. if a single TTL was used but it was set to 1 week (or some other value, with justification) instead of 24hrs, i would be concept ACK but still not convinced.

if the argument in favor of this change is that the tradeoff of reduced code complexity over poorer quality of service is desirable is worth it, then sure... not sure i agree but if others (@spacebear21?) disagree i'm happy to be overruled. having just one TTL value can simplify pruning as that can just be implemented as a fixed size circular buffer of IDs.

BIP 77, as currently written, does not specify a default expiry time at all, and this change effectively enforces a 24 hour limit instead of a soft 1 week limit, even though the resource requirements by a directory operator aren't that different and that doesn't seem very justified

it follows that if this PR was only modified so that the hard limit was set to say 1 week in order to preserve the current best effort quality of service (and upgrade it to a guarantee in a sense), and this change is instead framed as removing the complexity of early deletion of read messages, we want the complexity of early deletion of read messages? arguably deleting those messages sooner rather than later is the more privacy preserving of the two, and comes with a 7* increase in storage capacity required, which also seems negligible (since it's only around 2GB now with the 24hr limit, and storage is cheap).

On the other hand, if the argument is that this is a reduction in trust in the directory operator, i categorically disagree and believe that that actually undermines trust as i've argued in the ticket, so concept NACK if that's the justification

Arowolokehinde · 2026-04-01T13:21:29Z

Thanks for the detailed feedback @nothingmuch

I’m still getting familiar with BIP 77 and the intended expectations around TTL, so this is helpful context. From your explanation, I understand that enforcing a strict 24-hour TTL may reduce quality of service compared to a 1 week expectation.

I also see your point that simplification could still be achieved with a longer duration like 1 week, without negatively impacting service guarantees.

Based on this, would you prefer that I update the PR to use a longer TTL (e.g., ~1 week) or should I hold off for now until there’s broader agreement on the desired behavior?

spacebear21 · 2026-04-02T18:20:11Z

In my opinion the simplification is significant enough that I'm leaning towards concept ACK, but I agree that 1 week sounds more appropriate for giving more choice to implementations re: session expirations. Obviously this doesn't reduce trust in the operator - in fact I wonder if DEFAULT_TTL should be straight-up configurable?

nothingmuch · 2026-04-02T23:33:40Z

-            unread_ttl_below_capacity: DEFAULT_UNREAD_TTL_BELOW_CAPACITY,
-            unread_ttl_at_capacity: DEFAULT_UNREAD_TTL_AT_CAPACITY,
-            read_ttl: DEFAULT_READ_TTL,
+            ttl: DEFAULT_TTL,


i agree with spacebear, iirc the reason i didn't make this configurable was that at the time we already expected to rewrite the config handling, but this should be at the operator's discretion, since it's now only one param i think adding it to init as a standalone argument seems reasonable

also, when added this should cross reference the rate limiting stuff, because these two parameters together enforce the limit on storage... maybe it's better to set the rate limiting by defining another config parameter for max storage, and then ensure that rate limit is (max_storage/storage_per_mailbox))/TTL writes per unit time

nothingmuch · 2026-04-02T23:33:49Z

-            unread_ttl_at_capacity: DEFAULT_UNREAD_TTL_AT_CAPACITY,
-            read_ttl: DEFAULT_READ_TTL,
+            ttl: DEFAULT_TTL,
            early_removal_count: 0,


Suggested change

early_removal_count: 0,

nothingmuch · 2026-04-02T23:34:04Z

        while let Some((created, id)) = self.insert_order.front().cloned() {
-            if created + self.unread_ttl_below_capacity < now {
+            if created + self.ttl < now {
                debug_assert!(self.insert_order.len() >= self.early_removal_count);


Suggested change

debug_assert!(self.insert_order.len() >= self.early_removal_count);

nothingmuch · 2026-04-02T23:34:17Z

-    unread_ttl_at_capacity: Duration,
-    read_ttl: Duration,
+    ttl: Duration,
    early_removal_count: usize,


Suggested change

early_removal_count: usize,

nothingmuch · 2026-04-02T23:34:56Z

Suggested change

self.insert_order.len()

nothingmuch · 2026-04-02T23:37:29Z

Suggested change

self.remove(&id).await?; // FIXME what to do on is_none()?

github is being glitchy, this suggestion needs to start from the if self.remove(&id) line but for some reason i can't make a comment on those lines

i guess is_none would now only imply that the operator deleted mailboxes while the server was running, or there is some bug or disk failure.

therefore IMO None should not be treated this as an error (it may be intentional), but a log a warning is appropriate so if it's unintentional the operator would be informed of the data on disk disappearing. in case the operator suspects it's a bug, it's probably best not to log the shortid since the time at which this error would be discovered implies the time at which the shortid was inserted, and that information might be inadvertently posted in an issue, making this temporal information public.

nothingmuch · 2026-04-02T23:42:48Z

        if self.len() == self.capacity {
            if let Some((created, id)) = self.insert_order.front().cloned() {
-                if created + self.unread_ttl_at_capacity < now {
+                if created + self.ttl < now {


this is redundant with the code above, i believe this is dead code because self.len() == self.capacity will only be true if created + self.ttl < was already false before

okay . thanks for the review
i would make the neccessary changes

nothingmuch

utACK, code looks good but two minor corrections to (doc) comments

nothingmuch · 2026-04-07T19:18:28Z

        self.pending_v2.retain(|_, v| v.receiver.strong_count().unwrap_or(0) > 1);

-        // Prune any fully expired mailboxes, whether read or unread
+        // Prune any fully expired mailboxes


nit:

Suggested change

// Prune any fully expired mailboxes

// Prune any expired mailboxes

nothingmuch · 2026-04-07T19:25:38Z

    #[serde(deserialize_with = "deserialize_duration_secs")]
    pub timeout: Duration,
+    /// Mailbox lifetime in seconds. Together with `DEFAULT_CAPACITY` in
+    /// `db::files`, this bounds both disk usage and write throughput.


this could bound write throughput but doesn't do so currently except in the trivial sense that once storage is full no new writes are accepted, see tracking tasks in #941, this could be done with tower middleware

Noted
i would remove the comment

caarloshenriq

tACK 8988cc0

Tested on linux mint with nix. Server starts cleanly, /ohttp-keys returns 200 with
the expected binary payload. Unit tests pass (cargo test -p payjoin-mailroom).
mailbox_ttl is configurable via config.toml and defaults to 1 week as
discussed

coveralls · 2026-04-14T06:16:10Z

Coverage Report for CI Build 24402852101

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.04%) to 84.38%

Details

Coverage increased (+0.04%) from the base build.
Patch coverage: 2 uncovered changes across 2 files (67 of 69 lines covered, 97.1%).
1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin-mailroom/src/config.rs	2	1	50.0%
payjoin-mailroom/src/db/files.rs	50	49	98.0%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-mailroom/src/db/files.rs	1	93.31%

Coverage Stats


Relevant Lines:	12766
Covered Lines:	10772
Line Coverage:	84.38%
Coverage Strength:	413.48 hits per line

💛 - Coveralls

Replace three separate TTL constants (read: 10 min, unread at capacity: 1 day, unread below capacity: 7 days) with a single DEFAULT_TTL of 24 hours based on creation time only. Removes read_order, read_mailbox_ids, mark_read(), and the read-TTL pruning pass. Capacity eviction remains as a storage pressure mechanism.

spacebear21

utACK d3d4c8a, thanks @caarloshenriq for testing!

nothingmuch reviewed Apr 2, 2026

View reviewed changes

Arowolokehinde force-pushed the unify-mailbox-ttl branch from 16828a8 to 8988cc0 Compare April 6, 2026 19:51

Arowolokehinde requested a review from nothingmuch April 6, 2026 19:54

nothingmuch approved these changes Apr 7, 2026

View reviewed changes

caarloshenriq approved these changes Apr 12, 2026

View reviewed changes

Arowolokehinde force-pushed the unify-mailbox-ttl branch from 8988cc0 to abebc6d Compare April 14, 2026 13:23

Arowolokehinde force-pushed the unify-mailbox-ttl branch from abebc6d to d3d4c8a Compare April 14, 2026 13:52

spacebear21 approved these changes Apr 15, 2026

View reviewed changes

spacebear21 merged commit 592f914 into payjoin:master Apr 15, 2026
16 checks passed

nothingmuch mentioned this pull request Apr 15, 2026

OHTTP keys should be rotated #1449

Open

2 tasks

	// Prune any fully expired mailboxes
	// Prune any expired mailboxes

Conversation

Arowolokehinde commented Mar 31, 2026

Uh oh!

nothingmuch commented Mar 31, 2026

Uh oh!

Arowolokehinde commented Apr 1, 2026

Uh oh!

spacebear21 commented Apr 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caarloshenriq left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 24402852101

Coverage increased (+0.04%) to 84.38%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Apr 14, 2026 •

edited

Loading