feat(ertp): Upgrade quotes to drop newly optional recovery sets

[erights]

closes: #8400
refs: #8417 #8404 #8401

Description

Take 2 of #8406

Make recovery sets optional.

Enable an old issuer with recovery sets to be upgraded to one without, that also empties and suppresses any recovery sets it inherits.

Upgrade quote issuers to empty and suppress their recovery sets. This is

• appropriate for quote issuers because quote "payments" do not actually transfer tradable value. A new quote can always be obtained that is at least as good as an old lost quote.
• needed, because many existing quotes have been otherwise dropped, but are being pointlessly retained only by recovery sets.

One of the solution strategies for #8400 previously discussed is to deposit or burn the old quote "payments" in the recovery sets, in order to clean them up. This would be somewhat more traumatic for clients, though tolerably so. A client holding a pre-upgrade quote "payment" would find that it had been burned, requiring them to get a new payment if they wished to continue holding a valid one. This PR is much less traumatic. By just dropping the old recovery sets, any old payments that are actually being held remain as valid as they were. The only trauma would be anyone counting on recovering payments from recovery sets. We are confident no code using quotes does so.

Security Considerations

Recovery sets are a safety feature. This PR introduces the option to suppress that safety feature, including by upgrading old issuers with that feature to one without. If such an upgrade could be done maliciously, clients depending on this safety feature could be surprised by its absence.

Scaling Considerations

This PR is motivated by one scaling consideration: The storage leak of retaining old useless payments. In particular, dropped quote "payments".

If merged before #8417 is fixed, this PR would also cause a painful scaling issue. On upgrade, it will likely make a massive number of quote "payments" unreachable all at once. Currently, this will cause a large amount of gc-driven cleanup activity during that one crank and block, which can be problematic. Instead, the PR is written assuming #8417 has already been fixed, and depends on it to spread out the resulting gc-driven cleanup activity over a number of future cranks and blocks. Thus, the PR should not be merged until after #8417 is fixed.

Separate from this PR, we should find those places where we drop live payments of any kind, and see if we can safely burn or deposit them before dropping them.

Documentation Considerations

Any documentation of issuer creation should document the new option for suppressing recovery sets. This should explain both the storage-leak hazard of not suppressing them and the safety hazard of suppressing them.

Any documentation on use of recovery sets to recover old payments should make clear that this feature is now optional, and some issuers, such as the quote issuers, omit the feature.

Testing Considerations

The most important tests needed are the upgrade tests, discussed in the next section.

Even aside from upgrade, this PR needs more tests before it can become ready for review. At the time of this writing, all that is tested is that this PR does not break any existing tests.

• We also need tests that create issuerKits without recovery sets, and test that these behave as expected.

Since the point of opting out of upgrade tests is to avoid a durable storage leak, we should have

• a test where durable storage consumption grows linearly in the number of created and dropped payments with recovery sets,
• and (after gc) maintains a steady state without recovery sets.

Need to find out how to write such a test.

Upgrade Considerations

Need to test

• that old quote issuers with recovery sets upgrade to new quote issuers without recovery sets,
• that otherwise they continue to pass all the same tests
• that their behavior now reflects the suppression of recovery sets
• that the old space for dropped payments is indeed recovered
• Once rate-limited GC in BringOutYourDead #8417 is fixed, that the work of recovering that storage is adequately spread out across cranks and blocks.

We should also test

• that an old issuer without recovery sets cannot (yet?) be upgraded into an issuer with recovery sets. That is the intent as of this PR. Eager for feedback whether this upgrade direction should be supported. We could, but I'd rather not if no one would use it.

[Chris-Hibbert]

I think we shouldn't merge this until it includes a test showing that the upgrade will be successful. If you like, you can assign that to the userland team (No, I don't know how to assign a PR to a team.)

If addressing the leak is urgent, talk to P.M. about prioritization, or we'll be glad to give advice about adding upgrade tests (packages/deployment/upgrade-test/)

[erights]

I think we shouldn't merge this until it includes a test showing that the upgrade will be successful. If you like, you can assign that to the userland team (No, I don't know how to assign a PR to a team.)

Thanks for the offer! I agree, so I've put the PR in its current state into Draft. Let's revisit the mechanics of that reassignment once this PR otherwise seems to work.

If addressing the leak is urgent, talk to P.M. about prioritization, or we'll be glad to give advice about adding upgrade tests (packages/deployment/upgrade-test/)

This testing could proceed as soon as this PR is otherwise green. But merging must wait until after SwingSet rate limits gc activity anyway, so this one could only become urgent after it is unblocked on that.

cc @warner @ivanlei @rowgraus @turadg

[turadg]

Requesting that the PR description use the pull request template. That has a section about Upgrade Considerations, under which should be mentioned the need for an upgrade test.

Also requesting that this PR have such a test before merging. Userland can help with that, adapting

agoric-sdk/packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/zoe-full-upgrade/zcf-upgrade-script.js

Line 16 in 83b1fc8

const upgradeZoeAndZcf = async powers => {

[erights]

Requesting that the PR description use the pull request template. That has a section about Upgrade Considerations, under which should be mentioned the need for an upgrade test.

Thanks for suggesting that! Done.

Also requesting that this PR have such a test before merging. Userland can help with that, adapting

agoric-sdk/packages/deployment/upgrade-test/upgrade-test-scripts/agoric-upgrade-12/zoe-full-upgrade/zcf-upgrade-script.js

Line 16 in 83b1fc8

const upgradeZoeAndZcf = async powers => {

I would really appreciate that help, thanks!

[Chris-Hibbert]

@warner wrote:

The #8404 0-IST zoe Payments issue can probably be cleaned up one-fell-swoop, as of 01-dec we have 4k of them, and I think those should GC in about 13 seconds. So one long block and then it should be done.