Fix the epoch used for gas in the message pool & validation #8163
Conversation
We need to use the height at which the messages will be executed, not the height of the previous tipset. This brings the gas checking for validation with the gas we actually _charge_ during message execution. This only matters for the Calico upgrade (the only upgrade where we changed the gas prices). This change could potentially cause a block at that height to be rejected if it includes a message with insufficient gas for inclusion. However, that _should_ have shown up as a miner penalty when we executed the blocks in the following tipset. Given that there were no miner penalties at 265199-265201, this change should be "safe".
Previously, we checked message gas/validity with the previous block's height. This doesn't affect consensus, but will help us avoid adding messages to the message pool that shouldn't be there.
Codecov Report
@@ Coverage Diff @@
## master #8163 +/- ##
==========================================
- Coverage 39.48% 39.45% -0.04%
==========================================
Files 666 666
Lines 72498 72498
==========================================
- Hits 28627 28603 -24
- Misses 38915 38933 +18
- Partials 4956 4962 +6
Continue to review full report at Codecov.
|
|
The right way to test this is to:
But doing that sounds... painful. Thoughts? |
There was a problem hiding this comment.
@Stebalien I'm comfortable landing this as-is, I think it's a strict improvement (and also can't find any messages that would break consensus around the Calico upgrade).
The test you're describing wouldn't be that hard to write. I can do it, but it's probably not the highest priority, so it's up to you whether you wanna block landing this on such a test.
Also, just to confirm our understandings align, the reason this was affecting the recent caterpillarnet setups was because the pricelist was changing at the first epoch (even though the Calico upgrade was disabled) because Lotus always starts with the v0 pricelist (for epoch 0) before then switching if appropriate. Is that your take?
If so, an easy improvement might be to change that -- the first pricelist should be dictated by a network's Genesis network version. That wouldn't affect mainnet, and would mean any new networks start from the v1 pricelist. I think it would affect calibnet, though...
Unfortunately, this change isn't actually relevant to that issue. I'm just fixing what I can without breaking anything. I'll let you merge when/if ready. As you say, writing a test for this isn't high priority. |
Related Issues
Partially fixes #6652.
Proposed Changes
Bring the gas epoch used in chain validation in-line with the gas epoch used in message execution.
Additional Info
This change should only affect the one gas repricing we had (Calico). It should only affect consensus if the blocks at the upgrade height included a message without enough gas to cover the message's execution. In the old code:
Looking at the relevant blocks around 265200 (the Calico upgrade height), I'm not seeing any miner penalties so I have to assume that this didn't happen.
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps