Address POV Underestimations and remove base cost#243
Address POV Underestimations and remove base cost#243TarekkMA wants to merge 22 commits intomoonbeam-polkadot-stable2409from
Conversation
TarekkMA
force-pushed
the
tarekkma/remove-base-cost
branch
from
b06e9eb to
3588cb9
Compare
There was a problem hiding this comment.
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
frame/evm/src/runner/stack.rs:316
- By directly assigning actual_proof_size from measured_proof_size_after without subtracting any prior value, the previous proof cost is entirely dropped; please verify that this behavior correctly implements the intended removal of the base cost.
let actual_proof_size = measured_proof_size_after;
frame/ethereum/src/lib.rs:404
- [nitpick] Consider adding a comment here to explain why the condition 'weight_limit.proof_size() > 0' is used to determine transaction eligibility, which would help clarify the rationale behind transitioning to proof_size_pre_execution.
if weight_limit.proof_size() > 0 {
frame/evm/src/tests.rs:103
- [nitpick] Tests have been updated to pass 0 instead of Some(0); please ensure that there are also test cases covering scenarios where a non-zero proof_size_pre_execution is provided to validate the new logic.
weight_limit, 0,
541643c to
5fdde97
Compare
frame/ethereum/src/lib.rs
Outdated
| ), | ||
| _ => (None, None), | ||
| ); | ||
| let proof_size_pre_execution = get_proof_size().unwrap_or_default(); |
There was a problem hiding this comment.
get_proof_size return the proof size consumed buy the whole block so far, it's not per transaction (expect at tax validation). So, tests with one transaction might pass but it will not work with several transactions
There was a problem hiding this comment.
This is the proof_size pre execution, used later to compute the PoV difference between after and before evm call. The base cost should no longer be necessary after paritytech/polkadot-sdk#4765
Context: polkadot-evm#1490 (comment)
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
The host function get_proof_size return the current PoV consumed buy the block under construction, so we can't use it to measure the pov consumed by the transaction pre execution.
…to tarekkma/remove-base-cost
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
proof_size_pre_execution seeems to be never used, you should use it to compute the actual_proof_size
librelois
left a comment
librelois
left a comment
There was a problem hiding this comment.
In case of underestimation, the actual_proof_size should be normalize to the maximum that can be consumed according to the gas limit
|
Added this commit to enable the CI: 4ab4997 We need to merge the changes from moonbeam-polkadot-stable2409 to this branch and fix all PoV related errors. |
…to tarekkma/remove-base-cost
RomarQ
changed the base branch from
moonbeam-polkadot-stable2409
to
moonbeam-polkadot-stable2412
RomarQ
changed the base branch from
moonbeam-polkadot-stable2412
to
moonbeam-polkadot-stable2409
| let _ = weight_limit | ||
| .proof_size() | ||
| .checked_sub(proof_size_base_cost) | ||
| .checked_sub(proof_size_pre_execution) |
There was a problem hiding this comment.
This does not seem right, I would say that this check is no longer necessary
No description provided.