feat(ertp): upgraded quotes incrementally empty recovery-sets #8498
Conversation
erights
changed the title
erights
changed the title
erights
force-pushed
the
markm-really-prepare-issuer-kit
branch
from
45d94ee
to
71cf064
Compare
Chris-Hibbert
force-pushed
the
markm-chris-recovery-sets-incremental-gc
branch
from
b33b024
to
fb64db7
Compare
| * `'hasRecoverySets'`. By contrast, the absence of `RecoverySetsOption` provide | ||
| * parameter defaults to the predecessor's `RecoverySetsOption` state, or | ||
| * `'hasRecoverySets'` if none. |
There was a problem hiding this comment.
I can't parse the "By contrast" sentence. I suspect 'defaults' is the verb, but I can't tell how to parse "provide parameter defaults". I think it's trying to tell me something about a situation where some parameter is going to default to a previous state, but I don't know which 'provide' method is referred to. Something resembling this was going on in preparePaymentLedger when it called preparePurseKind, but the description doesn't match up.
There was a problem hiding this comment.
This difference is more clearly represented by the distinction between variables labeled recoverySetsState and recoverySetsOption. If there is no previously stored recoverySetsState, then the implicit recoverySetsState is considered equivalent to 'hasRecoverySets' (and made explicit in oldRecoverySetsState). If there is no provided recoverySetsOption in the optional options parameter typed IssuerOptionsRecord, especially in the optional options parameter of reallyPrepareIssuerKit and upgradeIssuerKit), then this absence defaults to preserving oldRecoverySetsState as the new recoverySetsState. Otherwise, the new recoverySetsState is made the same as the recoverySetsOption if possible (and errors in not possible).
There was a problem hiding this comment.
The logic implementing this defaulting logic is in upgradeIssuerKit, but it should be understood at all call sites upstream of it with a IssuerOptionsRecord-typed options parameter or simply an optional recoverySetsOption parameter.
There was a problem hiding this comment.
Yes, fwiw, "defaults" is the verb of that bad sentence :( .
Chris-Hibbert
force-pushed
the
markm-chris-recovery-sets-incremental-gc
branch
from
423bda0
to
21f1aab
Compare
Chris-Hibbert
force-pushed
the
markm-chris-recovery-sets-incremental-gc
branch
from
21f1aab
to
646cccb
Compare
There was a problem hiding this comment.
I was waiting to understand the upgrade process. The process is that PRs should be approved on the master branch before they can be added to a release branch. This has been ready for a while. Now would be a good time to approve it for master, and prepare to add it to a branch for release to the chain.
cleanerRecoverySet() asserts paymentRecoverySet !== undefined when recoverSetState === noRecoverySets, so paymentLedger must provide it when calling preparePurseKind().
erights
force-pushed
the
markm-chris-recovery-sets-incremental-gc
branch
from
646cccb
to
fcde617
Compare
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.
Staged on #8497
Extracted from #8418 but adapted to incrementally empty the recovery sets, rather than dropping lots all at once.
closes: #XXXX
refs: #8418 #8497
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations