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

Derive StorageAccess for struct #3062

Merged

Conversation

maciejka
Copy link
Contributor

@maciejka maciejka commented May 8, 2023

This is an incomplete draft of storage_access derive for structs.

I am stuck at how to compose StorageAccess operations. For example for a struct:

struct AB {
  a: u256,
  b: u32
}

I would like to write it in a series of fields StorageAccess writes:

    fn write(address_domain: u32, base: starknet::StorageBaseAddress, value: AB) -> starknet::SyscallResult<()> {
        starknet::StorageAccess::<u256>::write(address_domain, base, value.a)?;
        starknet::StorageAccess::<u32>::write(address_domain, next_address, value.b)?;
        starknet::SyscallResult::Ok(())
    }

The problem is that information about space used by previous write is not available in the StorageAccess trait and I see no general way to calculate next_address.

@spapinistarkware any sugestions?


This change is Reviewable

@maciejka maciejka changed the title Derive storage_access for struct Derive StorageAccess for struct May 8, 2023
Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

One solution is const generics and exprs, which wedont haev yet and is hard.
Another solution is to ahve associated consts on the StorageAccess trait, which we also don't have yet. This might be a viable path though.
Another solution is to have annotations on the struct fields #[size(4)] as a temporary solution. But it won't be verified in any way, and can lead to bugs.

Reviewable status: 0 of 5 files reviewed, all discussions resolved

@maciejka
Copy link
Contributor Author

maciejka commented May 8, 2023 via email

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

There is no "written in stone" answer here. You can decide how the storage layout works. Yuo have all the ifnormation regarding how StorageAccess works.

Reviewable status: 0 of 5 files reviewed, all discussions resolved

@maciejka
Copy link
Contributor Author

I would either turn base argument into a reference:

trait StorageAccess<T> {
    fn read(address_domain: u32, ref base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, ref base: StorageBaseAddress, value: T) -> SyscallResult<()>;
}

or, if we want to save on memory operations in case of single storage operations, add another pair of read/write methods for consecutive operations:

trait StorageAccess<T> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, base: StorageBaseAddress, value: T) -> SyscallResult<()>;
    fn read_con(address_domain: u32, ref base: StorageBaseAddress) -> SyscallResult<T>;
    fn write_con(address_domain: u32, ref base: StorageBaseAddress, value: T) -> SyscallResult<()>;
}

WDYT?

@spapinistarkware
Copy link
Contributor

I would either turn base argument into a reference:

trait StorageAccess<T> {
    fn read(address_domain: u32, ref base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, ref base: StorageBaseAddress, value: T) -> SyscallResult<()>;
}

or, if we want to save on memory operations in case of single storage operations, add another pair of read/write methods for consecutive operations:

trait StorageAccess<T> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, base: StorageBaseAddress, value: T) -> SyscallResult<()>;
    fn read_con(address_domain: u32, ref base: StorageBaseAddress) -> SyscallResult<T>;
    fn write_con(address_domain: u32, ref base: StorageBaseAddress, value: T) -> SyscallResult<()>;
}

WDYT?

I think making it a ref is a good option.
@orizi ?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

this sounds like a good idea - but i think we'd maybe want to update this later somehow -
so if we can have a worse name for *_con variants - that would be even better.
maybe even add read_con_internal or something of that manner.

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, all discussions resolved

@maciejka
Copy link
Contributor Author

this sounds like a good idea - but i think we'd maybe want to update this later somehow - so if we can have a worse name for *_con variants - that would be even better. maybe even add read_con_internal or something of that manner.

Maybe just read_internal, write_internal?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

this lacks some of the meaning read_continuous_internal sounds very explicit - but it shouldn't be called directly anyway - so a bad name is good IMO.

Reviewable status: 2 of 5 files reviewed, all discussions resolved

@maciejka
Copy link
Contributor Author

maciejka commented May 11, 2023 via email

@maciejka maciejka marked this pull request as ready for review May 15, 2023 15:35
@maciejka maciejka requested a review from spapinistarkware May 15, 2023 15:37
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

a discussion (no related file):
you should just increase the u8 offset - not the base.
the idea of storage_address_from_base_and_offset(base, offset) is exactly the fact that we know this never fails, and base is limited to 2^251 - 256.


@maciejka
Copy link
Contributor Author

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

a discussion (no related file): you should just increase the u8 offset - not the base. the idea of storage_address_from_base_and_offset(base, offset) is exactly the fact that we know this never fails, and base is limited to 2^251 - 256.

Fair enough. In this case I would go with straightforward offset argument on consecuitive methods and additional :

trait StorageAccess<T> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, base: StorageBaseAddress, value: T) -> SyscallResult<()>;
    fn read_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8
    ) -> SyscallResult<T>;
    fn write_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8, value: T
    ) -> SyscallResult<()>;
    fn size_internal(value: T) -> u8;
}

with intention to use it like this:

impl StorageAccessU256 of StorageAccess<u256> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<u256> {
        StorageAccess::<u256>::read_consecutive_internal(address_domain, base, 0_u8)
    }
    fn write(address_domain: u32, base: StorageBaseAddress, value: u256) -> SyscallResult<()> {
        StorageAccess::<u256>::write_consecutive_internal(address_domain, base, 0_u8, value)
    }
    fn read_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8
    ) -> SyscallResult<u256> {
        let low = StorageAccess::<u128>::read(address_domain, base)?;
        let high = StorageAccess::<u128>::read_consecutive_internal(
            address_domain, base, offset + StorageAccess::<u128>::size_internal(low)
        )?;
        Result::Ok(
            u256 {
                low,
                high
            }
        )
    }
    fn write_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8, value: u256
    ) -> SyscallResult<()> {
        StorageAccess::<u128>::write(address_domain, base, offset, value.low)?;
        StorageAccess::<u128>::write(
            address_domain, base, offset + StorageAccess::<u128>::size_internal(value.low), value.high
        )?;
    }
}

WDYT?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

a discussion (no related file):

Previously, maciejka (Maciej Kamiński) wrote…

Fair enough. In this case I would go with straightforward offset argument on consecuitive methods and additional :

trait StorageAccess<T> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<T>;
    fn write(address_domain: u32, base: StorageBaseAddress, value: T) -> SyscallResult<()>;
    fn read_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8
    ) -> SyscallResult<T>;
    fn write_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8, value: T
    ) -> SyscallResult<()>;
    fn size_internal(value: T) -> u8;
}

with intention to use it like this:

impl StorageAccessU256 of StorageAccess<u256> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<u256> {
        StorageAccess::<u256>::read_consecutive_internal(address_domain, base, 0_u8)
    }
    fn write(address_domain: u32, base: StorageBaseAddress, value: u256) -> SyscallResult<()> {
        StorageAccess::<u256>::write_consecutive_internal(address_domain, base, 0_u8, value)
    }
    fn read_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8
    ) -> SyscallResult<u256> {
        let low = StorageAccess::<u128>::read(address_domain, base)?;
        let high = StorageAccess::<u128>::read_consecutive_internal(
            address_domain, base, offset + StorageAccess::<u128>::size_internal(low)
        )?;
        Result::Ok(
            u256 {
                low,
                high
            }
        )
    }
    fn write_consecutive_internal(
        address_domain: u32, base: StorageBaseAddress, offset: u8, value: u256
    ) -> SyscallResult<()> {
        StorageAccess::<u128>::write(address_domain, base, offset, value.low)?;
        StorageAccess::<u128>::write(
            address_domain, base, offset + StorageAccess::<u128>::size_internal(value.low), value.high
        )?;
    }
}

WDYT?

generally sound good.
write_consecutive_internal should probably now be write_at_offset_internal - but i'm still now sure if this would be preferable to just adding ref the offset value.


@maciejka
Copy link
Contributor Author

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

a discussion (no related file):

Previously, maciejka (Maciej Kamiński) wrote…
generally sound good. write_consecutive_internal should probably now be write_at_offset_internal - but i'm still now sure if this would be preferable to just adding ref the offset value.

Agree on write_at_offset_internal.

Adding ref to offset is problematic since offset has to be increased before it is clear that there will be next operation. Which might result in overflow in case of writing data of lenght == 256. I mean, if we have:

fn write_at_offset_internal(
    address_domain: u32, base: StorageBaseAddress, ref offset: u8, value: u256
) -> SyscallResult<()>

then what should it do when offset is 255?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

a discussion (no related file):

Previously, maciejka (Maciej Kamiński) wrote…

Agree on write_at_offset_internal.

Adding ref to offset is problematic since offset has to be increased before it is clear that there will be next operation. Which might result in overflow in case of writing data of lenght == 256. I mean, if we have:

fn write_at_offset_internal(
    address_domain: u32, base: StorageBaseAddress, ref offset: u8, value: u256
) -> SyscallResult<()>

then what should it do when offset is 255?

got it.


Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @maciejka and @spapinistarkware)


corelib/src/starknet/storage_access.cairo line 365 at r3 (raw file):

impl StorageAccessClassHash of StorageAccess<ClassHash> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<ClassHash> {
        StorageAccess::<ClassHash>::read_at_offset_internal(address_domain, base, 0_u8)

don't make existing implementations less efficient just for creating less code.

Suggestion:

        Result::Ok(
            StorageAccess::<felt252>::read(address_domain, base)?.try_into().expect('Non ClassHash')
        )

@maciejka
Copy link
Contributor Author

Reviewed all commit messages.
Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @maciejka and @spapinistarkware)

corelib/src/starknet/storage_access.cairo line 365 at r3 (raw file):

impl StorageAccessClassHash of StorageAccess<ClassHash> {
    fn read(address_domain: u32, base: StorageBaseAddress) -> SyscallResult<ClassHash> {
        StorageAccess::<ClassHash>::read_at_offset_internal(address_domain, base, 0_u8)

don't make existing implementations less efficient just for creating less code.

Suggestion:

        Result::Ok(
            StorageAccess::<felt252>::read(address_domain, base)?.try_into().expect('Non ClassHash')
        )

I've been thinking about exactly this. I just wanted to make it work first before optimising.

Another question: What should be the critiria for adding #[inline(always)] to StorageAccess read/write methods. It is not clear to mewhy the write methods are usually inlined when read are not?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r1, 1 of 17 files at r4, 1 of 7 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: 10 of 25 files reviewed, 3 unresolved discussions (waiting on @maciejka and @spapinistarkware)


corelib/src/starknet/storage_access.cairo line 97 at r7 (raw file):

impl StorageAccessBool of StorageAccess<bool> {
    #[inline(always)]

make this in another pr - to reduce the changes in generation sierra code.

Code quote:

#[inline(always)]

crates/cairo-lang-starknet/src/plugin/events.rs line 206 at r6 (raw file):

Previously, maciejka (Maciej Kamiński) wrote…

Removed unnecesary comments.
What's wrong with derive_event_needed? It returns true if event emission functions should be derived for a type. Name makes sense to me.

didn't ee you added a parallel function for the storage access - all good.


crates/cairo-lang-starknet/src/plugin/storage_access.rs line 107 at r7 (raw file):

}

pub fn derive_storage_access_needed<T: QueryAttrs>(with_attrs: &T, db: &dyn SyntaxGroup) -> bool {

doc

Code quote:

derive_storage_access_needed

@maciejka
Copy link
Contributor Author

Reviewed 1 of 5 files at r1, 1 of 17 files at r4, 1 of 7 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: 10 of 25 files reviewed, 3 unresolved discussions (waiting on @maciejka and @spapinistarkware)

corelib/src/starknet/storage_access.cairo line 97 at r7 (raw file):

impl StorageAccessBool of StorageAccess<bool> {
    #[inline(always)]

make this in another pr - to reduce the changes in generation sierra code.

Code quote:

#[inline(always)]

crates/cairo-lang-starknet/src/plugin/events.rs line 206 at r6 (raw file):

Previously, maciejka (Maciej Kamiński) wrote…
didn't ee you added a parallel function for the storage access - all good.

crates/cairo-lang-starknet/src/plugin/storage_access.rs line 107 at r7 (raw file):

}

pub fn derive_storage_access_needed<T: QueryAttrs>(with_attrs: &T, db: &dyn SyntaxGroup) -> bool {

doc

Code quote:

derive_storage_access_needed

Done. There are still changes in erc20 sierra code. Maybe it is due to this change: https://github.com/starkware-libs/cairo/pull/3062/files#diff-4feb70577299f5836d5feacb9c62326358848bdfe48afcf5de19f51a8663e1f9R313 ? Should I delegate it to a separate pr as well?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 8 files at r2, 9 of 18 files at r5, 8 of 8 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @maciejka and @spapinistarkware)


crates/cairo-lang-starknet/src/plugin/storage_access.rs line 107 at r7 (raw file):

Previously, maciejka (Maciej Kamiński) wrote…

Done. There are still changes in erc20 sierra code. Maybe it is due to this change: https://github.com/starkware-libs/cairo/pull/3062/files#diff-4feb70577299f5836d5feacb9c62326358848bdfe48afcf5de19f51a8663e1f9R313 ? Should I delegate it to a separate pr as well?

you still need the inline of write_at_offset_internal to remove the changes - aside from that it should be fine.

@maciejka
Copy link
Contributor Author

Reviewed 1 of 8 files at r2, 9 of 18 files at r5, 8 of 8 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @maciejka and @spapinistarkware)

crates/cairo-lang-starknet/src/plugin/storage_access.rs line 107 at r7 (raw file):

Previously, maciejka (Maciej Kamiński) wrote…
you still need the inline of write_at_offset_internal to remove the changes - aside from that it should be fine.

Is it ok now?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)


crates/cairo-lang-starknet/src/plugin/storage_access.rs line 107 at r7 (raw file):

Previously, maciejka (Maciej Kamiński) wrote…

Is it ok now?

looks good!

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciejka and @spapinistarkware)

@maciejka
Copy link
Contributor Author

Any chance of merging this any time soon? @spapinistarkware

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 5 files at r1, 2 of 8 files at r2, 1 of 17 files at r4, 10 of 18 files at r5, 3 of 4 files at r7, 3 of 8 files at r8, 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciejka)

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciejka)

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciejka)

@spapinistarkware spapinistarkware added this pull request to the merge queue May 23, 2023
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @maciejka)

Merged via the queue into starkware-libs:main with commit 9203b80 May 23, 2023
@milancermak
Copy link
Contributor

Why does size_internal take a value argument? Isn't the type size independent of its value?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

since there are options where it would be dependent - for example - if we implement this for Option<VeryLargeStruct> - we would probably decide to not write as much in the None case.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Not sure I agree here. This is not Serde where we always write sequentially. We need to be able to write out of order, and we need a constant size slot in storage

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@maciejka
Copy link
Contributor Author

Not sure I agree here. This is not Serde where we always write sequentially. We need to be able to write out of order, and we need a constant size slot in storage

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Defining a model of underlying storage would definitely help. What do you mean by "we need to be able to write out of order"?

@milancermak
Copy link
Contributor

Not being dependent on the value would also allow for some kind of lazy loading or writing only the dirty values (not sure how that would look like in practice tho).

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

but i think this should be up to the user (be it through the usage of another wrapping trait or something of that manner) - but i don't think it should block the other option.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

milancermak pushed a commit to milancermak/cairo that referenced this pull request May 23, 2023
@maciejka
Copy link
Contributor Author

It is not clear to me what is the outcome of recent considerations about sizeInternal being value dependent. Should it be changed somehow? What are the usecases it is not compatible with? It would be better to do it before current version is released...

It seems like there are a few simple tasks left to complete introduction of #derive(storage_access::StorageAccess) into the code base:

  • use it for starknet builtin types like: u256, EthAddress
  • use it for Dispatcher's generated for #[abi]
  • add #[inline] to StorageAccess impls for builtin types
    Anything else?
    WDYT?

@maciejka
Copy link
Contributor Author

It seems like there are a few simple tasks left to complete introduction of #derive(storage_access::StorageAccess) into the code base:

  • use it for starknet builtin types like: u256, EthAddress
  • use it for Dispatcher's generated for #[abi]

Here is a pr deriving StorageAccess for u256, EthAddress, Dispatcher: #3247

@moodysalem
Copy link

moodysalem commented May 27, 2023

This is a breaking change so I would personally think it better to just remove the old methods. If you're optimizing for gas I imagine you're going to write custom implementations of StorageAccess, since tx cost should be dominated by posting storage diffs to L1 so you'll want to do maximum packing, and the multiple methods to do the same thing is a bit annoying

Also if the size is constant it could be a generic const parameter. Not sure if this is possible but I would prefer constant size where this is used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants