fix: watch ERTP purse balances across zoe upgrades (release branch) #8557
Conversation
dckc
force-pushed
the
8293-upgrade-watch-balance
branch
from
4fa6332
to
8e5c74e
Compare
dckc
changed the title
There was a problem hiding this comment.
Code reviewed together. Pushed some commits, LGTM.
the failure case now throws. It used to log and return undefined. Is this a good API change?
Yes good. We confirmed the only consumers use void instead of awaiting the promise, so the change is basically to log as an UnhandledPromiseRejection instead of a simple console message.
Should it target master and then get back-ported to the release branch?
Both. Currently, it needs to be on the release branch to be released. We also need it on master going forward.
| console.error(address, `failed updateState observer`, reason); | ||
| }, | ||
| }); | ||
| // This would seem to fit the observeNotifier() pattern, |
dckc
changed the title
- provision a smartWallet for an oracle operator - upgrade zoe; reproduce smartWallet bug - check for new invitation
observeNotifier() would handle upgrade disconnects if the notifiers were durable. But in @agoric/ertp, they're ephemeral, so we open-code the loop.
dckc
force-pushed
the
8293-upgrade-watch-balance
branch
from
b0abdb4
to
e166f4a
Compare
There was a problem hiding this comment.
Besides the surprising tail recursion, I'm a little confused as to how this new subscription will apply to existing purses, since this was and remains an active / heap subscription, and I don't see any iteration over existing purses during upgrade.
I understand this is a separate issue (durability of the subscriber), but I'm surprised to not see a mention of it in Upgrade Considerations given that this code requires an upgrade of the smart wallet vat.
| } | ||
| } catch (err) { | ||
| if (isUpgradeDisconnection(err)) { | ||
| helper.watchPurse(purse); // retry |
There was a problem hiding this comment.
No return? Not sure who is the initial caller but this looks like a tail recursion, and it feels weird without a return.
There was a problem hiding this comment.
Actually thinking about this more, this seem too simplistic, and doesn't handle the same disconnection error being replayed, which is why tools like reconnectAsNeeded exist which check the incarnation in the disconnect error.
There was a problem hiding this comment.
doesn't handle the same disconnection error being replayed
What am I missing? The way I understand things, the test is compelling evidence that it does.
There was a problem hiding this comment.
I think the problem is in case of a proxy between the notifier in the upgraded vat, and the subscriber. It's possible that this is not the case here, but you could get in an infinite loop if the subscription request somehow rejects with the same disconnect error. The increasing incarnation number is there to help differentiate the case of replayed disconnect error, and should be checked when re-attempting subscription.
There was a problem hiding this comment.
No return? Not sure who is the initial caller but this looks like a tail recursion, and it feels weird without a return.
Agreed. #8573 has a return.
@dckc which PR should be reviewed? I support this one first and the port be in draft until this lands, so that it can match whatever changes are made here
|
Besides that, the code only changes the smart wallet vat, so is safe from the point of view of targeting the |
There was a problem hiding this comment.
Paging @gibson042 who is likely more familiar with the disconnect errors than me.
| } | ||
| } catch (err) { | ||
| if (isUpgradeDisconnection(err)) { | ||
| helper.watchPurse(purse); // retry |
There was a problem hiding this comment.
Actually thinking about this more, this seem too simplistic, and doesn't handle the same disconnection error being replayed, which is why tools like reconnectAsNeeded exist which check the incarnation in the disconnect error.
That's a problem with upgrading the walletFactory; for that, see #8286. This PR is about a problem when upgrading zoe. Perhaps they're not entirely orthogonal. I suppose this could depend on a fix for #8286. But I would prefer to land this independently. |
Good point. I added something in Upgrade Considerations. |
That issue mentions problems with offer notifications, but here it seems to be an upgrade problem with purse balances. Is that issue too narrowly described? These fixes are indeed not entirely orthogonal as this new code won't take effect on upgrade for existing purses, which to me seems like an upgrade concern for the issues of "purse balances stopping to update", albeit not when the issuer/zoe is upgraded, but when the smart wallet vat is. (Also, how was this not a concern in the 11wf upgrade?) |
The only purses were vbank purses and the zoe invitation purse. walletFactory doesn't subscribe to vbank purse updates (we don't provide them: #5896). The smartWallet |
refs: #8293
Description
observeNotifier(...)would handle upgrade disconnects if the notifiers were durable. But in the@agoric/ertpimplementation, they're ephemeral. So we open-code the loop.Security / Scaling / Documentation Considerations
I don't see any.
Upgrade Considerations
This change is not intended to ship on its own. Fixes for #8286 and #8292 are intended for the same upgrade.
This PR targets the release-mainnet1B branch. A separate PR targeting master is in progress.
Testing Considerations
The first commit reproduces #8293 by upgrading zoe and noting the balance update failure.
The 2nd commit fixes it.