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

vat-provisionpool will not re-subscribe to vat-bank getCurrentAmountNotifier #8722

Closed
warner opened this issue Jan 8, 2024 · 0 comments · Fixed by #9481
Closed

vat-provisionpool will not re-subscribe to vat-bank getCurrentAmountNotifier #8722

warner opened this issue Jan 8, 2024 · 0 comments · Fixed by #9481
Assignees
Labels
bug Something isn't working contract-upgrade

Comments

@warner
Copy link
Member

warner commented Jan 8, 2024

As part of #8499 (comment) (look for v14-bank-v28-provisionpool), we identified a likely problem in the ProvisionPool vat. It has about a dozen promises that are subscribed to v14-bank, which will probably not re-subscribe when v14-bank is restarted/upgraded. As such, it represents an upgrade blocker: we must deploy a fix to v28-provisionpool before we can safely upgrade v14-bank.

v28-provisionpool uses observeNotifier to subscribe to a notifier obtained with E(exchangePurse).getCurrentAmountNotifier(). The use of observeNotifier is great, it knows how to react to vat upgrades, however the notifier it is pulling from (created by ERTP) is not durable. So observeNotifier will see an unrecoverable error, and will stop following.

We need the provisionpool code to react to this by calling getCurrentAmountNotifier() again (perhaps the fail: clause can inspect the reason and, if it is a new disconnection, try again).

Upgrade Considerations

v28-provisionpool does not use durable followers, so if/when v28 is upgraded, all of the observeNotifier instances will go away. This presents a problem when upgrading v28. We need the provisionpool code to somehow re-acquire all the asset subscriptions at upgrade time. This needs to be present in the next deployment of v28, to avoid losing track of the amounts that it is supposed to be watching. This yields the following sequence of upgrade dependencies:

  • fix provisionpool to re-establish its followers upon vat-provisionpool upgrade
  • deploy this fix to v28
  • fix provisionpool to re-establish followers upon vat-bank upgrade
  • deploy this fix to v28
  • now it is safe to upgrade/restart v14-bank

The first two v28 deployments can be merged.

@warner warner added bug Something isn't working contract-upgrade labels Jan 8, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Jun 3, 2024
@mergify mergify bot closed this as completed in #9481 Jun 12, 2024
mergify bot added a commit that referenced this issue Jun 12, 2024
fixes: #8722
fixes: #8724
fixes: #8238

## Description

Upgrade provisionPool so its notifiers are robust to upgrades. 

### Security Considerations

This is about soundness of the chain and its services.

### Scaling Considerations

N/A

### Documentation Considerations

No change to functionality.

### Testing Considerations

Includes a test in `a3p-integration` that demonstrates that
provisionPool will survive its own or vat-bank's upgrade.

### Upgrade Considerations

See above.
mergify bot added a commit that referenced this issue Dec 3, 2024
…grade (#10419)

closes: #10395
closes: #10425 
closes: #10564



## Description

As mentioned in #8158, we need the ability to restart or replace all vats. This PR focuses on the vats v28-provisionPool and v14-bank. The goal is to make sure after upgrading both of those vats, v28-provisionPool functionality keeps working. We pay special attention to #8722 and #8724 during the tests as those issues were identified as potential problems against v28-provisionPool upgrade.

### Security Considerations

v28-provisionPool and v14-bank are critical vats for the system when it comes to onboarding new users and keeping track of ERTP representations of user assets. Reviewers, please highlight the slightest risk you see.

### Scaling Considerations

v28-provisionPool vat has a linear relationship with the number of users entering the Agoric system. So it is pretty important it keeps working. Reviewers, please highlight the slightest risk you see.

### Documentation Considerations

None.

### Testing Considerations

During the testing, we focused on `provisionPoolAddress`' cosmos level balances as our source of truth. The reasoning behind this is; if the IST balance of the `provisionPoolAddress` it can only mean
- it has received the anchor asset we've sent 
- v28-provisionPool is notified of this balance change
- executed a swap against corresponding PSM
- deposited the payout to IST purse
- v14-bank updated the balances correctly

In our test we send two anchor assets;
* USDC_axl to make sure v28-provisionPool recovered existing purses
* USD_LEMONS to make sure v28-provisionPool is notified of new assets

### Upgrade Considerations

Both v28-provisionPool and v14-bank are staged upgrades in `upgrade.go`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working contract-upgrade
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants