upgrade scaledPriceAuthorities #9382
Conversation
Chris-Hibbert
added
ERTP
packages/ERTP/src/issuerKit.js
Outdated
| // if the existing state was 'hasRecoverySets' and recoverySetsOption is | ||
| // 'noRecoverySets', then we'll leave the old recoverySet in place, but not | ||
| // add to it. |
There was a problem hiding this comment.
I had some difficulty making sense of this. It's not really about the option matrix, because oldRecoverySetsState is ignored if recoverySetsOption == noRecoverySets`.
| // if the existing state was 'hasRecoverySets' and recoverySetsOption is | |
| // 'noRecoverySets', then we'll leave the old recoverySet in place, but not | |
| // add to it. | |
| // Extant sets are never deleted. If the new option is 'noRecoverySets', they won't | |
| // be used but extant ones will remain. |
There was a problem hiding this comment.
"Never" is too strong. The expectation is that we will find a way to delete the old objects in a controlled fashion, but we have to preserve the membership in the set in order to do so.
There was a problem hiding this comment.
Just for the record, #8498 did such incremental deletion. Its deletion logic could be revived. If it is, the 'noRecoverySets' option should be adequate to engage it, rather than requiring yet another options setting.
There was a problem hiding this comment.
When I last discussed this with @warner, he pointed out that those deletions and their consequences that cascade through the kernel to other vats are gated by GC passes, and bring-out-your-dead phases. The effects of that buffering is that a deletions every so often in Zoe or some other vat might be saved up by the kernel and passed en masse to some other vat which might be overwhelmed. Until there's better support for regulating the flow of such updates between the kernel and downstream vats, he prefers that we not do mass deletions even at a slow rate.
| @@ -1,7 +1,24 @@ | |||
| # Proposals | |||
|
|
|||
| These are code snippets that go into propoals to the BLDer DAO to start the Inter Protocol. | |||
| These scripts are referenced by proposals to the BLDer DAO to run on the chain. | |||
There was a problem hiding this comment.
This documentation is good. It strikes me as not specific to inter-protocol.
Consider putting it in deploy-script-support and referencing that here.
Deploying agoric-sdk with
|
| Latest commit: |
04ba2b5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff4bbe76.agoric-sdk.pages.dev |
| Branch Preview URL: | https://8400-upgradescaledpas.agoric-sdk.pages.dev |
|
@erights, when you return, I'd appreciate a quick glance to approve the ERTP change. |
There was a problem hiding this comment.
@erights, when you return, I'd appreciate a quick glance to approve the ERTP change.
Thanks for checking.
ERTP changes LGTM. I did not look at the rest, so my approval is only scoped to that. But I see you already have a general approval, so you're clear to merge.
Chris-Hibbert
added
the
automerge:rebase
Upgraded issuers would keep the recoverySet without removing anything.
Chris-Hibbert
force-pushed
the
8400-upgradeScaledPAs
branch
from
f9e3b65 to
04ba2b5
Compare
refs: #9382 refs: #9584 ## Description Add a test that was supposed to be in #9283, where it says > A3P tests that verify that vaultFactory has been upgraded, that a new Auctioneer is running and is receiving prices. Verify that when prices drop, assets are sold via the auction, the bidder gets the proceeds, and the vaults are liquidated or reconstituted appropriately. It was too hard to verify the results of the auction, because of the timing of vault liquidations and auction runs, so the actual check was dropped. The subsequent PR (#9371) that upgraded scaledPriceAuthorities seems to have broken the upgrade, and the missing test failed to warn us. Here we test that vaultFactory was actually upgraded by verifying that it's getting prices from the new price feeds, and drop the upgrade of scaledPriceAuthority until we can figure out how to make that upgrade compatible. ### Security Considerations Not relevant ### Scaling Considerations Drops the upgrade of scaledPriceAuthority, which fixed part of the memory growth. This was the smaller portion of the growth, so it's more important to get the rest of the fixes in than to also include this. ### Documentation Considerations None. ### Testing Considerations Replaces a missing test. ### Upgrade Considerations Repairs upgrade.
refs: #9382 refs: #9584 ## Description Add a test that was supposed to be in #9283, where it says > A3P tests that verify that vaultFactory has been upgraded, that a new Auctioneer is running and is receiving prices. Verify that when prices drop, assets are sold via the auction, the bidder gets the proceeds, and the vaults are liquidated or reconstituted appropriately. It was too hard to verify the results of the auction, because of the timing of vault liquidations and auction runs, so the actual check was dropped. The subsequent PR (#9371) that upgraded scaledPriceAuthorities seems to have broken the upgrade, and the missing test failed to warn us. Here we test that vaultFactory was actually upgraded by verifying that it's getting prices from the new price feeds, and drop the upgrade of scaledPriceAuthority until we can figure out how to make that upgrade compatible. ### Security Considerations Not relevant ### Scaling Considerations Drops the upgrade of scaledPriceAuthority, which fixed part of the memory growth. This was the smaller portion of the growth, so it's more important to get the rest of the fixes in than to also include this. ### Documentation Considerations None. ### Testing Considerations Replaces a missing test. ### Upgrade Considerations Repairs upgrade.
closes: #9584 closes: #9928 refs: #9827 refs: #9748 refs: #9382 closes: #10031 ## Description We added upgrading the scaledPriceAuthority to the steps in upgrading vaults, auctions, and priceFeeds, and didn't notice that it broke things. The problem turned out to be that the "priceAuthority" object registered with the priceFeedRegistry was an ephemeral object that was not upgraded. This fixes that by re-registering the new priceAuthority. Then, to simplify the process of cleaning up the uncollected cycles reported in #9483, we switched to replacing the scaledPriceAuthorities rather than upgrading them. We also realized that we would need different coreEvals in different environments, since the Oracle addresses and particular assets vary for each (test and mainNet) chain environment. #9748 addressed some of the issues in the original coreEval. #9999 showed what was needed for upgrading priceFeeds, which was completed by #9827. #10021 added some details on replacing scaledPriceAuthorities. ### Security Considerations N/A ### Scaling Considerations Addresses one of our biggest scaling issues. ### Documentation Considerations N/A ### Testing Considerations Thorough testing in a3p, and our testnets. #9886 discusses some testing to ensure Oracles will work with the upgrade. ### Upgrade Considerations See above
refs: #8400
refs: #8498
closes: #9371
Description
#8400 reported that the priceFeed vats hold onto old quotes in their recoverySet, preventing them from being collected. #9283 applied the fixes to master. These fixes address the growth in priceFeed vats and Zoe, but scaledPriceAuthorities were still growing. This resolves that problem by upgrading scaledPriceAuthority contracts to not use their recoverySets.
Expand for performance graphs
Kernel allocation is in blue, and the scale is on the left. It varies from 48862 to 49743, with a small amount of long-term growth.The other active vats (v9=Zoe, v29=ATOM-USD_priceFeed, v43=wallet, 72=ATOM-USD_priceFeed, 74=auctioneer) use the scale on the right, with Zoe varying tightly around 3600, and the others low and stable.
scaledPriceAuthority-ATOM doesn't have enough variation to be worth graphing.
Security Considerations
Upgrade existing contracts. No new vulnerabilities.
Scaling Considerations
This addresses the largest known category of growth on the chain.
Documentation Considerations
Add some documentation on creating proposals.
Testing Considerations
Tested in A3P. We should exercise all the clients of priceFeeds in our test environments.
Upgrade Considerations
This PR includes a proposal that will upgrade all vats with
scaledPriceAuthorityin their label. That should work on or test chains as well as MainNet. These changes should be included in the next release.