Skip to content
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

fix(async-flow): endowment equate bug #9722

Closed

Conversation

erights
Copy link
Member

@erights erights commented Jul 17, 2024

#endo-branch: master

Staged on #9719

closes: #XXXX
refs: #XXXX

Description

The failure at https://github.com/Agoric/agoric-sdk/pull/9719/files#diff-cbd091458bbd51ddc6640a3783cdb340d7f2588cb7ff27b330ba228cfbd0e18dR64-R87 revealed a bug introduced to async-flow when adding support for endowments. Because of the so-called "unwrapping" of some guests, there can be two guests corresponding to one host, with the host of course only mapping back to one of them -- the outer one. This makes bijection.js more complicated and irregular than an actual bijection.

equate(g, h) had a test for early return, if the g and h were already "equated", i.e., were corresponding guest and host. But the equate test was written before the elaboration of bijection. In fact, it should only test whether this guest g maps to the host h, irrespective of whether h maps back to this g.

Unfortunately:

This failure was not tested by any async-flow test before it failed for a user of async-flow.

  • This PR must add such a test before it becomes a candidate for merging

This PR alters the failing test case https://github.com/Agoric/agoric-sdk/pull/9719/files#diff-cbd091458bbd51ddc6640a3783cdb340d7f2588cb7ff27b330ba228cfbd0e18dR64-R87 to see if this fix to async-flow fixes that test case. Unfortunately, it does not. However, it now fails with different symptoms. Someone who understands #9719 better than I should investigate the nature of these new symptoms. If it still looks like it might be an async-flow problem, let me know!

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@erights erights changed the base branch from master to 9303-orchestrate-upgrade July 17, 2024 03:47
Copy link

cloudflare-workers-and-pages bot commented Jul 17, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 883c0bb
Status: ✅  Deploy successful!
Preview URL: https://76050423.agoric-sdk.pages.dev
Branch Preview URL: https://markm-fix-async-flow-endowme.agoric-sdk.pages.dev

View logs

@erights erights self-assigned this Jul 17, 2024
@erights
Copy link
Member Author

erights commented Jul 17, 2024

At https://github.com/Agoric/agoric-sdk/actions/runs/9967848861/job/27542195386?pr=9722#step:5:1588 I see the following. @Chris-Hibbert , does that seem like the same kind of mysterious problem we were looking at earlier today?

RemoteError(error:liveSlots:v9#70001)#4 ERROR_NOTE: Sent as error:liveSlots:v1#70001
Logging sent error stack (RemoteError(error:liveSlots:v9#70001)#4)
not ok 8 - orchestration › contract-upgrade › resume %ava-dur=51124ms
#   start sendAnywhere
#   making offer
#   upgrade sendAnywhere with fix
#   REJECTED from ava test("resume"): (RemoteError(error:liveSlots:v1#70001)#5)
#   RemoteError(error:liveSlots:v1#70001)#5: vat-upgrade failure
#     at kunser (packages/kmarshal/src/kmarshal.js:102:51)
#     at queueAndRun (packages/SwingSet/tools/run-utils.js:40:15)
#     at async evalProposal (packages/boot/tools/supports.ts:1:9163)
#     at async packages/boot/test/orchestration/contract-upgrade.test.ts:1:996

  ---
    name: AssertionError
    message: Rejected promise returned by test
    values:
      'Rejected promise returned by test. Reason:': |-
        Error {
          message: 'vat-upgrade failure',
        }
    at: |-
      kunser (packages/kmarshal/src/kmarshal.js:102:51)
      queueAndRun (packages/SwingSet/tools/run-utils.js:40:15)
      async evalProposal (packages/boot/tools/supports.ts:1:9163)
      async packages/boot/test/orchestration/contract-upgrade.test.ts:1:996
  ...

@erights erights changed the title Markm fix async flow endowment bug fix(async-flow): endowment equate bug Jul 17, 2024
@erights
Copy link
Member Author

erights commented Jul 17, 2024

Argh! I see the bad old typescript stack traces with line-number 1. Now that endojs/endo#2355 is merged and endojs/endo#2359 is in review, either we'll get a new endo-release-agoric-sdk-sync soon, or I'll complete #9711 so we have proper typescript line-numbers.

@erights erights force-pushed the markm-fix-async-flow-endowment-bug branch from 0d34a14 to 883c0bb Compare July 18, 2024 02:14
@erights
Copy link
Member Author

erights commented Jul 18, 2024

As an experiment, I added

sharp-endo-branch: master

to the top of the PR comment and provoked CI to run again. The bad news is I'm getting different problems, which I have yet to diagnose but seem unrelated to the equate bug.

The good news is that these CI errors now show proper TypeScript line-numbers!

#       at runPackageScript (/home/runner/work/agoric-sdk/agoric-sdk/packages/boot/tools/supports.ts:124:25)
#       at buildAndExtract (/home/runner/work/agoric-sdk/agoric-sdk/packages/boot/tools/supports.ts:163:7)
#       at evalProposal (/home/runner/work/agoric-sdk/agoric-sdk/packages/boot/tools/supports.ts:492:29)
#       at <anonymous> (/home/runner/work/agoric-sdk/agoric-sdk/packages/boot/test/orchestration/contract-upgrade.test.ts:26:3)

@erights
Copy link
Member Author

erights commented Jul 18, 2024

At least some of these failures may be the same as #7937 , explaining them as not having anything to do with the contents of this PR.

@Chris-Hibbert
Copy link
Contributor

does that seem like the same kind of mysterious problem we were looking at earlier today?

Oh yeah, absolutely! I've ruled out a number of possible causes of the issue I'm tracking, but haven't reached resolution yet.

@erights
Copy link
Member Author

erights commented Jul 22, 2024

Closing in favor of #9736

@erights erights closed this Jul 22, 2024
mergify bot added a commit that referenced this pull request Oct 4, 2024
closes: #9830
refs: #9722 #9719

## Description

#9719 originally failed on upgrade replay for an endowment. It revealed a bug introduced to async-flow when adding support for endowments. Because of the so-called "unwrapping" of some guests, there can be two guests corresponding to one host, with the host of course only mapping back to one of them -- the outer one. This makes `bijection.js` more complicated and irregular than an actual bijection.

`equate(g, h)` had a test for early return, if the `g` and `h` were already "equated", i.e., were corresponding guest and host. But the equate test was written before the elaboration of bijection. In fact, it should only test whether this guest `g` maps to the host `h`, irrespective of whether `h` maps back to this `g`.

Additional testing revealed that the unwrapped function was also not passable, and would fail to be passed back as an argument.

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
This change potentially makes the diagnostic error when misusing async-flow slightly less precise.

### Testing Considerations
Introduces equate checks in the endowments test exercising the bijection. For endowments with are further "unwrapped", we test both the original guest (which was previously failing) and the unwrapped one (which also was, but for a different reason).

Since #9719 landed with a failing test, this PR also sets that test as passing, effectively working as an integration test of functions as endowments.

### Upgrade Considerations
Can be deployed as a new version of the async-flow NPM package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants