fix(ses): normalize `bestEffortsStringify` property order by erights · Pull Request #1678 · endojs/endo

erights · 2023-07-17T08:07:38Z

Currently bestEffortsStringify uses the default property enumeration order, which is insertion order. As a result, semantically neutral changes that happen to change property insertion order accidentally cause error messages to change, breaking golden tests of error messages.

See example breakage at https://github.com/Agoric/agoric-sdk/actions/runs/5572658660/jobs/10178998419?pr=5422#step:5:212

With expository whitespace:

({
  "want": {
    "TokenJ": {
      "brand": "[Alleged: moola brand]",
      "value": "[3n]"
    }
  },
  "give": {
    "TokenK": {
      "brand": "[Alleged: bucks brand]",
      "value": "[5n]"
    }
  },
  "multiples": "[1n]",
  "exit": {"onDemand": null}
})

vs

({
  "exit": {"onDemand": null},
  "give": {
    "TokenK": {
      "brand": "[Alleged: bucks brand]",
      "value": "[5n]"
    }
  },
  "multiples": "[1n]",
  "want": {
    "TokenJ": {
      "brand": "[Alleged: moola brand]",
      "value": "[3n]"
    }
  }
})

Amusingly, at least in this case, the golden is in property-name sorted order, so this change will not break that one golden. However, this PR may very well break other golden error tests; but only those that were inherently fragile anyway.

dckc · 2023-07-17T15:14:12Z

As noted, this is a breaking change. In agoric-sdk, we document those using conventional commits; that is: either change the commit message headline to fix(ses)!: ... or add a BREAKING CHANGE: footer.

What's the convention in endo? Documenting it in CONTRIBUTING.md would be handy.

dckc · 2023-07-17T15:27:43Z

Have we carefully considered the costs and benefits of a breaking change in the marshal and patterns packages? The packages aren't at 1.x yet, but since they're deployed on chain, let's suppose that they were. This breaking change would bump the version to 2.x. What is the support policy for 1.x clients?

kriskowal · 2023-07-17T17:51:27Z

cc @mhofman regarding breaking change. What kind of breaking change?

I suspect that any breaking change at this layer gets integrated in the kernel and would break a full replay of the chain. I imagine there are exceptions. For example, if this change demonstrably does not cause a divergence in the material of observed messages in the entire history of the chain.

I’m pretty sure the time for breaking changes at this layer has come to an end and that all such changes going forward need to be framed as differently-named opt-in alternatives.

mhofman · 2023-07-17T18:21:54Z

cc @warner for upgrade and breaking changes.

kriskowal · 2023-07-17T22:30:49Z

I’ve reviewed the change and like the outcome. Withholding my stamp until we hear back from @mhofman and @warner whether this needs to be an opt-in behavior change to preserve continuity of Agoric chains.

If this would break continuity of Agoric chains, we would need to discuss ways we can make progress in ses without impacting the chain, like allowing for the ses major version to vary between vats, or cultivating an expectation of regular kernel upgrades / chain restarts.

mhofman · 2023-07-18T00:46:08Z

I'll defer to @warner for the authoritative answer, but my understanding is that the version of SES and endo loaded in vats, both by the supervisor & lockdown bundles, and by the vat bundle, are fixed at vat creation / upgrade time, and that changing any bundle requires a vat upgrade. As such, only changes that have an incompatibility with the usage of the API are breaking, and will require some kind of versioning support (which Swingset currently lacks). While a change in the property order of serialization is observable, I do not consider that part of the API, and as such is a "minor" semver change. Therefore, it is ok to include this change in agoric-sdk. The only effect is that vat started (and maybe upgraded?) after this change is deployed will use this new order. Other vats, including replaying ones, will continue to use the previous implementation.

packages/ses/src/error/stringify-utils.js

erights · 2023-07-25T20:06:31Z

@warner @kriskowal @gibson042 @mhofman @dckc , at the upcoming kernel meeting, I would like to use this PR as a test case to understand what should count as a breaking change, and what coordination we need for a PR like this to proceed.

erights · 2023-07-26T22:30:53Z

IIUC, @warner says this PR can proceed without needing an opt-in mechanism.

@warner , is that correct?

warner · 2023-07-26T22:34:28Z

Summary of the kernel meeting discussion:

this change is OK to land
- it's also OK to make a new SES/Endo release from this
- it's also OK to do another "Endo sync" to make agoric-sdk packages start using it
- these packages will behave slightly differently, but that's fine, we never expect two different bundles to behave the same way, and the chain stores its bundles specifically to handle that case
- the only behavior-divergence scenario this is likely to impact is an as-yet-unwritten "debugging follower" (where we extract a vat transcript from the chain, originally executed under XS, and then replay it under V8, perhaps with a newer SES/Endo version), and we know that such a follower needs to be tolerate of behavior differences anyways
this change is unlikely to be observed from userspace (but not impossible: error.toString() where the .value has properties added in non-alphabetic order), so it is unlikely to induce "what version of SES do I need" compatibility issues
- contrast with the recent addition of assert.bare, which more obviously induces a version dependency
- our package.json, bundling, and execution pathway does not have a good way for code to declare that it needs to be run in an environment where globalThis.assert has a .bare
- since this does not depend upon what version of @agoric/assert you import, since that package just delegates to globalThis.assert, which depends upon what version of @endo/init was used to create the environment
- the declaration is most similar in character to the engine: declaration, which allows packages to indicate what version of the Node.js runtime they require, which is a proxy for requirements about the behavior of the (unversioned) stdlib packages it imports, or the availability of new global properties (like Promise, once upon a time)

dckc

@kriskowal says:

I’ve reviewed the change and like the outcome.

and discussion with @warner addressed my upgrade concerns.

breaking change documentation

dckc · 2023-07-28T14:29:02Z

On 2nd thought, I defer to @kriskowal , because I don't clearly understand endo's breaking change policy / norms. Is this a breaking change? If so, should its git commit message conform to conventional commits syntax for such?

erights · 2023-07-29T18:55:01Z

On 2nd thought, I defer to @kriskowal , because I don't clearly understand endo's breaking change policy / norms. Is this a breaking change? If so, should its git commit message conform to conventional commits syntax for such?

@kriskowal , what say you? Should I change the commit message to fix(ses)!: ?

kriskowal · 2023-07-29T19:19:41Z

My intuition is that, apart from the chain, most consumers will not rely on consistent property order and this should not require dependees to make changes to compensate for the upgrade. So, this does not require any breaking change ceremony.

…

On Sat, Jul 29, 2023 at 11:55 AM Mark S. Miller ***@***.***> wrote: On 2nd thought, I defer to @kriskowal <https://github.com/kriskowal> , because I don't clearly understand endo's breaking change policy / norms. Is this a breaking change? If so, should its git commit message conform to conventional commits syntax for such? @kriskowal <https://github.com/kriskowal> , what say you? Should I change the commit message to fix(ses)!: ? — Reply to this email directly, view it on GitHub <#1678 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOXBXHPBZVBAK2GQ5ZRCLXSVMBDANCNFSM6AAAAAA2MSID4I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dckc · 2023-07-29T19:32:52Z

I defer to @kriskowal to approve still.

Perhaps I'll get comfortable witht that way of looking at breaking in due course, but I'm not there yet. I recall once when we changed a type in some JSDoc or .d.ts, we considered it not a breaking change because it had no runtime impact, but one of our customers pointed out that it broke their build, so inasmuch as it required maintenance work by them, it was a breaking change from their perspective. Likewise, this change caused tests to fail in agoric-sdk, which is tangible evidence that while consumers perhaps should not rely on consistent property order, at least one of them does.

erights · 2023-07-29T21:39:15Z

@kriskowal , how to proceed?

erights · 2023-07-29T23:52:32Z

In case it helps decide whether we add a ! or not, Agoric/agoric-sdk#8055 is the PR that repairs the test case that breaks on agoric-sdk after this change.

kriskowal

My thinking is that semantic versioning is currently irrelevant for Agoric SDK since our process for upgrades is to sync all packages regardless of version numbers. So, until or unless we move toward a model where Agoric SDK dependencies are versioned piecemeal, contracts have independent lockfiles, and the platform moves independently from contracts, Agoric SDK will really be stuck in the realm where the platform itself must never admit a breaking change. In this world, breaking CI is not equivalent to breaking production and the cost of upgrading golden masters must be bourn regardless.

For every other use of @endo/* and ses, I expect they’re already on the semantic version train and use a separate lockfile for every application. In this world, the spectrum of norms varies from Hyrum’s Law (and its corollary that all changes are breaking changes) all the way out to the other law (If a dependency in npm changes and nobody depends on it, it’s not a breaking change). Here in the practical middle, Apart from Agoric SDK,ses is the only package anyone relies on in production. For these users, the moral calculus hinges on what happens if someone runs yarn upgrade ses.

The risk of marking this as a minor/patch/non-breaking change is that a user (like Agoric SDK) depends on some behavior of ses that causes a CI failure. I tend to agree with @dckc that anything that causes someone else’s CI to fail after yarn upgrade is a breaking change.
The risk of marking this as a breaking-change is that the same user will run yarn upgrade and not receive a security fix that is available only if you expressly upgrade to @latest and sort out the breaking changes.

My feeling is that Agoric SDK’s golden master tests are too tightly constrained and that other users are unlikely (thought not impossible) to break in this way. I would like to err on the side of accidentally creating work for these people after a yarn upgrade rather than wait for them to adopt a major version to get a fix.

However, if we do elect to treat this as a breaking change, we will need to back-port security fixes to all prior major version trains.

So, I release @erights to land this change either way. I can bear the shared responsibility either way.

However, if we do mark this as a breaking change, please recall that the full incantation is:

! must be in the subject like fix(ses)!: normalize etc AND
*BREAKING CHANGE*: with an explanation must appear in the body of the commit message

Our tooling does not work unless you provide both indications! The later section will appear in the changelog entry that Lerna generates.

erights · 2023-07-31T23:00:39Z

So, I release @erights to land this change either way. I can bear the shared responsibility either way.

I elect not to add the !. Thanks.

erights requested review from gibson042 and kriskowal July 17, 2023 08:07

erights self-assigned this Jul 17, 2023

erights mentioned this pull request Jul 17, 2023

feat(zoe): multiples, for making divisible offers Agoric/agoric-sdk#5422

Draft

erights force-pushed the markm-normalize-q-print-order branch 3 times, most recently from 699e759 to 84f2ac7 Compare July 17, 2023 14:06

erights mentioned this pull request Jul 17, 2023

DO NOT MERGE: test #8126 against "current" endo master. Agoric/agoric-sdk#8055

Closed

erights force-pushed the markm-normalize-q-print-order branch 2 times, most recently from 02fdacf to 799de2e Compare July 25, 2023 19:47

erights commented Jul 25, 2023

View reviewed changes

packages/ses/src/error/stringify-utils.js Outdated Show resolved Hide resolved

erights requested review from dckc and warner July 26, 2023 22:28

warner removed their request for review July 27, 2023 01:10

erights force-pushed the markm-normalize-q-print-order branch from caef296 to 016e799 Compare July 27, 2023 01:18

dckc previously approved these changes Jul 28, 2023

View reviewed changes

erights force-pushed the markm-normalize-q-print-order branch from 016e799 to d937985 Compare July 31, 2023 22:09

kriskowal approved these changes Jul 31, 2023

View reviewed changes

fix(ses): normalize bestEffortsStringify property order

137daff

erights force-pushed the markm-normalize-q-print-order branch from d937985 to 137daff Compare July 31, 2023 22:59

erights enabled auto-merge July 31, 2023 23:01

erights merged commit 5465d45 into master Jul 31, 2023

erights deleted the markm-normalize-q-print-order branch July 31, 2023 23:06

erights mentioned this pull request Aug 1, 2023

fix: tolerate golden error property orders Agoric/agoric-sdk#8126

Merged

Conversation

erights commented Jul 17, 2023

Uh oh!

dckc commented Jul 17, 2023

Uh oh!

dckc commented Jul 17, 2023

Uh oh!

kriskowal commented Jul 17, 2023

Uh oh!

mhofman commented Jul 17, 2023

Uh oh!

kriskowal commented Jul 17, 2023

Uh oh!

mhofman commented Jul 18, 2023

Uh oh!

Uh oh!

erights commented Jul 25, 2023

Uh oh!

erights commented Jul 26, 2023

Uh oh!

warner commented Jul 26, 2023

Uh oh!

dckc left a comment

Choose a reason for hiding this comment

Uh oh!

dckc commented Jul 28, 2023

Uh oh!

erights commented Jul 29, 2023

Uh oh!

kriskowal commented Jul 29, 2023 via email

Uh oh!

dckc commented Jul 29, 2023

Uh oh!

erights commented Jul 29, 2023

Uh oh!

erights commented Jul 29, 2023

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

erights commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants