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

Changes from all 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
74 changes: 49 additions & 25 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 @@ -118,6 +118,8 @@ export const makeOfferSpecShape = (bidBrand, collateralBrand) => {
export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
const makeScaledBidBook = prepareScaledBidBook(baggage);
const makePriceBook = preparePriceBook(baggage);
// Brands that have or are making active quoteNotifier Observers
const observedBrands = new Set();
Comment on lines +121 to +122
Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to get the relevant invariant in my head.

Something that combines observedBrands with state.updatingOracleQuote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

state.updatingOracleQuote has a value when we've received an update since the last auction. observedBrands contains the brand if the priceNotifier observer has been started and hasn't returned an error or called finish.

In the normal case, observedBrands doesn't change, but state.updatingOracleQuote gets reset every auction.

The interesting cases are startup and if the notifier ever breaks. The problem with startup is that it can take an arbitrary timespan for the oracles to start reporting prices after the auction starts. We want to start the observer, but we don't want to do it multiple time (as we were previously doing) because each creates a new observer that persists.

Copy link
Member

Choose a reason for hiding this comment

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

Can we explain why it's ok to have this be a heap Set? Both from a cardinality and upgradability pov.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brian and I talked through this. If we decide to upgrade auctions, then the restart won't have an observer running, but it won't have reset any tracking variable we create, so it's important that the tracking go away on restart.

I think the cardinality question is about the fact that we're sharing a Set across auctoinBooks. We don't have ephemeral per-object state, but we can have ephemeral shared state. The number of brands handled by the auction is small and seldom grows, so the set won't get too big. We're already keeping all the auctionBooks in a single vat.

Copy link
Member

Choose a reason for hiding this comment

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

I figured as much for the upgradability side, but I would prefer this to be written out as comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, we lost the race with the merge. Please let me know if you think it's worth a new PR to add the comment. I'm happy to do it if you ask.


const AuctionBookStateShape = harden({
collateralBrand: M.any(),
Expand Down Expand Up @@ -454,38 +456,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 (observedBrands.has(collateralBrand)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using the collateralBrand as key instead of the identity of this auctionBook? Are we guaranteed there is only ever a 1:1 relationship between auctionBook and collateralBrand?

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 rest of auctionBook and auctioneer use the collateralBrand, so we're already reliant on it being 1:1.

Copy link
Member

@mhofman mhofman Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So there is never multiple bid brands for a given collateral? I'm just wary of assumptions like this that aren't super clear or documented in code comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the bids are in IST, so there's no chance of that.

With Vaults, we explicitly intended that we might want to have multiple vaultManagers for a given currency that could have different minimums, or other requirements, and charge different rates.

With auctions, it's a mistake to split the bidding equity across multiple auctions, so the design was always that each auctionBook would handle all the bids and asks for a particular asset.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think having a single bidding denom is the assumption that wasn't clear to me.

return;
}
observedBrands.add(collateralBrand);
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;
observedBrands.delete(collateralBrand);
},
finish: done => {
trace(
`quoteNotifier invoked finish(${done}). setting quote to null`,
);
// lack of quote will trigger restart
state.updatingOracleQuote = null;
observedBrands.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.

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

void facets.helper.publishBookData();
},
Expand Down Expand Up @@ -645,8 +668,9 @@ 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 (or hasn't been started for this
// incarnation), (re)start it.
facets.helper.ensureQuoteNotifierObserved();
return;
}

Expand Down Expand Up @@ -750,7 +774,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
Loading