Add History Retention Window of 7 days

[psheth9]

Introduce new history retention window of 7 days - We are going to have only this single retention window, Txn and events retention window soon to be deprecated.

Context: #115
Design Doc: RPC DB Schema

[Shaptic]

This has a default, so this will basically always be the case unless someone explicitly sets it to zero, right?

[psheth9]

Yes correct, actually I can make it more explicit by removing default value but then client needs to pass it while running RPC. Assuming support of Txn from DB would work without this flag. What do you think?

[Shaptic]

This becomes a breaking behavior change. I think we should be checking the old flags first, then using the new flag if those aren't set. We also have to be careful not to set it for events, since we can't fit 7 days into memory. So like

eventRetention = args.eventRetention
txRetention = args.txRetention == 0 ? args.txRetention : args.historyRetention

Actually maybe it's too early to introduce until we have events 🤔

[Shaptic]

That's why I thought we wouldn't merge this branch until the whole DB backend was ready (@mollykarcher?).

[aditya1702]

@Shaptic Agree that it is a breaking change. Maybe we can completely remove the txn-retention-window functionality in the code and just use the value for history-retention-window.

So in your pseudo-code,

eventRetention = args.eventRetention 
txRetention = args.historyRetention

We just keep the flag as a deprecation warning but remove all configs for txn-retention-window

[Shaptic]

Ah, like "this flag no longer has any effect"?

[psheth9]

yeah I like that, seems better option if we clean up all the logic related to Txn retention window and keep it just in options.go

I think we should be checking the old flags first, then using the new flag if those aren't set

This would not work right? because old flags has default values too so it is always going to run in that if branch

Also just a thought if we are going to release this in next protocol bump, can we not ship it as breaking change i.e New Txn DB support with history-retention-window? -- This message might be more clear to community instead of letting them worry about intermediate confusing retention flag situation

[aditya1702]

Ah, like "this flag no longer has any effect"?

@Shaptic Yes, exactly this. I did a similar thing last year in Horizon when we removed the captive-core-use-db functionality but kept the flag as a deprecation warning.

[mollykarcher]

This would not work right? because old flags has default values too so it is always going to run in that if branch

I think what's being misunderstood/missed here, is that the default values shouldn't apply before we check for the actual explicit existence of the config value. And I would agree with @Shaptic that we want to release this in a backwards compatible way. If people are currently sending the 2 old flags, we need to continue to respect them until we hit a "breaking" API boundary.

In this particular case, (and if we're merging just transactions without events moved over) then I think this looks like:

• don't touch any logic related to eventsRetentionWindow right now
• determine value for historyRetentionWindow as follows:
  • if historyRetentionWindow is being sent by the user, use that
  • elif transactionRetentionWindow is being sent by the user, use that
  • default to 7 days
• in p22 we'll remove transactionRetentionWindow

I will also say, that while I know there is precedence for this in other things we've done (ex. horizon rate limit=0), I really dislike the usage of 0 to indicate null/not present/disabled. Sure, 0 would be a silly thing to set historyRetentionCount to, but it still has real semantic meaning that I think we should respect.

[Shaptic]

the default values shouldn't apply before we check for the actual explicit existence of the config value

idk if we can do this in the code since we use a lib for args but I could be wrong

[psheth9]

Discussed offline: We are not introducing new common retention window !!

You can opt-in to longer transaction retention by setting --transaction-retention-window / TRANSACTION_RETENTION_WINDOW to a higher number of ledgers. This will also retain corresponding number of ledgers in the database. Keep in mind, of course, that this will cause an increase in disk usage for the growing database.

We no longer need this code change so closing this PR.