-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bump threshold #998
Add bump threshold #998
Conversation
@@ -332,7 +333,7 @@ impl Storage { | |||
) | |||
})?; | |||
|
|||
if new_expiration > old_expiration { | |||
if new_expiration > old_expiration && new_expiration - old_expiration >= threshold { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to fail if it's never possible to bump the entry, i.e. if bump_by_ledgers <= threshold
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of bump_by_ledgers
probably needs to change - it's not the actual bump amount. bump_by_ledgers
is the bump from the current ledger, but doesn't account for the difference between old_expiration and current ledger. This means you could have a bump_by_ledgers of 10, but new_expiration-old_expiration could be greater than 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not possible. old_expiration is at least current_ledger
, so it's guaranteed that new_expiration-old_expiration <= bump_by_ledgers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah my bad...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the check be bump_by_ledgers < threshold
? If current ledger == 10, expiration == 10, bump_by_ledgers == 2, and threshold == 2, the new expiration would be 12 with a bump of 2 right?
What
Add a threshold for bumping entries.