Repository navigation
refactor(tests): bind the EIP-8141 calldata floor settlement tests by the minimum data - #3784
Conversation
… the minimum data
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/bogota/eip-8141 #3784 +/- ##
=======================================================
Coverage ? 94.94%
=======================================================
Files ? 676
Lines ? 41858
Branches ? 3942
=======================================================
Hits ? 39741
Misses ? 1465
Partials ? 652
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
I create a PR for some refactor, please let me know what you think about it:
| ) | ||
| worker_code = (write if with_state_write else Bytecode()) + Op.STOP | ||
| worker = pre.deploy_contract(code=worker_code) | ||
| state_used = write.state_cost(fork) if with_state_write else 0 |
There was a problem hiding this comment.
I think this is the same
| state_used = write.state_cost(fork) if with_state_write else 0 | |
| state_used = worker_code.state_cost(fork) |
| data_length -= 1 | ||
| tx, gas_used_before_refund, calldata_floor = build(data_length) | ||
| # The premise of the test: the refund applies uncapped. | ||
| assert 0 < refund <= gas_used_before_refund // 5 |
There was a problem hiding this comment.
I suggest not hardcode the number here, use max_quotient() function instead.
| gas_used = max( | ||
| execution_used(padding), floor_calculator(frames=frames(padding)) | ||
| ) |
There was a problem hiding this comment.
| gas_used = max( | |
| execution_used(padding), floor_calculator(frames=frames(padding)) | |
| ) | |
| execution_gas = execution_used(padding) | |
| calldata_floor = floor_calculator(frames=frames(padding)) | |
| # The premise of the test: the floor binds, or in `below_floor` | |
| # falls short by the last byte. | |
| assert (calldata_floor > execution_gas) == floor_bound | |
| gas_used = calldata_floor if floor_bound else execution_gas |
I know the premise of this test is already asserted by the helper but it is still worth making it explicit here imo
| tx, gas_used_before_refund, calldata_floor = build(data_length) | ||
| # The premise of the test: the refund applies uncapped. | ||
| assert 0 < refund <= gas_used_before_refund // 5 | ||
| gas_used_after_refund = gas_used_before_refund - refund |
There was a problem hiding this comment.
| gas_used_after_refund = gas_used_before_refund - refund | |
| gas_used_after_refund = gas_used_before_refund - refund | |
| # Assert the premise of each case | |
| if against_post_refund and one_byte_short: | |
| assert calldata_floor < gas_used_after_refund | |
| elif not against_post_refund and not one_byte_short: | |
| assert gas_used_before_refund < calldata_floor | |
| else: | |
| assert gas_used_after_refund < calldata_floor < gas_used_before_refund |
The premise of this test is a bit more complex and asserting it in full at this stage prevents silent drift. Also serves as a documentation of the premise here.
| @pytest.mark.parametrize( | ||
| "floor_case", | ||
| "against_post_refund,one_byte_short", | ||
| [ | ||
| pytest.param("below_post_refund", id="post_refund_above_floor"), | ||
| pytest.param("between", id="floor_between_pre_and_post_refund"), | ||
| pytest.param("above_pre_refund", id="floor_above_pre_refund"), | ||
| pytest.param(True, True, id="post_refund_above_floor"), | ||
| pytest.param(True, False, id="floor_between_pre_and_post_refund"), | ||
| pytest.param(False, True, id="floor_just_below_pre_refund"), | ||
| pytest.param(False, False, id="floor_above_pre_refund"), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
| @pytest.mark.parametrize( | |
| "floor_case", | |
| "against_post_refund,one_byte_short", | |
| [ | |
| pytest.param("below_post_refund", id="post_refund_above_floor"), | |
| pytest.param("between", id="floor_between_pre_and_post_refund"), | |
| pytest.param("above_pre_refund", id="floor_above_pre_refund"), | |
| pytest.param(True, True, id="post_refund_above_floor"), | |
| pytest.param(True, False, id="floor_between_pre_and_post_refund"), | |
| pytest.param(False, True, id="floor_just_below_pre_refund"), | |
| pytest.param(False, False, id="floor_above_pre_refund"), | |
| ], | |
| ) | |
| @pytest.mark.parametrize( | |
| "boundary,floor_side", | |
| [ | |
| pytest.param("post_refund", "below", id="floor_just_below_post_refund"), | |
| pytest.param("post_refund", "above", id="floor_just_above_post_refund"), | |
| pytest.param("pre_refund", "below", id="floor_just_below_pre_refund"), | |
| pytest.param("pre_refund", "above", id="floor_just_above_pre_refund"), | |
| ], | |
| ) |
Not sure the parameter names here are self-explanatory. Perhaps we should name the boundary (pre vs post) and the side (above vs below) more directly?
| sender = pre.fund_eoa() | ||
| write = Op.SSTORE(SLOT_CREATED, 1) | ||
| approve = Op.APPROVE(0, 0, Spec.APPROVE_EXECUTION_AND_PAYMENT) | ||
| # A contract sender carries no signature entries, so the charged data |
There was a problem hiding this comment.
This is not a blocker but more of a nice to have.
Rather than choosing between an EOA and a contract sender here, could the test take sender_kind as a parameter? The floor is composed differently in the two cases, so might be valuable to test both. The EOA case needs a little worker padding so the search precondition holds in no_state_write
PS: for the EOA case, the floor only sees signature bytes after signing.
| padding | ||
| ) | ||
|
|
||
| padding = floor_binding_padding(floor_binds) |
There was a problem hiding this comment.
Nit: the first test calls the data length padding while the refund test uses padding for bytecode and data_length for the data. data_length in both would avoid the overlap
… the minimum data (ethereum#3784) Co-authored-by: Guruprasad Kamath <48196632+gurukamath@users.noreply.github.com> Co-authored-by: 蔡佳誠 Louis Tsai <72684086+LouisTsai-Csie@users.noreply.github.com>
… the minimum data (#3784) Co-authored-by: Guruprasad Kamath <48196632+gurukamath@users.noreply.github.com> Co-authored-by: 蔡佳誠 Louis Tsai <72684086+LouisTsai-Csie@users.noreply.github.com>
Description
Follow-up to this review comment on #3756: the calldata floor settlement tests now bind the floor by the minimum data instead of a fixed margin.
floor_binding_paddingin the 8141 helpers, the binary search feat(spec-specs,test-forks,tests): charge EIP-8250 keyed nonce access in intrinsic gas and the calldata floor #3756 added for 8250, finds the fewest bytes of frame data that bind the floor.test_calldata_floor_with_state_gasdrops the 30,000-byteFLOOR_PADDING_DATAand addsbelow_floor, one byte short of binding. It now runs from a contract sender, which carries no signature bytes, so the floor starts below the standard cost, and each frame budget is exactly what it uses.test_storage_refund_settlementreplaces the 16-byte linear search with byte-exact boundaries against the post-refund and pre-refund gas, and addsfloor_just_below_pre_refund.Counting one floor byte too many in the spec fails all eight cases, and one too few fails the five where the floor binds.
Related Issues or PRs
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.