Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: shared mutable storage #5490
feat: shared mutable storage #5490
Changes from 13 commits
9e8e40c
a52933a
0c3495f
cd334e0
2165fc9
3e28aef
a0d94f4
60d57f1
92853d2
3740ac4
8c85c60
2eec474
367235f
cbb4821
4a8fd2c
6da0b4b
5eff481
55ecfac
f567507
dc75345
c987857
8517458
92646b0
ac96461
8e95c72
39aab8e
bbeefc6
65b5605
74dc42c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something feels weird here, that if I read really far back, the "pre" might not be the actual value.
Like. At block 10 value might be
(a,b,c)
and then later it becomes(b,d,e)
but If I then say read at 10, I would read the pre asb
.It makes sense, was just from the naming that it is what sprung to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow your example, could you provide concrete values for
c
ande
?But yes, values do move from
post
topre
when a new change is scheduled. Clients that have not seen the new scheduled change will readpost
, while clients that have seen the change will readpre
. The important thing is that these values will be the same, and all reads will be valid (as long asmax_block_number
is properly constrained).This is what the
assert_block_horizon_invariants
helper function tests:current_value
returns the same value at the historical block number and at the block horizon, even if a new change is scheduled as early as possible (i.e. in the block following the historical block).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking about the -1. So we are having the -1 to make sure that the block horizon ends before the change occurs.
But probably be nicce to have some better clarity on why it is not needed for the
historical_block_number + DELAY
.Like, If I m just thinking of an execution where you havae
Which sounds weird?
But as we are constraining the functions in the
shared_mutable
to only use theget_block_horizon
in private, you cannot really get this case, as the current cannot be used.Rambly rambler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what the comments and the diagram above try to convey: the earliest a new block of change could be is
historical + 1 + DELAY
, so the missing-1
is because it cancels out the+1
.Re.
get_block_horizon
, yes, its meant to be a value consumed in private with some historical block number. If you pass the current block in public then you'll get an invalid horizon, because as you point out you could immediately schedule a new change in the same block.I'll add some comments and do some light renamings to make it clearer that
get_current_at()
can be called in both private and public, whileget_block_horizon()
should be private only (unless you know what you're doing).Regardless, this is ultimately a helper data structure that would likely only be used by
SharedMutable
and not an end user, so I'm not too worried.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would likely be nice to test a case with larger structs for the pre and post to ensure working as expected, looks mainly to be single fields here?
It might be too disgusting to get working before we can do
N*2+1
as the size of the serialization though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
ScheduledValueChange
by itself would be fine since it doesn't useT
for anything, the problem will be accesing storage due to incomplete support for numeric generics in Noir, as mentioned in #5492.