Output builtin features for bootloader support #1580
Conversation
odesenfans
requested review from
igaray,
Oppen,
fmoletta,
entropidelic,
juanbono and
pefontana
as code owners
vm/src/vm/runners/cairo_pie.rs
Outdated
| @@ -41,6 +41,8 @@ pub type Pages = HashMap<usize, PublicMemoryPage>; | |||
|
|
|||
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] | |||
| pub struct OutputBuiltinAdditionalData { | |||
| #[serde(skip)] | |||
| pub base: usize, | |||
There was a problem hiding this comment.
Why is this field needed?
There was a problem hiding this comment.
The problem is the following: the bootloader starts by saving the VM output builtin and replaces it by a dedicated one to run the "simple" bootloader program and capture its output.
To resolve that problem, I thought about adding this field here so that we can get the full state of the output builtin in one go. We don't want to serialize it, hence the skip. But I'm open to other proposals, I could create a dedicated struct for example.
There was a problem hiding this comment.
Why does the new output builtin need to know the base of the old one?
There was a problem hiding this comment.
We just store it so that we can restore it later on.
There was a problem hiding this comment.
The output builtin's segment index can already be found on the CairoPie itself on the CairoPie.metdata.builtin_segments map. I don't see the need to add it in the BuiltinAdditionalData too
There was a problem hiding this comment.
Oh, in that case the output builtin is available and you could call the base method to obtain the output's base right?
You can then choose to store it either as a tuple or a separate struct in the bootloader code itself
There was a problem hiding this comment.
I think there is a mixup in what the output builtin's state is (base, attributes & pages) and what the additional data is (attributes & pages). The python vm has its own get_state method that has the behaviour you are adding to get_additional_data
There was a problem hiding this comment.
I can definitely do that, but then the methods to retrieve/restore the output builtin state become more specific. I can either:
- Replace the whole builtin object
- Add a method that takes a base + additional data to set the builtin state
- Add a new struct (ex:
OutputBuiltinState) and add a getter + setter for this particular struct.
What do you prefer?
There was a problem hiding this comment.
2 & 3 are both good solutions, 2 sound simpler so I would go for that one
There was a problem hiding this comment.
I went with solution 3 and introduced an OutputBuiltinState struct.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
+ Coverage 97.63% 97.65% +0.01%
==========================================
Files 92 92
Lines 37464 37619 +155
==========================================
+ Hits 36579 36735 +156
+ Misses 885 884 -1 ☔ View full report in Codecov by Sentry. |
odesenfans
force-pushed
the
msl/output-builtin-get-set-state
branch
from
a3b078e to
a9b8545
Compare
|
@odesenfans can you solve merge conflicts so we can merge it |
odesenfans
force-pushed
the
msl/output-builtin-get-set-state
branch
from
9f3efea to
b20dce7
Compare
odesenfans
force-pushed
the
msl/output-builtin-get-set-state
branch
from
b20dce7 to
b3a1851
Compare
|
FYI the CI/CD fails when publishing the benchmarks to Github, no idea why. |
odesenfans
force-pushed
the
msl/output-builtin-get-set-state
branch
from
b3a1851 to
73167b2
Compare
There was a problem hiding this comment.
@odesenfans looks like some tests are failing. Can you take a look?
This commit introduces the following features/changes: * Paging: pages can now be added to the output builtin. These pages are reflected in the public memory of the VM when exporting the public input. * The state of the output builtin can now be modified using the new `set_state` method. * The output builtin can now handle attributes. These are used to generate the fact topologies of the bootloader.
The `get_state` and `set_state` methods now rely on the new `OutputBuiltinState` struct. Rolled back the introduction of the base field in `OutputBuiltinAdditionalData`.
odesenfans
force-pushed
the
msl/output-builtin-get-set-state
branch
from
73167b2 to
48f86ff
Compare
This commit introduces the following features/changes:
set_statemethod.Checklist