Skip to content

Add new host APIs set_storage_or_clear and get_storage_or_zero#7857

Merged
athei merged 22 commits intomasterfrom
castilla-new-uapi
Apr 25, 2025
Merged

Add new host APIs set_storage_or_clear and get_storage_or_zero#7857
athei merged 22 commits intomasterfrom
castilla-new-uapi

Conversation

@castillax
Copy link
Copy Markdown

@castillax castillax commented Mar 10, 2025

Description

This PR introduces two new storage API functions—set_storage_or_clear and get_storage_or_zero—which provide fixed‑size (32‑byte) storage operations. These APIs are an attempt to match Ethereum’s SSTORE semantics. These APIs provide additional functionality for setting and retrieving storage values and clearing storage when a zero value is provided and returning zero bytes when a key does not exist.

Fixes #6944

Review Notes

  • Changes in runtime.rs
    Added the set_storage_or_clear function to set storage at a fixed 256-bit key with a fixed 256-bit value. If the provided value is all zeros, the key is cleared.
    Added the get_storage_or_zero function to read storage at a fixed 256-bit key and write back a fixed 256-bit value. If the key does not exist, 32 bytes of zero are written back.
  • Changes in storage.rs
    Added test cases to cover the new set_storage_or_clear and get_storage_or_zero APIs.
    .
// Example usage of the new set_storage_or_clear function
let existing = api::set_storage_or_clear(StorageFlags::empty(), &KEY, &VALUE_A);
assert_eq!(existing, None);

// Example usage of the new get_storage_or_zero function
let mut stored: [u8; 32] = [0u8; 32];
let _ = api::get_storage_or_zero(StorageFlags::empty(), &KEY, &mut stored);
assert_eq!(stored, VALUE_A);

All existing tests pass

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@xermicus xermicus added the T7-smart_contracts This PR/Issue is related to smart contracts. label Mar 10, 2025
Copy link
Copy Markdown
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.

You need to write benchmarks and charge weights. Additionally we should consider transient storage for the new host functions too.

To minimize code duplication I'd probably factor out the logic from set_storage and get_storage into one or more internal methods and add a boolean flag or something similar to specify whether the clearing / zero-ing is wanted. The exposed runtime APIs are then just thin wrappers around those.

@castillax
Copy link
Copy Markdown
Author

/cmd prdoc --audience runtime_dev --bump minor

Copy link
Copy Markdown
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.

Yeah a separate test would be nice. Will give the test code more on depth review when the other comments are resolved.

@castillax castillax removed the request for review from pgherveou March 11, 2025 17:23
@paritytech-workflow-stopper
Copy link
Copy Markdown

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

@athei
Copy link
Copy Markdown
Member

athei commented Mar 18, 2025

I know it seems pedantic. But please spent some time to come up with useful docs and names.

@athei
Copy link
Copy Markdown
Member

athei commented Apr 23, 2025

Looks good. But before we can merge new stable functions we need to make use of them in revive. So we need to wait until @xermicus has time to implement that in revive or we merge as unstable.

Copy link
Copy Markdown
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.

We verified that it works in revive.

Copy link
Copy Markdown
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.

The revive test suite passes against this branch. Modulo some unrelated test somehow failing and that for external observers there is a semantic difference visible between a storage key holding zero vs. no storage key. Which will no longer be observable. I think this is fine because for the contract it isn't observable.

@pgherveou
Copy link
Copy Markdown
Contributor

Would the new variants influence the benchmarks?
right now the set_storage benchmark always use the storageValue::Memory variant
and the get_storage the StorageReadMode::Standard

@athei
Copy link
Copy Markdown
Member

athei commented Apr 25, 2025

All storage access is benchmarked with the maximum key length. So we are still safe here since we are using smaller keys. The maximum for keys is only 128 bytes. It is negligible. We won't gain much by benchmarking with the exact key size.

@athei athei added this pull request to the merge queue Apr 25, 2025
Merged via the queue into master with commit 7e5b993 Apr 25, 2025
240 of 249 checks passed
@athei athei deleted the castilla-new-uapi branch April 25, 2025 16:49
wassimans pushed a commit to wassimans/polkadot-sdk that referenced this pull request Apr 27, 2025
…ytech#7857)

# Description

*This PR introduces two new storage API functions—set_storage_or_clear
and get_storage_or_zero—which provide fixed‑size (32‑byte) storage
operations. These APIs are an attempt to match Ethereum’s SSTORE
semantics. These APIs provide additional functionality for setting and
retrieving storage values and clearing storage when a zero value is
provided and returning zero bytes when a key does not exist.*

Fixes paritytech#6944 
## Review Notes

* Changes in `runtime.rs`
Added the set_storage_or_clear function to set storage at a fixed
256-bit key with a fixed 256-bit value. If the provided value is all
zeros, the key is cleared.
Added the get_storage_or_zero function to read storage at a fixed
256-bit key and write back a fixed 256-bit value. If the key does not
exist, 32 bytes of zero are written back.
* Changes in `storage.rs`
Added test cases to cover the new set_storage_or_clear and
get_storage_or_zero APIs.
.

```
// Example usage of the new set_storage_or_clear function
let existing = api::set_storage_or_clear(StorageFlags::empty(), &KEY, &VALUE_A);
assert_eq!(existing, None);

// Example usage of the new get_storage_or_zero function
let mut stored: [u8; 32] = [0u8; 32];
let _ = api::get_storage_or_zero(StorageFlags::empty(), &KEY, &mut stored);
assert_eq!(stored, VALUE_A);
```

*All existing tests pass*

# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>
Co-authored-by: xermicus <[email protected]>
ordian added a commit that referenced this pull request Apr 28, 2025
* master: (120 commits)
  [CI] Improve GH build status checking (#8331)
  [CI/CD] Use original PR name in prdoc check for the backport PR's to the stable branches (#8329)
  Add new host APIs set_storage_or_clear and get_storage_or_zero (#7857)
  push to dockerhub (#8322)
  Snowbridge - V1 - Adds 2 hop transfer to Rococo (#7956)
  [AHM] Prepare `election-provider-multi-block` for full lazy data deletion (#8304)
  Check umbrella version (#8250)
  [AHM] Fully bound staking async (#8303)
  migrate parachain-templates tests to `gha` (#8226)
  staking-async: add missing new_session_genesis (#8310)
  New NFT traits: granular and abstract interface (#5620)
  Extract create_pool_with_native_on macro to common crate (#8289)
  XCMP: use batching when enqueuing inbound messages (#8021)
  Snowbridge - Tests refactor (#8014)
  Allow configuration of worst case buy execution weight (#7944)
  Fix faulty pre-upgrade migration check in pallet-session (#8294)
  [pallet-revive] add get_storage_var_key for variable-sized keys (#8274)
  add poke_deposit extrinsic to pallet-recovery (#7882)
  `txpool`: use tracing for structured logging (#8001)
  [revive] eth-rpc refactoring (#8148)
  ...
castillax pushed a commit that referenced this pull request May 12, 2025
# Description

*This PR introduces two new storage API functions—set_storage_or_clear
and get_storage_or_zero—which provide fixed‑size (32‑byte) storage
operations. These APIs are an attempt to match Ethereum’s SSTORE
semantics. These APIs provide additional functionality for setting and
retrieving storage values and clearing storage when a zero value is
provided and returning zero bytes when a key does not exist.*

Fixes #6944 
## Review Notes

* Changes in `runtime.rs`
Added the set_storage_or_clear function to set storage at a fixed
256-bit key with a fixed 256-bit value. If the provided value is all
zeros, the key is cleared.
Added the get_storage_or_zero function to read storage at a fixed
256-bit key and write back a fixed 256-bit value. If the key does not
exist, 32 bytes of zero are written back.
* Changes in `storage.rs`
Added test cases to cover the new set_storage_or_clear and
get_storage_or_zero APIs.
.

```
// Example usage of the new set_storage_or_clear function
let existing = api::set_storage_or_clear(StorageFlags::empty(), &KEY, &VALUE_A);
assert_eq!(existing, None);

// Example usage of the new get_storage_or_zero function
let mut stored: [u8; 32] = [0u8; 32];
let _ = api::get_storage_or_zero(StorageFlags::empty(), &KEY, &mut stored);
assert_eq!(stored, VALUE_A);
```

*All existing tests pass*

# Checklist

* [x] My PR includes a detailed description as outlined in the
"Description" and its two subsections above.
* [x] My PR follows the [labeling requirements](

https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process
) of this project (at minimum one label for `T` required)
* External contributors: ask maintainers to put the right label on your
PR.
* [x] I have made corresponding changes to the documentation (if
applicable)
* [x] I have added tests that prove my fix is effective or that my
feature works (if applicable)

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Alexander Theißen <[email protected]>
Co-authored-by: xermicus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T7-smart_contracts This PR/Issue is related to smart contracts.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Create a set_storage_or_clear that deletes an storage item if 0u256 is passed as value

4 participants