refactor: prepare for use of non-trapping integrity trait #10795
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
94cb9d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aef03699.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-prepare-for-non-trappi.agoric-sdk.pages.dev |
There was a problem hiding this comment.
This made me realize a broader upgrade hazard which I've described in endojs/endo#2675 (review)
There was a problem hiding this comment.
The change in this file points to a potential future upgrade hazard. It means we cannot simply use the currently deployed code and re-run its transcript on top of a lockdown + liveslots (or an XS engine) that introduces non-trapping behavior. Such a "replay" is one of the option we're still keeping under consideration, especially for the bootstrap vat which cannot be upgraded.
There was a problem hiding this comment.
Below, @michaelfig writes:
We may need to have some kind of opt-in for
packages/swingset-liveslots/src/liveslots.jschanges, or else replaying the bootstrap vat (our only upgrade mechanism that would work for it) may cause divergence, as @mhofman points out.I think that needs to be solved before we can land this change.
(repeating here to answer both together)
There was a problem hiding this comment.
Assuming this PR by itself is a pure refactor, as claimed, without any observable effect, why would this question need to be resolved before this PR is merged?
There was a problem hiding this comment.
I do understand that replay is especially sensitive. But is there some way that the changes in this file have an observable effect even on replay?
There was a problem hiding this comment.
Yes, in a "replay" of the bootstrap vat (such as on mainnet), and indeed any source bundle in use that is not rebuilt and upgraded, the proxy target hardening "refactor" changes in this PR and the earlier Endo PR will not apply, since the existing source bundle will be reused. The refactors will only apply to future chains (or vat/contract upgrades).
By "needs to be solved," I mean we should have a plan for upgradable vats (and contracts) to opt-in to the shim for their future upgrades so that non-opted-in vats/contracts will continue to execute in a non-shimmed environment.
Thinking about adopting the non-trapping shim without such an opt-in plan makes me nervous about asserting that there are no Upgrade Considerations.
There was a problem hiding this comment.
So, the changes have an observable effect (the tolerance of a stabilizing harden()), but only for new vats or existing vats' future upgrades. This PR alone does not prepare existing vats for a stabilizing harden() (whether native or shimmed).
Calling this out in the "Upgrade Considerations" section, and in the markdown file you wrote (I don't remember the PR, but it was in Endo) describing what needs to change to be ready for suppressTrapping would be useful.
There was a problem hiding this comment.
Calling this out in the "Upgrade Considerations" section, and in the markdown file you wrote (I don't remember the PR, but it was in Endo) describing what needs to change to be ready for suppressTrapping would be useful.
The markdown file in question is
https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
2 times, most recently
from
f43c9e5 to
4d00b72
Compare
There was a problem hiding this comment.
It seems to me that these changes are not sufficiently defensive against accidental hardening of a proxy that is intended to remain trapping. Would it make sense to add a (I'm handwaving here) suppressTrapping(x) { throw Error('This proxy cannot be passed through a marshaller because hardening it would suppress its trap handling...'); } trap handler to all the proxy handlers whose targets have been changed to be merely frozen instead of hardened?
My mistake, I see this documented as fixed by default in endojs/endo#2675 |
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
4d00b72 to
c851744
Compare
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
c851744 to
9c4cad6
Compare
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
9c4cad6 to
1220dd0
Compare
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
1220dd0 to
fc10f0d
Compare
|
As with endojs/endo#2679 (comment) Reviewers: I claim that this PR by itself prior to the introduction or use of the non-trapping or stabilize integrity traits is a pure refactor, i.e., it should not produce any observable changes. Please review with that claim in mind. If you do spot something that looks like an observable change, please let me know. Thanks! |
There was a problem hiding this comment.
We may need to have some kind of opt-in for packages/swingset-liveslots/src/liveslots.js changes, or else replaying the bootstrap vat (our only upgrade mechanism that would work for it) may cause divergence, as @mhofman points out.
I think that needs to be solved before we can land this change.
Closes: #XXXX Refs: Agoric/agoric-sdk#10795 ## Description Prepare for anticipated introduction and use of the non-trapping integrity trait as explained at https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md These preparations must work now, before these traits are introduced, and should continue to work after these traits are introduced and used. ### Security Considerations Some things that had been deeply frozen automatically by `harden` are now manually frozen by explicit calls to `freeze`. We need to review these carefully to ensure that nothing has inadvertently be left unfrozen as a result of the changes in this PR. Some proxies will become unhardenable, but they will still be hardenable as of now, so mistaken hardenings will not be detected. ### Scaling Considerations For this PR by itself, none. Using the shim-based implementation of the non-trapping trait will have scaling consequences: #2675 ### Documentation Considerations https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md will need to be reflected in developer docs. ### Testing Considerations Since this PR by itself should be a pure refactor with no observable changes, there is nothing to test at this stage. The testing burden will come with #2675 to see how adequate these preparations were. ### Compatibility Considerations The point. This changes to coding patterns that should be compat both with the current status quo and with #2675 ### Upgrade Considerations As a pure refactor, none.
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
efecd94 to
ad52363
Compare
erights
force-pushed
the
markm-prepare-for-non-trapping
branch
from
ad52363 to
94cb9d3
Compare
#endo-branch: master
closes: #XXXX
refs: endojs/endo#2679
Description
Does for agoric-sdk what endojs/endo#2679 does for endo
Prepare for anticipated introduction and use of the non-trapping integrity trait as explained at https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md
These preparations must work now, before these traits are introduced, and should continue to work after these traits are introduced and used.
Security Considerations
Some things that had been deeply frozen automatically by
hardenare now manually frozen by explicit calls tofreeze. We need to review these carefully to ensure that nothing has inadvertently be left unfrozen as a result of the changes in this PR.Some proxies will become unhardenable, but they will still be hardenable as of now, so mistaken hardenings will not be detected.
Scaling Considerations
For this PR by itself, none. Using the shim-based implementation of the non-trapping trait will have scaling consequences: endojs/endo#2675
Documentation Considerations
https://github.com/endojs/endo/blob/b12eb434b6672f0ceae41be55aac7f24c4562b7b/packages/ses/docs/preparing-for-stabilize.md will need to be reflected in developer docs.
Testing Considerations
Since this PR by itself should be a pure refactor with no observable changes, there is nothing to test at this stage. The testing burden will come with #10796 to see how adequate these preparations were.
Compatibility Considerations
The point. This changes to coding patterns that should be compat both with the current status quo and with #10796
Upgrade Considerations
As a pure refactor, none.