fix(p2p): move gas-limit admission out of GasTxValidator - #25221
Conversation
GasTxValidator re-ran the gas-limit check that the RPC factory's isSimulation exemption is meant to skip, so simulations with fee enforcement were rejected with TX_ERROR_GAS_LIMIT_TOO_HIGH. GasLimitsValidator is now the sole owner of declared gas-limit admission; gossip stage 1 and block building include it explicitly. Fixes #25167
| ]); | ||
| }); | ||
|
|
||
| it('rejects declared gas limits above the protocol ceiling', async () => { |
There was a problem hiding this comment.
Before this fix, block building got the ceiling implicitly through the check embedded in GasTxValidator. Since the fix removes that nested check, this test pins that block building didn't silently lose the ceiling.
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Code move only, no behavior change. GasLimitsValidator no longer shares ownership with GasTxValidator, so it gets its own gas_limits_validator.ts and test file, matching the one-validator-per-file layout of the directory. Exports still flow through the index barrel, so external imports are unaffected.
| ]); | ||
| }); | ||
|
|
||
| it('forwards the network admission limits to the gas limits validator', async () => { |
There was a problem hiding this comment.
This replaces forwards L2 limits through GasTxValidator which was removed from gas_validator.test.ts
| expect((result as { reason: string[] }).reason[0]).toContain(TX_ERROR_GAS_LIMIT_TOO_HIGH); | ||
| }); | ||
|
|
||
| it('forwards the network DA admission limit to the gas limits validator', async () => { |
There was a problem hiding this comment.
same as above but for forwards DA limits through GasTxValidator'
| return privateTx; | ||
| }; | ||
|
|
||
| describe('gas limits', () => { |
There was a problem hiding this comment.
moved to yarn-project/p2p/src/msg_validators/tx_validator/gas_limits_validator.test.ts
nchamo
left a comment
nchamo
left a comment
There was a problem hiding this comment.
Great work!
Some comments
| // `skipFeeEnforcement`, and GasTxValidator is constructed without the limit opts so it does not re-run | ||
| // this same check. | ||
| // `skipFeeEnforcement`, and GasTxValidator does not re-run this same check. | ||
| if (!isSimulation) { |
There was a problem hiding this comment.
GasTxValidator used to re-run GasLimitsValidator internally, so the minimum-limit check still applied during simulation even though createTxValidatorForAcceptingTxsOverRPC skips the explicit validator when isSimulation. With the delegation gone I think nothing enforces the minimum on that path now, so a simulate with under-minimum gasLimits would pass and then fail on sendTx. Estimation only ever needed the ceiling exempted, not the minimum. Could we add a test for that, and fix it if it holds?
There was a problem hiding this comment.
Good catch, I made a red-green to confirm. I split into MinGasLimitsValidator and MaxGasLimitsValidator. They feel like different types of checks as the MaxGasLimitsValidator actually has situations in which it is exempted.
| this.#gasLimitOpts = opts; | ||
| } | ||
|
|
||
| async validateTx(tx: Tx): Promise<TxValidationResult> { |
There was a problem hiding this comment.
An over-limit tx used to return here before validateTxFee ran. Now that the limit check is a separate validator and neither the aggregate nor gossip's Promise.all short-circuits, we still read the fee payer's balance from world state for a tx we're going to reject anyway. Can we avoid that?
There was a problem hiding this comment.
It looks we already have some world state lookups on rejected txs in the BlockHeaderTxValidator and the DoubleSpendTxValidator. So we would be going from 2 state lookups to 3. Short circuiting in the aggregate or gossip feels like a separate behavior change. I'm looking at a follow-up now but as I think this would require its own tests and is a more general behavior change it is best left as a separate change.
…x_simulator.ts Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar>
The declared gas-limit floor and ceiling had different exemption rules but shared one validator, so skipping the ceiling for gas estimation also skipped the floor. Splitting them lets the RPC factory keep the floor on the simulation path while exempting only the ceiling. A tx declaring less than the fixed protocol overheads can never be mined, so rejecting it during simulation is the earliest useful feedback rather than a surprise on sendTx.
|
❌ Failed to cherry-pick to |
3 participants
Fixes #25167
createTxValidatorForAcceptingTxsOverRPCskipsGasLimitsValidatorwhenisSimulationis set, since gas estimation submits intentionally-inflatedGasSettings.forEstimationlimits (2x the per-tx cap). That exemption is lost:GasTxValidator(added when fee enforcement is on) re-ran the same limit check internally. No optional limit defaults the ceiling toMAX_PROCESSABLE_L2_GASrather than disabling it.Any simulation with
skipFeeEnforcement: falsewas rejected withTX_ERROR_GAS_LIMIT_TOO_HIGH.GasTxValidatorentirely, andGasLimitsValidatoris split intoMinGasLimitsValidator(protocol overhead floor) andMaxGasLimitsValidator(per-tx ceiling), making the pair the sole owner of declared gas-limit admission. It is cleaner to an have an ordered list of filters and activate them given certain conditions rather than nesting filters. Nesting filters will inevtiably lead to more bugs like this one.Factories that relied on the embedded check now include it explicitly. The majority of the diff is some reorganization in
yarn-project/p2p/src/msg_validators/tx_validator/gas_validator.test.tsdue to the splitting of the validators.Behaviour change on the simulation path
The two limits have different exemption rules, which is why they are now separate filters: only the ceiling is exempted for
isSimulation. A tx declaring less than the fixed protocol overheads can never be mined, so rejecting it during simulation is the earliest useful feedback rather than a surprise onsendTx.That makes simulation stricter than the base branch, not just equal to it. The floor previously reached the simulation path only as a side effect of the nested call inside
GasTxValidator, so it was already absent wheneverskipFeeEnforcementwas set, which is the default forwallet.simulate(). The send path is unaffected either way, sinceisSimulationis only ever set byPXE.simulateTx.