[bug]: garbage collect `batch-replay` bucket in `sphinxreplay.db`

[C-Otto]

The sphinxreplay.db on my node is larger than my channel.db (2.2 GByte vs. 1.8 GByte), which seems odd. Please add a feature that reduces the size of sphinxreplay.db.

(And, while we're at it, please document what the file is actually used for: #6584)

• lnd v0.15.4-beta
• bitcoind v23

[guggero]

The Sphinx replay DB is lnd's implementation of the ReplayLog defined in the sphinx packet. It keeps track of onion packets to make sure they aren't used in a replay attack.
Each package is stored with its CLTV and is garbage collected as soon as the CLTV has expired. That means the DB is subject to a lot of reads and writes and therefore does profit from regular compaction.

Can you try compacting it and see if it still stays as as it currently is?

Maybe another option (after compaction) would be to take a look at it with boltbrowser (shut down lnd first, create a copy of the DB file, start lnd again, inspect copy) and check whether there are expired hashes in there?
Or maybe there's some data stored in there in addition to the hashes themselves that aren't properly garbage collected?
I'll take a look at my DB as well to see if I can identify anything.

[C-Otto]

This was the previous compaction:
2022-10-26 19:57:03.444 [INF] CHDB: DB compaction of .../sphinxreplay.db successful, 2405040128 -> 2255822848 bytes (gain=1.07x)

The one before:
2022-08-16 22:48:33.767 [INF] CHDB: DB compaction of /home/bitcoin/.lnd/data/graph/mainnet/sphinxreplay.db successful, 1969864704 -> 1900961792 bytes (gain=1.04x)

I don't think that having 2 GByte of data is OK?

[guggero]

Yeah, you're right. That's a lot of data.
And I think I found the bug. We write to the batch-replay bucket here but we apparently never delete from it. That needs garbage collection as well. Though I'm not familiar with it enough to say whether that's easy to fix or not.

[guggero]

I'm going to change the title to indicate this is a bug.

[C-Otto]

I have 612,489 pairs in batch-replay, 31,389 pairs in shared-hash.

[C-Otto]

I had a look at the code and I can't see how the information persisted in batch-replay is used. The data is only read in a situation where we'd write the same data again. This doesn't affect the control flow, i.e. the code triggering this doesn't notice that the data already existed (wrong). Aside from this I don't see any read access to batch-replay.

PutBatch was added in 2018, without any test (EDIT: testing this function directly, in the sense of a unit test). The code related to batch-replay never changed, i.e. what we see today should match the original intent.

A comment shortly before the bucket put operation indicates that the persisted data can be used during recovery:

// Write the replay set under the batch identifier to the batch
// replays bucket. This can be used during recovery to test (1)
// that a particular batch was successfully processed and (2)
// recover the indexes of the adds that were rejected as
// replays.

Given the fact that this was introduced 4 yours ago, we should have better idea of how helpful/necessary this is. Sadly, I lack these insights. @guggero, as someone who lives and breathes bbolt (I think, sorry), what's your position on this?

NB: The information in "shared-hash" is used. When storing new data (Put, PutBatch), replays are detected and treated as an error. I guess this is the main reason for this code to exist.

[C-Otto]

The idempotency idea is mentioned in the definition:

// batchReplayBucket is a bucket that maps batch identifiers to
// serialized ReplaySets. This is used to give idempotency in the event
// that a batch is processed more than once.

However, I don't see how idempotency helps. If the code invoking PutBatch always provides the same batch information, we do not need batch-replay persistence to have idempotency (I think).