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

feat: [EXC-1753] Add mint_cycles128 API #2589

Merged
merged 50 commits into from
Dec 11, 2024
Merged

feat: [EXC-1753] Add mint_cycles128 API #2589

merged 50 commits into from
Dec 11, 2024

Conversation

michael-weigelt
Copy link
Contributor

@michael-weigelt michael-weigelt commented Nov 13, 2024

This PR adds a new system API method ic0_mint_cycles128 which, like its predecessor ic0_mint_cycles, is not usable by any canister except the CMC. It is therefore not necessary to add it to the interface spec or the SDKs. It also means that this system call does not need an instruction cost adjustment.

The method's signature deviates from the predecessor because 128 bit values cannot be handled by WASM natively. Therefore, the argument is split into two u64s, representing the high and low bits. The amount of actually minted cycles cannot be returned as a WASM return value either. Instead, the result is written to a caller-specified location in memory, where it occupies the next 16 bytes.

@github-actions github-actions bot added the feat label Nov 13, 2024
@michael-weigelt michael-weigelt marked this pull request as ready for review November 22, 2024 13:47
@michael-weigelt michael-weigelt requested a review from a team as a code owner November 22, 2024 13:47
@michael-weigelt
Copy link
Contributor Author

Note: The benchmark results will be added this week after the wasmtime upgrade.

berestovskyy
berestovskyy previously approved these changes Nov 25, 2024
Copy link
Contributor

@berestovskyy berestovskyy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

rs/system_api/src/lib.rs Outdated Show resolved Hide resolved
@michael-weigelt michael-weigelt added this pull request to the merge queue Dec 11, 2024
Merged via the queue into master with commit 858c038 Dec 11, 2024
25 checks passed
@michael-weigelt michael-weigelt deleted the mwe/cycles128 branch December 11, 2024 12:50
cgundy added a commit that referenced this pull request Dec 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2024
@michael-weigelt michael-weigelt restored the mwe/cycles128 branch December 12, 2024 12:44
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
)

#2589 caused a problem in a backup
test in the hourly pipeline, so it was reverted in #3125.

#3147 addressed the underlying problem
in the backup test and was merged.
#3151 is 2589+3147 and shows that the
mitigation works, but instead of merging that, it's nicer to revert the
revert:

This PR reverts #3125
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants