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.

Sign up for GitHub

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

Jump to bottom

pallet-revive: disable host functions not in revive recompiler #6844

Merged
merged 7 commits into from
Dec 12, 2024
Merged

pallet-revive: disable host functions not in revive recompiler #6844

merged 7 commits into from
Dec 12, 2024

Conversation

davidk-pt
Copy link
Contributor

Resolves #6720

List of used host functions in PolkaVM recompiler is here https://github.com/paritytech/revive/blob/main/crates/runtime-api/src/polkavm_imports.c#L65

@davidk-pt davidk-pt added the T7-smart_contracts This PR/Issue is related to smart contracts. label Dec 11, 2024
@davidk-pt davidk-pt requested review from athei and xermicus December 11, 2024 11:52
@davidk-pt davidk-pt mentioned this pull request Dec 11, 2024
Closed
@davidk-pt davidk-pt changed the title Disable host functions not in revive recompiler pallet-contracts: disable host functions not in revive recompiler Dec 11, 2024
@athei
Copy link
Member

athei commented Dec 11, 2024

You submitted your diff against the wrong pallet :)

@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/12276786843
Failed job name: run-frame-omni-bencher

@davidk-pt davidk-pt changed the title pallet-contracts: disable host functions not in revive recompiler pallet-revive: disable host functions not in revive recompiler Dec 11, 2024
@davidk-pt
Copy link
Contributor Author

You submitted your diff against the wrong pallet :)

My bad, made change against pallet-revive now, thought this was about old contracts pallet

@davidk-pt davidk-pt marked this pull request as ready for review December 11, 2024 14:08
athei
athei reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@athei athei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Can I ask you for a a follow up PR that is more like a refactoring?

  1. Sort the functions so that the not stable functions are all in the bottom so that they are grouped together.
  2. In the pallet-revive-uapi package feature guard all the non stable functions behind a feature flag like unstable-api. There might be more than the ones you just disabled which were already unstable.

Merge conflicts need to be resolved after the PR renaming the attributes was merged. Will approve after that.

@davidk-pt
Copy link
Contributor Author

Looks good. Can I ask you for a a follow up PR that is more like a refactoring?

  1. Sort the functions so that the not stable functions are all in the bottom so that they are grouped together.
  2. In the pallet-revive-uapi package feature guard all the non stable functions behind a feature flag like unstable-api. There might be more than the ones you just disabled which were already unstable.

Merge conflicts need to be resolved after the PR renaming the attributes was merged. Will approve after that.

Resolved merge conflicts, yeah I'll make a follow up PR

@davidk-pt davidk-pt requested a review from athei December 12, 2024 09:59
@davidk-pt davidk-pt enabled auto-merge December 12, 2024 10:51
@davidk-pt davidk-pt added this pull request to the merge queue Dec 12, 2024
xermicus
xermicus approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

LGTM and the PR branch passes the revive compiler test suite.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@athei
Copy link
Member

athei commented Dec 12, 2024

LGTM and the PR branch passes the revive compiler test suite.

You have unstable interfaces activated in your integration test suite, though.

@athei athei added this pull request to the merge queue Dec 12, 2024
@xermicus
Copy link
Member

LGTM and the PR branch passes the revive compiler test suite.

You have unstable interfaces activated in your integration test suite, though.

Good catch! It also passes without unstable interfaces 😅

Merged via the queue into master with commit f8e5a8a Dec 12, 2024
202 of 203 checks passed
@athei athei deleted the davidk/disable_host_functions_not_in_revive branch December 12, 2024 15:06
@davidk-pt davidk-pt mentioned this pull request Dec 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
@davidk-pt @athei
Add unstable-api feature flag to pallet-revive (#6866)
ec69b61 
Follow up refactor to
#6844 (review)

I still need to finish adding `#[cfg(feature = "unstable-api")]` to the
rest of the tests and make sure all tests pass, I want to make sure I'm
moving into right direction first

@athei @xermicus

---------

Co-authored-by: DavidK <davidk@parity.io>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
@davidk-pt @dudo50
pallet-revive: disable host functions not in revive recompiler (parit…
55d8f92 
…ytech#6844)

Resolves paritytech#6720

List of used host functions in PolkaVM recompiler is here
https://github.com/paritytech/revive/blob/main/crates/runtime-api/src/polkavm_imports.c#L65

---------

Co-authored-by: DavidK <davidk@parity.io>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
@davidk-pt @athei @dudo50
Add unstable-api feature flag to pallet-revive (paritytech#6866)
e95e85a 
Follow up refactor to
paritytech#6844 (review)

I still need to finish adding `#[cfg(feature = "unstable-api")]` to the
rest of the tests and make sure all tests pass, I want to make sure I'm
moving into right direction first

@athei @xermicus

---------

Co-authored-by: DavidK <davidk@parity.io>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xermicus xermicus xermicus approved these changes

@athei athei athei approved these changes

@pgherveou pgherveou pgherveou approved these changes

Assignees
No one assigned
Labels
T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable all host functions which are not needed for launch
4 participants
@davidk-pt @athei @xermicus @pgherveou