Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pallet-revive] ecrecover #7652

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

[pallet-revive] ecrecover #7652

wants to merge 29 commits into from

Conversation

pgherveou
Copy link
Contributor

@pgherveou pgherveou commented Feb 20, 2025

Add ECrecover 0x1 precompile and remove the unstable equivalent host function.

@pgherveou pgherveou marked this pull request as ready for review February 20, 2025 21:46
@pgherveou
Copy link
Contributor Author

/cmd prdoc --audience runtime_dev --bump minor

@pgherveou pgherveou added T7-smart_contracts This PR/Issue is related to smart contracts. R0-silent Changes should not be mentioned in any release notes labels Feb 20, 2025
@pgherveou pgherveou requested review from athei and xermicus and removed request for athei February 21, 2025 11:41
@athei
Copy link
Member

athei commented Feb 21, 2025

Please make this less invasive. We know all precompiles we are adding here are pure functions. Just call them directly from the Ext::call. Don't push a frame for them or anything. This will remove all the special casing you are doing right now and reduce the diff to a minimum. Please also rename precompiles.rs to basic_precompiles.rs.

Both of those points will keep the merge conflicts I have to deal with down.

@pgherveou
Copy link
Contributor Author

pgherveou commented Feb 21, 2025

Please make this less invasive. We know all precompiles we are adding here are pure functions. Just call them directly from the Ext::call. Don't push a frame for them or anything. This will remove all the special casing you are doing right now and reduce the diff to a minimum. Please also rename precompiles.rs to basic_precompiles.rs.

Both of those points will keep the merge conflicts I have to deal with down.

that's what I did initially, but you also need to duplicate all the tracing logic if you do it, and handle value transfer

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. But please still just duplicate the logic into precompiles.rs then. I want this PR to have minimal impact on exec.rs. First, because making contract_info optional generates a lot of changes I don't want to backport. Secondly, the merge conflicts I will get from this will break my brain. I checked my branch and it will be bad.

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather just duplicate the value transfer logic here to keep the diff easy.

@pgherveou
Copy link
Contributor Author

I understand. But please still just duplicate the logic into precompiles.rs then. I want this PR to have minimal impact on exec.rs. First, because making contract_info optional generates a lot of changes I don't want to backport. Secondly, the merge conflicts I will get from this will break my brain. I checked my branch and it will be bad.

Sounds good then, you have a branch somewhere with another version of precompile implemented?

@pgherveou pgherveou changed the base branch from master to pg/fix_tracing_2 February 23, 2025 22:50
@pgherveou pgherveou requested a review from athei February 23, 2025 23:27
@athei
Copy link
Member

athei commented Feb 24, 2025

I understand. But please still just duplicate the logic into precompiles.rs then. I want this PR to have minimal impact on exec.rs. First, because making contract_info optional generates a lot of changes I don't want to backport. Secondly, the merge conflicts I will get from this will break my brain. I checked my branch and it will be bad.

Sounds good then, you have a branch somewhere with another version of precompile implemented?

Yes its at/precompiles. But its a bit old and needs rebasing. And I am not 100% sure that the way I am doing it is how we want it. But my point is that it needs more thought and I want to defer the decision for after the release. Hence the requested copy paste solution here.

@pgherveou
Copy link
Contributor Author

Ok did the refactoring yesterday
Should be fine now might have missed a few things will come back to check ci logs later


impl<T: Config> Precompile<T> for ECRecover {
fn execute(gas_meter: &mut GasMeter<T>, i: &[u8]) -> ExecResult {
gas_meter.charge(RuntimeCosts::EcdsaRecovery)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct. But we also need to not charge the contract call gas in case of a pre-compile. Otherwise it will be super expensive since the contract call gas does include accessing the contract code storage item.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep the diff small here too and do it naively like this

54f6105

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13502890532
Failed job name: fmt

Base automatically changed from pg/fix_tracing_2 to master February 24, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants