Handle protocol upgrades in congestion control

[wacban]

Congestion Control keeps track of the gas and size of the receipts in buffers by storing the current total in the headers and updating it with deltas as computed in the apply function.

The gas and size of a receipt is computed and added to the total when receipt is buffered and re-computed and subtracted from the total when the receipt is removed from the buffer. This sadly will break when the runtime parameters are changed for any receipt that is added to the buffer before protocol upgrade and removed after the protocol upgrade.

This is also a blocker for introducing a cost for the PromiseYield receipts which are currently unaccounted for as discovered in #11873.

[wacban]

cc @jancionear and @jakmeier

I can think of a few potential solutions to this issue:

• Store the receipts together with the gas and size when added and use that when removing the receipt from buffer.
• Store the receipts together with the protocol version when added ...
• Allow invalid gas and size. Reset to zero if it goes below zero or if the corresponding buffers / queues are empty.

I'm curious if you have any better ideas and if you have any preference for which solution is the best.

[jancionear]

Ah that sucks :/

I'm curious if you have any better ideas and if you have any preference for which solution is the best.

Store the receipts together with the gas and size when added and use that when removing the receipt from buffer.

This sounds like the most proper and robust solution :+1:. We can keep a few more bytes per receipt, it shouldn't matter to much. The only problem is that modifying the current code sounds like a huge pain :/ I think this would be the proper way to do it if we were doing it from scratch, but now with existing code I'm not sure if it's worth it...

Store the receipts together with the protocol version when added ...

In theory a bit more space efficient, but less robust. The headache doesn't feel worth it to me. We can keep 16 bytes more per receipt, it's not a big problem.

Allow invalid gas and size. Reset to zero if it goes below zero or if the corresponding buffers / queues are empty.

Sounds good to me :+1:. It's not the prettiest solution, but it'd solve the problem and wouldn't require too many code changes. I think I'd be in favor of this one :+1: There might be weird situations where we have a lot of receipts in the buffer and zero congestion, but it shouldn't be too bad. All the new receipts would start increasing congestion again and the shard shouldn't be overran.

[jakmeier]

Uhh yeah I completely missed this issue, thanks for uncovering it!

Store the receipts together with the gas and size when added and use that when removing the receipt from buffer.

I also think this is the most robust solution. Perhaps almost over-engineered but definitely a clean way to get it done.

Store the receipts together with the protocol version when added ...

Possible, yes. A bit more subtle to get right compared to solution 1 but still quite robust IMO. Having the protocol version per receipt would even be a bit more general and therefore perhaps advantageous for future changes.

I wonder if we could optimize this solution. Instead of storing the protocol version with every receipt, we could keep the meta data externally. We have long ranges of the same protocol version with monotonically increasing protocol versions, storing it all explicitly on every receipts seems a bit overkill. We can do it with constant overhead rather than liner in the number of receipts.

First idea that comes to mind: Augment the receipt buffers and queues with a map of protocol versions to first receipt index.

For example, assume the queue start is at index 100 and it holds 5 receipts with protocol versions [70, 70, 71, 71, 71], then we would store "first_receipt_of_version": { "70": 0, "71": 102 }. After we pop two more receipts from the front, we can remove the entry for 70 and are left with only "first_receipt_of_version": { "71": 102 } until the next protocol upgrade.

If we store this in the trie, we could look up the protocol version for the receipt every time we pop a receipt from it. And every time we push, we have to ensure the current protocol version is correct or otherwise add a new entry to the map. This adds a bit of overhead but I think it should be rather small. The map should never grow above length 2 under normal circumstances, so we can represent it as a vectorized list and simply store it directly in the same trie node as the start/end index of the queue. (Could just be a field of TrieQueueIndices) This way it would not increase the number of trie accesses. And code wise this logic can be contained almost entirely inside the existing trait TrieQueue.

Maybe you can also come up with a better design, that's just what I thought about.

But again, solution 1 is probably easier to implement and more robust for now. I would probably tend towards solution 1, if we can bare the extra bytes per receipt. But this second solution seems viable, too, and would be more efficient.

Allow invalid gas and size. Reset to zero if it goes below zero or if the corresponding buffers / queues are empty.

Hm,I don't like it. In many other projects I would probably praise the pragmatic approach but not in this context where we are talking about data structures at the core of the blockchain state transitions. If something doesn't add up, I don't want to have it ignored silently, unless we can be 100% certain it was due to a protocol upgrade.

Also, imagine you join a company and see this sort of solution in the code. Personally, I would call this technical debt left by my predecessors. I don't think we are under particular time pressure to get this done, so let's maybe avoid adding technical debt.

[wacban]

Also, imagine you join a company and see this sort of solution in the code. Personally, I would call this technical debt left by my predecessors. I don't think we are under particular time pressure to get this done, so let's maybe avoid adding technical debt.

Hehe thanks that made me laugh :) Nothing like some fresh legacy code :D

[wacban]

Thanks both, personally I'm leaning towards solution 1. I will attempt it first and get a feel for how much of a pain adding it would be. If it's too crazy I will come back and reconsider some of the other options.

[jancionear]

I was worried that modifying the runtime to handle all these cases would introduce so much complexity that it becomes worse than a little bit of hacky code. Complexity in runtime makes it harder to develop code for everyone, while the hacky bit matters only to people working on congestion control. For correctness we could still check that the gas drops to zero in the tests that don't do protocol changes. But it's worth a try, I don't have a strong opinion.