upgrade scaledPriceAuthorities; measure impact #9371
Assignees
Labels
contract-upgrade
enhancement
New feature or request
next-release
about next agoric-sdk or endo release
performance
Performance related issues
Zoe Contract
Contracts within Zoe
Zoe
package: Zoe
Comments
Chris-Hibbert
added
enhancement
|
upgrading the scaledPriceAuthority contracts worked like a charm. Afterwards, allocation performance across all vats was flat during a test of sending 100 price updates. The remaining issue is that these vats still hold onto uncollected cycles. We have choices about this, since we have designs for
The only approach among these that is plausibly ready for releasing this week (planned cut of upgrade 16) would be upgrading the scaledPriceAuthority contracts so they no longer add to recoverySets. This stops the allocation problem from getting worse, but doesn't start us on a path to removing the unneeded objects. If we're soon going to rebuild the priceFeed paths without the scaledPriceAuthority contracts, that may not be of much value. If we're confident that we'll soon be able to terminate vats and slowly release their accumulated objects, it may be better to wait. |
|
I'm not sure what was going on in the graph above, but that code was directing a "null upgrade", which wasn't what we intended. Here's a graph with an actual upgrade: 
The blue line is kernel allocations, and uses the left scale (48500-49600, with slight growth). Zoe is the green light, and along with the other vats shows very steady allocations. |
mergify bot
added a commit
that referenced
this issue
May 24, 2024
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. <details> <summary><b>Expand</b> for performance graphs</summary> <img width="809" alt="image" src="https://github.com/Agoric/agoric-sdk/assets/13372636/889bb283-89c8-434f-8a67-efa56d0382ad"> 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. </details> ### 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 `scaledPriceAuthority` in their label. That should work on or test chains as well as MainNet. These changes should be included in the next release.
mergify bot
added a commit
that referenced
this issue
Jun 26, 2024
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.
mhofman
pushed a commit
that referenced
this issue
Jun 26, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is the Problem Being Solved?
#8400 reported that the priceFeed vats hold onto old quotes in their recoverySet, preventing them from being collected. #9283 applied the fixes to master. Measurements show that these fixes address the growth in priceFeed vats and Zoe, but leave a new issue visible in scaledPriceAuthorities.
Description of the Design
It's likely that upgrading the PriceFeed vats to incorporate the ERTP changes that addressed the previous issue will resolve this. Try this in a3p, and measure the results.
Security Considerations
None.
Scaling Considerations
Addressing scaling issues.
Test Plan
run in a3p and measure.
Upgrade Considerations
yes.
The text was updated successfully, but these errors were encountered: