feat(networking): only consider close records in price setting #1514
Conversation
|
I understand now @maqi why you were so hesitant to have the Hmmm... Need to chew on cache + backup implications of this approach some. |
|
Removing do not merge, as I think we need this either way. As churn happens, our area of responsibility shrinks, so the price needs to reflect that, not be based upon what we're holding. I know we only replicate in relevant data now, but as churn happens, some of that will become "out of scope". So we need to discard based upon that. We can memoise the fn for every X checks or so perhaps if perf is an issue. |
sn_networking/src/record_store.rs
Outdated
|
|
||
| // set's the local distance range to be further if the new record is further | ||
| // This assumes the verified record is to be held by _us_... | ||
| self.distance_range = self.distance_range.map(|range| range.max(new_distance)); |
There was a problem hiding this comment.
As the network grows, would we not be getting records that are closer to us? So this would mean that we'll be hanging on to the older value?
There was a problem hiding this comment.
Aye, cleaned that out and moved farthest record check into an interval setter. Should be enough and prevent too much overzealous "close" checking on store etc.
joshuef
force-pushed
the
StoreCostTweaks
branch
from
f0eccc3 to
de3255a
Compare
RolandSherwin
force-pushed
the
StoreCostTweaks
branch
from
0285cdb to
65990c0
Compare
| NanoTokens::zero() | ||
| } else if let Some(distance_range) = self.distance_range { | ||
| let record_keys_as_hashset: HashSet<&Key> = self.records.keys().collect(); | ||
| let relevant_records = |
There was a problem hiding this comment.
note that there is pruning to be carried out when hit max_records, which is regarding ALL.
i.e. this means relevant_records may way below max_records, and will never reach that bar.
It may be good, may be bad,
Also, I'd suggest just rename QuotingMetrics::records_stored to QuotingMetrics::relevant_records to avoid any potential confusion.
| } else if let Some(distance_range) = self.distance_range { | ||
| let record_keys_as_hashset: HashSet<&Key> = self.records.keys().collect(); | ||
| let relevant_records = | ||
| self.get_records_within_distance_range(record_keys_as_hashset, distance_range); |
There was a problem hiding this comment.
looks like the correspondent addr_distribution_sim test not got updated to reflect this change?
this will result in an un-sync between the sim and real code.
There was a problem hiding this comment.
Hmmm, im not sure what needs to be changed here.
I've updated some naming, and made things a bit clearer, but the crux of the test is entirely the same. We were already only calculating for close_records as there's no churn here.
JasonPaulGithub
added
Small
joshuef
force-pushed
the
StoreCostTweaks
branch
from
65990c0 to
218f76b
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
uses CLOSE_GROUP_SIZE + 1 peers' distance as a proxy. This gives some margin for error and should err on the side of caution
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // // As now using the `close_records_stored`, with the changing range, | ||
| // // there is chance that a newer close_records_stored will be smaller than old ones | ||
| // // | ||
| // // TODO: rethink about the detection policy on this part |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
Description
reviewpad:summary