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

Don't reschedule vault's priceNotifier if one exists #10668

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

refs: #10660

Description

in testing upgrade 18 candidates, we realized that vaultManager was subject to the same error as auctions fixed in #10615.

Security Considerations

No Security implications.

Scaling Considerations

Not fixing this would add a new notifier to the vaultManager's quote watcher for every hour that elapsed between upgrade and the oracles starting to provide price updates.

Documentation Considerations

Unnecessary.

Testing Considerations

This was detected by trawling slogs. It's not clear that we can do better than that to verify the fix.

Upgrade Considerations

It would probably be a mistake to upgrade vaults without this fix.

@Chris-Hibbert Chris-Hibbert added performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks Vaults VaultFactor (née Treasury) force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release sdk-50-upgrade labels Dec 10, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Dec 10, 2024
@Chris-Hibbert Chris-Hibbert requested a review from a team as a code owner December 10, 2024 23:09
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

When we upgrade vat-vaultFactory, it will still old a reference to the scaledPriceAuthority's QuoteNotifier from the previous incarnation (because the kernel does not currently know how to safely drop non-durable imports upon upgrade), and that notifier will still send an update to the makeStoredNotifier() and watchQuoteNotifier() from the previous incarnation (because it doesn't know that these clients were ephemeral). But that update will fall upon deaf ears (dispatch.notify() will be ignored), and they won't get new getUpdateSince(). So we'll have a lingering object, but no ongoing activity. If/when vat-vaultFactory or the scaledPriceAuthority vat is deleted, that object will go away.

I'd love it if we could have unit tests on this, which would require mocking the priceAuthority and counting the number of times it receives makeQuoteNotifier() as we trigger it with getCollateralQuote() and friends (and have our mock return errors sometimes). I'll leave it up to you to decide whether that test is worth the effort or not.

@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Dec 11, 2024
@mergify mergify bot merged commit 34f026b into master Dec 11, 2024
115 checks passed
@mergify mergify bot deleted the 10660-vaultNotifiers branch December 11, 2024 00:55
mujahidkay added a commit that referenced this pull request Dec 13, 2024
### Description

Cherry-picks the following commits from master:
- #10672
(3b478fb)
- #10668
(a74161c)
- #10680 (c883c39,
1581127)
 
 No new upgrade name has been added. 
 
 Done partially via git cherry-pick and via the following rebase-todo:
 ```
# PR #10680 Branch
Record-instances-that-will-be-replaced-so-we-can-manage-them-10680-
label
base-Record-instances-that-will-be-replaced-so-we-can-manage-them-10680-
pick c883c39 feat: record instances that will be replaced so we can
manage them
pick 1581127 refactor: provideRetiredInstances
label
pr-10680--Record-instances-that-will-be-replaced-so-we-can-manage-them-10680-
reset
base-Record-instances-that-will-be-replaced-so-we-can-manage-them-10680-
merge -C d35659b
pr-10680--Record-instances-that-will-be-replaced-so-we-can-manage-them-10680-
# Record instances that will be replaced so we can manage them (#10680)
 ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks sdk-50-upgrade Vaults VaultFactor (née Treasury)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants