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

Zoe drop empty payments from escrow purses #8686

Open
Chris-Hibbert opened this issue Dec 21, 2023 · 5 comments
Open

Zoe drop empty payments from escrow purses #8686

Chris-Hibbert opened this issue Dec 21, 2023 · 5 comments
Assignees
Labels
enhancement New feature or request performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks Zoe package: Zoe

Comments

@Chris-Hibbert
Copy link
Contributor

What is the Problem Being Solved?

According to #8404, Zoe is holding onto empty Payments in recovery sets.

Description of the Design

The only purses that Zoe has are the escrow Purses. My presumption, if there are empty payments in the recovery set, is that those are mostly payouts that clients didn't bother to collect because they are empty. #8498 has a fix that will keep us from adding more empty payments to the recoverySets, but the existing ones will never go away unless they are purged from the EscrowPurses.

We can do that with one-time code, added in an upgrade (and removed later) that can iterate the recoverySets and drop empty payments.

Security Considerations

Be careful when dealing with Zoe's escrow purses.

Scaling Considerations

The scaling issues are resolved by the fix mentioned above the keeps the problem from getting worse. We should still clean up dead objects when reasonable.

Test Plan

Verify that the empty payments are removed. Doesn't have to be done in an actual chain or a docker test.

Upgrade Considerations

It's all about cleaning up dead objects via upgrade.

@Chris-Hibbert Chris-Hibbert added enhancement New feature or request Zoe package: Zoe performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks labels Dec 21, 2023
@Chris-Hibbert Chris-Hibbert self-assigned this Dec 21, 2023
@Chris-Hibbert
Copy link
Contributor Author

@erights and I discussed this.

The relevant recoverSet is for the IST escrow purse, so the issuer is local, and the query to find the balance of each payment is local.

Zoe can incrementally walk through the recovery set, looking for empty payments. Each time it does so, it can delete up to 10 (or some other number) payments. If it finds fewer than 10, it can remember that it's done.
We don't want to start the iteration from scratch each time, since this is n^2. We can save the iterator as long as the vat doesn't get upgraded, and continue from where we left off. Additionally, each iteration, we can remember the last purse we didn't remove and use that to restart the iteration on upgrade.

The recoverySet is a SetStore, which can be iterated using recoverySet.values(M.gte(lastNonEmpty)).

We're planning to filter out empty payments when constructing the return value for getRecoverySet(). This doesn't have much to with cleaning up the dead objects, but will give a consistent appearance that we never have empty payments in the recoverySet.

@erights
Copy link
Member

erights commented Feb 10, 2024

Better recoverySet.keys(lastNonEmpty) // Note: incorrect. See below

IIRC values is not incorrect. But better to think of a set as containing only keys rather than containing only values.

M.gte will not do what you want here. All payments are remotables. M.gte uses key order, which is a partial order in which one remotable is the same as itself and incomparable to all other remotables. retrieving with M.gte(lastNonEmpty) should start at the same place and internally do the same iteration from that place, but would then post-filter out all the other payments as not being gte lastNonEmpty.

@erights
Copy link
Member

erights commented Feb 10, 2024

No sorry, that's not correct as well. lastNonEmpty as a pattern will match only itself. I am now confused about how to express what we need. We might not be able to in terms of the current store API :(

At least we can hold onto the iterator in the heap and keep pulling on it until it becomes invalid.

@erights
Copy link
Member

erights commented Feb 10, 2024

Zoe can incrementally walk through the recovery set, looking for empty payments

Since we're trying to transition to a world in which payments with empty amounts are not in any recovery sets, I'd prefer this emptying logic be encapsulated within ERTP if possible. Or as much as possible.

@Chris-Hibbert
Copy link
Contributor Author

Since we're trying to transition to a world in which payments with empty amounts are not in any recovery sets, I'd prefer this emptying logic be encapsulated within ERTP if possible. Or as much as possible.

#9013 ensures that empty payments aren't added to recovery sets going forward.

I think these payments are held within the IST mint inside Zoe's vat (vat9). There shouldn't be any external references to them, so deleting them all shouldn't cascade to the kernel or other vats. It's theoretically possible that other vats are holding some uncollectable empty payments, but the cardinality is probably very low. I don't think we need general support in ERTP for cautious deletion. ERTP gives the owner of a purse the ability (getRecoverySet()) to get all the outstanding payments. No further support is needed.

I'm going to investigate in A3P whether I can count and deposit all the empty payments, and see if the cascading effects (or lack thereof) are visible in the slog file and sqlite db.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks Zoe package: Zoe
Projects
None yet
Development

No branches or pull requests

2 participants