Incorrect ContractCostType::MapEntry calibration

[dmkozh]

ContractCostType::MapEntry represents a cost of accessing a single entry in the map, but is benchmarked using get function, that performs a binary search and then charges one more MapEntry access. Instead, benchmark should be using get_at_index which only charges a single MapEntry.

This results in overcharging_, thus there are no security risks. However, maps are used a lot for contract types and it would be nice to be more precise about them.

[dmkozh]

Also since map is implemented via vec, the cost could in theory be the same as VecEntry (the latter is currently 0 though...)

[jayz22]

This is correct.
Although the calibration calls get, which does a binary search. The budget keeps track of the correct number of times the MapEntry is accessed (log(N) times per get call). So in the end the calibrated result "total_instructions / total_number_of_map_entry_call" is correct.

Map is implemented with Vec so the two cost types could merge. There is a slight difference though. MeteredVector is mostly used for HostVec impl, which stores a u64 Val (and is calibrated with that assumption), while MeteredMap stores a pair of u64s -- double the entry size. The difference in calibration result is small.
Keeping both cost types allows more freedom in the future ( the fact that MeteredMap is implemented via MeteredVec is just an implementation detail).

[dmkozh]

So in the end the calibrated result "total_instructions / total_number_of_map_entry_call" is correct.

Good to know, thanks! Then we can close this issue.

Keeping both cost types allows more freedom in the future ( the fact that MeteredMap is implemented via MeteredVec is just an implementation detail).

Yeah, I'm fine with that. However, the calibrated value difference is a bit concerning (especially given that we have 0 for vec).

[jayz22]

However, the calibrated value difference is a bit concerning (especially given that we have 0 for vec).

I'll look into that as well as the difference in #1051