StorageWeightReclaim: Fix issue when underestimating refund.

[gui1117]

The code do reduce or increase the weight by comparing benchmarked_weight and consumed_weight.

But benchmarked_weight is the pre dispatch weight. not the post dispatch weight that is actually written into the block weight by CheckWeight.

So in case the consumed weight was: pre dispatch weight > consumed weight > post dispatch weight then the reclaim code was reducing the block weight instead of increasing it.

Might explain this issue even better #5229

@skunert
@s0me0ne-unkn0wn

[skunert]

Ouch 🤦.

Good find! @s0me0ne-unkn0wn can you throw your verification zombienet on this branch and see if it changes things?

[skunert]

I am thinking about the impact this bug has.

The actual_weight in the post info will be useful if you know during execution that you take a specific codepath and basically return only a subset of the weight. However that part that you return as actual_weight should still be benchmarked and an overestimation right? Not saying it can't happen, but my impression is that in most cases the actual_weight should still be bigger than the real used proof size on the node.

[s0me0ne-unkn0wn]

From my pov, it doesn't improve the situation, probably even making it a bit worse.

Runtime block weight info:

{
  normal: {
    refTime: 1,230,370,849,000
    proofSize: 2,923,186
  }
  operational: {
    refTime: 0
    proofSize: 0
  }
  mandatory: {
    refTime: 5,203,319,000
    proofSize: 77,900
  }
}

Registered by node: PoV size { header: 0.216796875kb, extrinsics: 626.0771484375kb, storage_proof: 5043.0947265625kb }

Something's wrong, although I haven't run with full logs yet, maybe it's something obvious. Will try later today.

[skunert]

probably even making it a bit worse

How is it worse? Looks unchanged to the results you posted in the past.

The scenario here is only occuring if the PostDispatchInfo contains an actual_weight that is lower than the consumed proof size. As far as I see the current situation where we miss out measuring the proof size of some of the SEs makes this even more unlikely to surface.

[s0me0ne-unkn0wn]

How is it worse? Looks unchanged to the results you posted in the past.

Yes, you're right. I forgot that in the previous round of tests, the collator's PoV limit was raised to 6 Mb, so these numbers seemed a bit too low to me.

And IIUC, this PR only changes how unspent is handled, and I've never observed non-zero unspent in my tests 🤷