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.

Sign up for GitHub

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

don't reschedule auction's price notifier if we already have one #10615

Merged
merged 9 commits into from
Dec 4, 2024
Merged

don't reschedule auction's price notifier if we already have one #10615

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e19377
feat: don't reschedule notifier if we already have one
Chris-Hibbert Dec 4, 2024
a12a965
feat: robustify startup of the quoteNotifier observer
Chris-Hibbert Dec 4, 2024
ba64933
feat: use a map based on collateralBrand for each auctionBook
Chris-Hibbert Dec 4, 2024
77f2260
chore: drop the map; use an instance variable in state
Chris-Hibbert Dec 4, 2024
ee6c0a1
refactor: use a non-durable set to reflect the observers' lifetime
Chris-Hibbert Dec 4, 2024
bff3484
chore: drop meta declaration
Chris-Hibbert Dec 4, 2024
59406eb
chore: update comment
Chris-Hibbert Dec 4, 2024
e88aebe
Merge branch 'master' into cth-auction-priceNotifier
mergify[bot] Dec 4, 2024
466d3b8
Merge branch 'master' into cth-auction-priceNotifier
mergify[bot] Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 57 additions & 26 deletions packages/inter-protocol/src/auction/auctionBook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import { Fail } from '@endo/errors';
import { E } from '@endo/captp';
import { AmountMath, RatioShape } from '@agoric/ertp';
import { mustMatch } from '@agoric/store';
import { M, prepareExoClassKit } from '@agoric/vat-data';
import {
M,
prepareExoClassKit,
provideDurableMapStore,
} from '@agoric/vat-data';

import { assertAllDefined, makeTracer } from '@agoric/internal';
import {
Expand Down Expand Up @@ -118,6 +122,12 @@ export const makeOfferSpecShape = (bidBrand, collateralBrand) => {
export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
const makeScaledBidBook = prepareScaledBidBook(baggage);
const makePriceBook = preparePriceBook(baggage);
// a map from collateralBrand to true when the quoteNotifier has an observer
// the brand is absent when there's no observer.
const quoteNotifierFlags = provideDurableMapStore(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, hey, should this just be a DurableMapSet?

baggage,
'quoteNotifierFlags',
);

const AuctionBookStateShape = harden({
collateralBrand: M.any(),
Expand Down Expand Up @@ -454,38 +464,59 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
});
return state.bookDataKit.recorder.write(bookData);
},
observeQuoteNotifier() {
// Ensure that there is an observer monitoring the quoteNotifier. We
// assume that all failure modes for quoteNotifier eventually lead to
// fail or finish.
Comment on lines +459 to +461
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I read this as "We rely on SOMETHING to see that all failure modes for quoteNotifier eventually lead to fail or finish"? What is the SOMETHING?

If we don't rely on something else, we have to ensure it here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption is baked into the design of the notifiers and observers. There's no way to interrupt it from the consumer end, and no way to verify that it's still alive. Its contract says if it every ceases to continue producing new values it will either call fail or finish, or return an exception because the vat died.

ensureQuoteNotifierObserved() {
const { state, facets } = this;
const { collateralBrand, bidBrand, priceAuthority } = state;

if (quoteNotifierFlags.has(collateralBrand)) {
return;
}
quoteNotifierFlags.init(collateralBrand, true);
trace('observing');

const quoteNotifier = E(priceAuthority).makeQuoteNotifier(
const quoteNotifierP = E(priceAuthority).makeQuoteNotifier(
AmountMath.make(collateralBrand, QUOTE_SCALE),
bidBrand,
);
void observeNotifier(quoteNotifier, {
updateState: quote => {
trace(
`BOOK notifier ${priceFrom(quote).numerator.value}/${
priceFrom(quote).denominator.value
}`,
);
state.updatingOracleQuote = priceFrom(quote);
},
fail: reason => {
trace(`Failure from quoteNotifier (${reason}) setting to null`);
// lack of quote will trigger restart
state.updatingOracleQuote = null;
},
finish: done => {
trace(
`quoteNotifier invoked finish(${done}). setting quote to null`,
);
// lack of quote will trigger restart

void E.when(
quoteNotifierP,
quoteNotifier =>
observeNotifier(quoteNotifier, {
updateState: quote => {
trace(
`BOOK notifier ${priceFrom(quote).numerator.value}/${
priceFrom(quote).denominator.value
}`,
);
state.updatingOracleQuote = priceFrom(quote);
},
fail: reason => {
trace(
`Failure from quoteNotifier (${reason}) setting to null`,
);
// lack of quote will trigger restart
state.updatingOracleQuote = null;
quoteNotifierFlags.delete(collateralBrand);
},
finish: done => {
trace(
`quoteNotifier invoked finish(${done}). setting quote to null`,
);
// lack of quote will trigger restart
state.updatingOracleQuote = null;
quoteNotifierFlags.delete(collateralBrand);
},
}),
e => {
trace('makeQuoteNotifier failed, resetting', e);
state.updatingOracleQuote = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to reset the state here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's safe because we'll check for a notifier again at the next auction. It's a good idea because we can't count on the notifier continuing to produce events if there's an error at this level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't quite understand in which case this isn't already null. This is an error making the notifier, so afaiu we would never have updated this state in the first place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't explore it deeply, but I think if a previous version was happily updating the state, then it upgrades, then anything which samples the price at the beginning of the new incarnation (before any new updates happen) will get the old price. If an auction then starts and initiates the observer process, but fails (maybe because the price feed vat is getting upgraded too?), then we'll hit this case, clear the ephemeral flag (so the next auction can initiate a new one), and also clear the durable state.updatingOracleQuote, which at least will tell clients to not rely on the old quote.

I suspect that's a bit weird. Not sure if it's a problem though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 469, we add the brand to the set, and on line 472, we attempt to create the notifier. On line 477, we await the notifier. If the creation promise fails, we end up here, and want to ensure the brand is removed from the set, or we'll never try again to create a notifier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure the brand is removed from the set, or we'll never try again to create a notifier.

I have no problem with the ephemeral set being reset on error. My concern is with how the durable updatingOracleQuote state is managed. Resetting it at this state doesn't seem correct to me, and might be an indication something else is amiss.

Brian mentions a case where this durable state may not be null: after an upgrade. I suppose the product question is whether we want to maintain or not the old "updating" quote while we attempt to restart the observer.

But beyond that question, the problem is that the ensureQuoteNotifierObserved call is gated on !updatingOracleQuote in the first place, so really it seems like restarting the vat wouldn't trigger the observer creation anyway because that state wouldn't be reset by the upgrade, and the contract would think it still has a notifier.

IMO, we need to switch the call to ensureQuoteNotifierObserved to be unconditional, return true if the observer is in the Set, and if not immediately reset the updatingOracleQuote (if we want to avoid pretending we have an updating quote), start the notifier, and return false so that captureOraclePriceForRound can continue bailing out early. In that case updatingOracleQuote wouldn't need to be reset if we fail to make the observer since we'd be guaranteed for it to already be null (again if we don't want to pretend we have an updating quote while re-establishing the observer).

Regarding the early bail out of captureOraclePriceForRound, I'm a little surprised that we would skip capturing a price for round when the observer isn't yet setup instead of delaying the capture, but that's a different topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, if the auctionBook has a price (in durable state.updatingOracleQuote), then gets restarted/upgraded, the new incarnation won't start a new observer at startup (because nothing special happens at restart, it's not like we walk through all the pre-existing brands and do something for each one), and when the auction timer fires and it calls captureOraclePriceForRound(), that will see the old price and assume it's ok, and won't start a replacement observer then either.

I agree with @mhofman that the captureOraclePriceForRound() needs to check the ephemeral set instead of the durable state, or simply always call ensureQuoteNotifierObserved() and rely on its internal check to prevent duplicates.

quoteNotifierFlags.delete(collateralBrand);
},
});
);

void facets.helper.publishBookData();
},
Expand Down Expand Up @@ -645,8 +676,8 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {

trace(`capturing oracle price `, state.updatingOracleQuote);
if (!state.updatingOracleQuote) {
// if the price has feed has died, try restarting it.
facets.helper.observeQuoteNotifier();
// if the price feed has died, restart it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add "(or hasn't been started for this incarnation yet)"

facets.helper.ensureQuoteNotifierObserved();
return;
}

Expand Down Expand Up @@ -750,7 +781,7 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
const { collateralBrand, bidBrand, priceAuthority } = state;
assertAllDefined({ collateralBrand, bidBrand, priceAuthority });

facets.helper.observeQuoteNotifier();
facets.helper.ensureQuoteNotifierObserved();
},
stateShape: AuctionBookStateShape,
},
Expand Down
8 changes: 8 additions & 0 deletions packages/inter-protocol/src/auction/auctioneer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,14 @@ export const distributeProportionalSharesWithLimits = (
return transfers;
};

// auctionBook changed to create a sub-baggage for each book (to distinguish
// their quoteNotifier flags) so older auctions will not be upgradable to this
// version. We believe this version is saving all the necesary state to be
// upgraded, but that hasn't been verified, so we don't mark it as `canUpgrade'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of those meta flags is that the first sentence ("older versions will not be upgradable to this version") is the reason we don't mark this as canUpgrade. The second sentence ("saving all the necessary state") is what justifies our use of canBeUpgraded. The fact that we haven't verified it is either good note to add but doesn't change the flags, or a reason to not use canBeUpgraded.

My opinion is that canBeUpgraded is appropriate, but I don't know how much of a stickler we are for "there must be tests to prove this claim before we're allowed to make it".

So maybe add ", so we do not mark this as canUpgrade" to the first sentence, and maybe make the second sentence be like "We believe.., so we mark this as canBeUpgraded (but this hasn't been verified with an a3p test)".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we haven't verified it is either good note to add but doesn't change the flags, or a reason to not use canBeUpgraded.

If we had verified it, it would be 'canUpgrade. The only time it should be canBeUpgraded` is when we hope it's upgraded, but haven't proven it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought canUpgrade is backwards-looking, while canBeUpgraded is forward-looking, and that there was no way to express "probably upgradable but not tested".

If I understand it correctly, canUpgrade means that this code is capable of upgrading a previous version, which we know is false here because the sub-baggage we added.. (actually we just changed that, we're no longer modifying the way baggage is used, so maybe this could upgrade the previous version, but I imagine there are other reasons that might not work).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never did know the difference. Ooh! we have docs now:

   * - `canUpgrade` means this code can perform an upgrade
   * - `canBeUpgraded` means that the contract stores kinds durably such that the next version can upgrade

So I concur that canBeUpgraded is correct and that any test for canUpgrade from the previous version is certain to fail.

export const meta = {
upgradable: 'canBeUpgraded',
};

/**
* @param {ZCF<
* GovernanceTerms<typeof auctioneerParamTypes> & {
Expand Down
Loading