Change how dynamic captcha stores data #2660
Conversation
lmuntaner
force-pushed
the
lm-change-dynamic-captcha-storage
branch
from
0fce48f to
c9d1e5a
Compare
|
@frederikrothenberger please review |
| // calculate the time window length with respect to the current time | ||
| .map(|ts| now - ts) | ||
| // the value _could_ be 0 if the oldest timestamp was added in the same execution round | ||
| // -> filter to avoid division by 0 | ||
| // There could be negative values if they haven't been pruned yet. |
There was a problem hiding this comment.
The values are u64 so the cannot be negative. But also ts is never bigger than now so the minimum value is 0, no?
There was a problem hiding this comment.
Oh, sorry, yes. I did some changes here and I didn't realize that it doesn't make sense this anymore.
| registration_rates.registration_rates().unwrap(), | ||
| NormalizedRegistrationRates { | ||
| current_rate_per_second: 2.0, | ||
| reference_rate_per_second: 2.0, // increases too, but more slowly |
There was a problem hiding this comment.
| reference_rate_per_second: 2.0, // increases too, but more slowly | |
| reference_rate_per_second: 2.0, // equal to current rate because window lenght is the same |
| registration_rates.new_registration(); | ||
| TIME.with_borrow_mut(|t| *t += Duration::from_secs(1).as_nanos() as u64); | ||
| } | ||
| assert_eq!( |
There was a problem hiding this comment.
Maybe change it to assert equality between current_rate_per_second and reference_rate_per_second? Because that is the important property of the test, not the actual resulting values.
There was a problem hiding this comment.
good point, done!
| // Needed to prune the data after new config | ||
| // It prunes 100 for each new registration | ||
| // we added 1100 before. Remove them with 110, plus 2 to remove the new 110 added. | ||
| for _ in 0..112 { |
There was a problem hiding this comment.
I'm confused by this. It only takes 11 registrations to prune 1100 entries if every entry prunes 100 elements.
But also, the whole block is not necessary as the subsequent setup of the new rates will more than prune all of the existing ones.
There was a problem hiding this comment.
Oh, yes, true. Removed.
|
@frederikrothenberger ready for another review |
| @@ -112,8 +112,6 @@ fn calculate_registration_rate<M: Memory>(now: u64, data: &MinHeap<Timestamp, M> | |||
| .peek() | |||
| // calculate the time window length with respect to the current time | |||
| .map(|ts| now - ts) | |||
| // There could be negative values if they haven't been pruned yet. | |||
| .filter(|val| *val != 0) | |||
There was a problem hiding this comment.
No,no you still need the 0 check. It's just the comment that was wrong. I.e. it should say filter out 0 rather than talk about negative values.
There was a problem hiding this comment.
oops, yes. Good that there was a test for it
lmuntaner
force-pushed
the
lm-change-dynamic-captcha-storage
branch
from
c8d4743 to
11b5552
Compare
Motivation
We realized that when the config changes, the stored data still takes into account the old config because it was stored by expiration date which took into account the config.
In this PR, the PR is stored with the create timestamp and pruned taking into account the config intervals.
There is a new test to check that the old config doesn't affect the new rates (given enough time to prune the old data).
🟡 Some screens were changed