DO NOT MERGE Valset: add pagination to active validators query #35
Conversation
ueco-jb
changed the title
| // First iteration - load all validators, second - remove all, third - add all new | ||
| // Not very efficient... | ||
| let old_validators = VALIDATORS | ||
| .range(deps.storage, None, None, Order::Descending) | ||
| .map(|validator_info| { | ||
| let (_, info) = validator_info?; | ||
| Ok(info) | ||
| }) | ||
| .collect::<StdResult<Vec<ValidatorInfo>>>()?; | ||
| for validator in &old_validators { | ||
| VALIDATORS.remove(deps.storage, &validator.operator); | ||
| } | ||
| for validator in &validators { | ||
| VALIDATORS.save(deps.storage, &validator.operator, validator)?; | ||
| } |
There was a problem hiding this comment.
Both new_ and old_validators vecs are necessary to calulate diff (line 571).
I had idea about looping through all validators, if address is not found in new_ then it would be removed from VALIDATORS... but that would require 1) in-place remove 2) completely different approach to calculate diff.
I'm still trying to figure out something more optimal.
There was a problem hiding this comment.
@maurolacy worked on something similar with HashMap in some other PR
There was a problem hiding this comment.
I think BTree map was important if we want deterministic sorting order.
Can you link the other contract that did this?
There was a problem hiding this comment.
There are two things here:
- Pagination. This is (relatively) minor, and can be solved in different ways.
Itemvalue size limitations. This is something that came up during discussion, and it's important, because we're storingValidatorMetadata(which has a lot ofStringfields) insideValidatorInfo. These fields are also in control of the operator, so, this can be clearly abused, and can lead to performance issues, or even, denial of service.
We'll tackle 2. in #42.
Regarding 1., and without changing the underlying structure, a simple approach would be to populate a BtreeSet with the data, then range over it using BTreeSet::range, and paginate using take over the iterator as usual.
A similar / equivalent way would be to sort the vec, and then bisect, but I think the btree approach is simpler / better. BTreeSet is already being used in this contract to compute the difference of the validators sets.
There was a problem hiding this comment.
Another option could be to turn VALIDATORS into an Item<BTreeSet<ValidatorInfo>> structure.
Need to research this a bit, but at first glance it looks overkill. We will also have to be very careful with the size requirements of BTreeSet itself. And, reducing the ValidatorInfo footprint as well, like mentioned in #42.
Update: BTreeSet footprint is similar to that of Vec (24 bytes). Will take a look at the code but in principle a BTreeSet can be treated like a vector, so, this looks feasible, and is perhaps the best option.
Update 2: Will do more analysis, as part of #42, to confirm these footprints / limits.
There was a problem hiding this comment.
Finally, implementing the Map approach for VALIDATORS as you're doing here doesn't look like a bad idea either. It will solve both the pagination, and the size / limits issue. At the expense of some required code changes / adaptations.
In that case, the way to make the code above efficient would be to process only the updates and removals over the differences, i.a. as part of (or after) calculate_diff.
Anyway, the hybrid approach looks better in that it will require less reading / writing to storage. Particularly and if we reduce the ValidatorInfo footprint as suggested in #42.
ueco-jb
changed the title
ueco-jb
changed the title
|
Can be now closed. |
closes #33It won't be merged, as Ethan suggested better solution for that problem - but stays as reference for #42