test(liveslots): relaxDurability should ignore promises #9539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with
|
| Latest commit: |
5c39d56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e762a69.agoric-sdk.pages.dev |
| Branch Preview URL: | https://markm-reform-relaxdurability.agoric-sdk.pages.dev |
erights
force-pushed
the
markm-reform-relaxDurabilityRules
branch
4 times, most recently
from
76c5375
to
1d0e95f
Compare
warner
reviewed
Jun 21, 2024
warner
reviewed
Jun 21, 2024
erights
added a commit
that referenced
this pull request
Jun 21, 2024
erights
commented
Jun 21, 2024
This was referenced Jun 27, 2024
warner
force-pushed
the
markm-reform-relaxDurabilityRules
branch
from
8a38fc2
to
5c7a7fb
Compare
mergify bot
pushed a commit
that referenced
this pull request
Jun 28, 2024
closes: #XXXX refs: https://github.com/Agoric/agoric-sdk/pull/9539/files#r1648285867 ## Description As @warner suggests at #9539 (comment) , I'm moving that change to this separate PR so we can decide separately when to merge it. It should be a pure refactor, since nothing should have been counting on the absence of the `harden` ### Security Considerations `harden`ing early is better for integrity, and will catch some integrity-violating bugs (property mutations) earlier. Almost certainly no difference in this case though, but good precedent for reenforce best practices. In fact, within the SwingSet kernel, this cannot have any effect on production under current configurations, where `harden` is turned off for SwingSet anyway. But at least we still have the option of turning `harden` on when testing, in which case we still get the bug finding benefit. Finally, it is possible we will someday find we can afford to turn `harden` back on for SwingSet as a whole, in which case we get back this integrity protection for real. ***PLEASE establish the habit of `harden`ing literals before they escape whenever possible!*** ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations `harden` in SwingSet could be turned on during testing, in which case these `harden` calls with detect more bugs. ### Upgrade Considerations Why we pulled this out into a separate PR. See https://github.com/Agoric/agoric-sdk/pull/9539/files#r1648285867
turadg
force-pushed
the
markm-reform-relaxDurabilityRules
branch
from
5c7a7fb
to
959aaf7
Compare
We have "fake vat" testing environments which configure their copy of liveslots with a setting named `relaxDurabilityRules = true`. This changes the rules for durable objects, which normally refuse to accept non-durable state. When the rules are relaxed, they accept ephemeral and merely-virtual objects as well. Promises were never supposed to be accepted, even when the rules are relaxed. But liveslots had a bug, and accepted Promises in the relaxed mode. This PR changes liveslots to correctly reject them. Lacking this enforcement, some other packages had mistakenly stored Promises in durable objects, and gotten away with it. Now that liveslots is fixed, their tests would fail. So this PR also fixes those packages. One case is the smart-wallet, whose tests which use contexts.js will get a Zoe that uses fully-resolved `agoricNames` and `board`. But some tests are deliberately not using that context, to exercise what happens when E(zoe).startInstance() is given bad arguments. This changes the tests to use bad *resolved* arguments, as to Promises for bad arguments, because the latter now fails in a different way. Giving Zoe a promise in `terms` causes "value for instanceRecord is not durable", which is not what this test is trying to exercise. In addition, the expected error message was updated, because it now mentions things like "NameHubKit" instead of "Promise" in the unrelated (good) arguments. The provisionPool test was fixed with an unsatisfying `await null` stall. See #9598 for notes and plans for a better fix.
warner
force-pushed
the
markm-reform-relaxDurabilityRules
branch
from
959aaf7
to
5c39d56
Compare
warner
approved these changes
Jul 10, 2024
turadg
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes: #9599
Description
Just an experiment to see what breaks under CI if we make promises non-durable even if
relaxDurabilityRulesis true.Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations
Note that this PR changes liveslots (specifically
src/virtualReferences.js), which triggers concerns about how pre-existing/deployed vats might (or might not) receive these changes. In this case, the liveslots change does not affect the behavior of real vats: it only behaves differently ifrelaxDurabilityRules = true, and we don't provide any way to enable that in a real kernel.This will change the contents of the liveslots bundle, which will be used by new vats, or by vats which are upgraded for any reason (once this code is deployed to the chain). But this PR should not cause any concerns about test behavior diverging between trunk and mainnet.