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

feat(async-flow): error on guest E use #9443

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

erights
Copy link
Member

@erights erights commented Jun 2, 2024

closes: #XXXX
refs: #9299 #9322

Description

Prepare for #9322 by making any guest use of E until then cause an error. We expect that it might be a while before #9322 is ready for review. By merging this PR soon, we prevent any guest code or logs that would commit us to an incompat way of handling E.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

Should document that guests cannot invoke host vows or remotables with E until #9322 , which won't happen immediately.

Testing Considerations

  • need to test what kind of error state this goes into. Should be a panic, so that an upgrade can unblock guest execution that got stuck trying to E.

Upgrade Considerations

The point. By making such use of E an error now, we ensure that #9322 can proceed without causing any compat problem with committed durable state.

Copy link

cloudflare-workers-and-pages bot commented Jun 2, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7416d5d
Status: ✅  Deploy successful!
Preview URL: https://12794b01.agoric-sdk.pages.dev
Branch Preview URL: https://markm-asyncflow-no-e.agoric-sdk.pages.dev

View logs

@erights erights mentioned this pull request Jun 2, 2024
3 tasks
@erights erights marked this pull request as ready for review June 2, 2024 20:31
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @michaelfig review the HandledPromise logic in the makeGuest* functions, my brain always hurt thinking of the handler plumbing.

  • need to test what kind of error state this goes into. Should be a panic, so that an upgrade can unblock guest execution that got stuck trying to E.

I just had a pesky thought about the following:

const payment= E(purse).withdraw(amount);
foo.receive(payment); // note not using `E` here. `foo` is a guest wrapper of a host local exo

at the time receive is called, payment is a basic HandledPromise unknown to the membrane because the promise handler is always called in a future turn (but always same crank). I honestly do now know how to solve this except providing our own E with our own HandledPromise implementation, or somehow enforce that the payment promise argument becomes know by the next turn, but I'm really warry of going down turn counting routes.

packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
erights added a commit that referenced this pull request Jun 4, 2024
@erights
Copy link
Member Author

erights commented Jun 4, 2024

I'll let @michaelfig review the HandledPromise logic in the makeGuest* functions, my brain always hurt thinking of the handler plumbing.

  • need to test what kind of error state this goes into. Should be a panic, so that an upgrade can unblock guest execution that got stuck trying to E.

I just had a pesky thought about the following:

const payment= E(purse).withdraw(amount);
foo.receive(payment); // note not using `E` here. `foo` is a guest wrapper of a host local exo

at the time receive is called, payment is a basic HandledPromise unknown to the membrane because the promise handler is always called in a future turn (but always same crank). I honestly do now know how to solve this except providing our own E with our own HandledPromise implementation, or somehow enforce that the payment promise argument becomes know by the next turn, but I'm really warry of going down turn counting routes.

I don't understand the problem. You didn't say what purse is, so let's break it down into two cases

  • purse is a guest thing, so the E(purse).withdraw(amount) is completely within the guest side. In this case, payment is a guest-created guest promise. Then, on the next line when payment is used as an argument in a host call, it fails for reasons already tested before this PR: The guest cannot pass guest-created guest promises as arguments in host calls.
  • purse, like foo, is guest wrapper of a host remotable. In that case it fails on this line because of exactly the use of E prohibited by this PR.

My guest is you mean the first. But either way, AFAICT, it should work. What am I missing?

@erights
Copy link
Member Author

erights commented Jun 4, 2024

I asked

What am I missing?

offline @mhofman explained

basically the problem is that even if purse is a guest wrapper, by the time we see payment used as an argument, the purse "pending / presence handler" has not yet been invoked (delayed by a turn always by eventual send / HandledPromise), so we don't know that payment is a handled promise derived from the membrane.

to which I responded

Right. We’d need to register it during the eventual send itself, which requires some kind of handler to run synchronously, which conflicts with our protection against reentrancy.

Just to separate issues, this threatens #9322 , but not #9443 , right? The latter (#9443) can still proceed?

@erights erights requested a review from mhofman June 4, 2024 04:20
@erights erights added the asyncFlow related to membrane-based replay and upgrade of async functions label Jun 4, 2024
@erights
Copy link
Member Author

erights commented Jun 4, 2024

t.throws(() => badGuest.badMethod(guestCreatedPromise), {
message:
'In a Failed state: see getFailures() or getOptFatalProblem() for more information',
});

verifies that passing a guest-created promise as argument of a host call is a panic. With that settled, this PR is Ready for re-review

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional approval assuming the suggested test change is not controversial. I would also still feel more comfortable if @michaelfig could double check that we're setting up eventual-send handlers the right way on guest objects.

packages/async-flow/src/replay-membrane.js Outdated Show resolved Hide resolved
packages/async-flow/src/replay-membrane.js Show resolved Hide resolved
packages/async-flow/test/replay-membrane-eventual.test.js Outdated Show resolved Hide resolved
Co-authored-by: Mathieu Hofman <86499+mhofman@users.noreply.github.com>
@erights erights added the automerge:squash Automatically squash merge label Jun 10, 2024
@mergify mergify bot merged commit e193e66 into master Jun 10, 2024
71 checks passed
@mergify mergify bot deleted the markm-asyncFlow-no-E branch June 10, 2024 17:03
mergify bot pushed a commit that referenced this pull request Jun 28, 2024
closes: #XXXX
refs: #9322, #9299 #9443

## Description

PR #9322 is supposed to provide production quality support for asyncFlow guest functions to use `E`. It is being reviewed for that goal, and will not be merged until we think it meets that bar. However, we need to start integration testing of asyncFlow with orchestration, to spot mismatched assumptions we may have missed. For this purpose, we do not immediately need production quality `E` support. That is the purpose of this PR. It starts as a copy of the code from #9322 but need only be evaluated as adequate for these stopgap purposes before being merged. This PR does *NOT* claim to f-i-x #9299 , leaving that job to remain with #9322 

Even though the requirements on this PR are so much lighter, reviewers should still look at the unresolved conversations on #9322 and determine if any of those need to first be solved even in this PR.

### Security Considerations

When merging stopgap code to master, there is always the danger that it might be used as if it production code. We need to remember not to do so, instead waiting for #9322 to do the job for real.

### Scaling Considerations
none
### Documentation Considerations
just as this stopgap unblocks integration testing, it also likely unblocks documenting how to use asyncFlow, both in general and for orchestration.
### Testing Considerations
As a stopgap, this PR does not need the rigorous testing that #9322 should have.
### Upgrade Considerations
We need to not use this stopgap for production purposes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants