feat: auctioneer detects failing priceAuthority; requests new one #8691

Chris-Hibbert · 2023-12-22T17:49:36Z

refs: #8675

Description

When observeNotifier() on the quoteNotifier detects an error, request a new notifier and restart the observer.

Security Considerations

robustness for the chain.

Scaling Considerations

Not a scaling issue.

Documentation Considerations

No user docs required.

Testing Considerations

Added a test that the auctioneer switches when the manual authority throws an error.

Upgrade Considerations

The priceAuthority is holding onto cyclic data structures involving exitObjects. In order to force GC of those objects, we plan to fix the problem in Zoe and then abandon the old priceAuthorities. This fix allows the Auctions to survive when the old priceAuthority is killed.

Chris-Hibbert · 2023-12-22T22:50:48Z

I added a commit ("If the quoteNotifier throws, don't restart immediately") that doesn't restart immediately in the failure case, based on a test that failed with an infinite restart loop when the ATOM-USD_price_feed vat was restarted, but scaledPriceAuthority-ATOM remained alive. Please give me an opinion on whether this code is worth including.

With this fix in, the change to restart-vats.js isn't necessary.

packages/inter-protocol/src/auction/auctionBook.js

packages/inter-protocol/test/auction/test-auctionContract.js

packages/inter-protocol/src/auction/auctionBook.js

turadg · 2024-01-04T17:46:48Z

packages/inter-protocol/src/auction/auctionBook.js

+                fail: _reason => {
+                  trace('fail', _reason);


no strong opinion here, but the value is used.

Suggested change

fail: _reason => {

trace('fail', _reason);

fail: reason => {

trace('fail', reason);

I improved the Trace message, and renamed it to reason

same issue with _done

turadg · 2024-01-04T17:49:10Z

packages/inter-protocol/src/auction/auctionBook.js

+                },
+                finish: _done => {
+                  trace('finish', _done);
+                  facets.helper.observeQuoteNotifier();


Suggested change

facets.helper.observeQuoteNotifier();

// start a fresh observer

facets.helper.observeQuoteNotifier();

I copied the version from fail(). In testing, I noticed that if it's really broken, resetting the notifier immediately makes an infinite loop. Setting the quote to empty OTOH, makes a much slower infinite loop. (It only retries at the auction frequency, which means other auctions can continue.)

packages/inter-protocol/src/auction/auctionBook.js

turadg · 2024-01-04T17:51:40Z

packages/inter-protocol/test/auction/test-auctionContract.js

@@ -58,15 +58,15 @@ const test = anyTest;

 const trace = makeTracer('Test AuctContract', false);

-const defaultParams = {
+const defaultParams = harden({


packages/inter-protocol/test/auction/test-auctionContract.js

turadg · 2024-01-04T18:01:27Z

Please give me an opinion on whether this code is worth including

I don't have a strong position either way. I first look to the runtime impact, I don't know the cost of restarting the observer unnecessarily. My next consideration is code complexity (interpretability and maintenance). The creates an extra state to have to understand and more room for bugs, but it also improves encapsulation by not requiring changes in restart-vats.

But… what happens if a quote is requested and the price is zero? Could that cause new failures downstream?

If it's any risk I say leave it out.

turadg · 2024-01-05T17:51:51Z

packages/inter-protocol/src/auction/auctionBook.js

@@ -630,6 +679,12 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
          const { facets, state } = this;

          trace(`capturing oracle price `, state.updatingOracleQuote);
+          if (AmountMath.isEmpty(state.updatingOracleQuote.numerator)) {


checking for empty seemed a little magic to me and I had to keep track of the Ratio getting out as an actual quote.

I thought a null value would be more clear. To test it out I made the changes locally. They passed and since I know this PR is somewhat urgent I pushed the change. I'm approving with the change. If I missed something and it's a bad idea, please re-request review and help me understand.

turadg · 2024-01-06T03:44:27Z

@Chris-Hibbert now that there are no zero ratios, I think your e3b0e6b is low risk and worthwhile for the encapsulation

github-actions · 2024-01-07T08:00:34Z

@Chris-Hibbert - This PR appears to be stuck. It's had a merge label for > 24 hours

If the quoteNotifier is broken, set updatingOracleQuote to null. If updatingOracleQuote is null when attempting to capture the oracle price, restart observeQuoteNotifier Vaults detects failing priceAuthority and requests new one (#8696)

Chris-Hibbert · 2024-01-08T23:12:17Z

@turadg Since #8697 was merged here, the change to restart-vats, (since reverted) showed that vaultManager wasn't as robust as auctions to a priceAuthority failing. I've made it more robust, but this isn't a trivial change, so I'm removing the automerge label and re-requesting review.

turadg · 2024-01-08T23:39:41Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

@@ -452,7 +452,15 @@ export const prepareVaultManagerKit = (
              ephemera.storedCollateralQuote = value;
            },
            onRejected() {
-              facets.helper.observeQuoteNotifier();
+              // XXX drastic action, if the quoteNotifier fails, we don't know


great comment overall.

XXX marks tech debt. Do you have ideas for improving this? If not, it seems more like a NOTE or NB (https://en.wikipedia.org/wiki/Nota_bene)

also, small typo: ingorance.

turadg

Logic looks good. One type annotation request before approving

turadg · 2024-01-09T19:24:12Z

packages/inter-protocol/src/vaultFactory/vaultManager.js

+              // notifier immediately, we'll trigger an infinite loop, so try
+              // to restart each time we get a request.
+
+              // @ts-expect-error reset value


this should be put into the typedef instead of suppressed.
line 205:

* storedCollateralQuote: PriceQuote | null;

…oric#8691) * feat: auctioneer/vaults detect failing priceAuthority & request new If the quoteNotifier is broken, set updatingOracleQuote to null. If updatingOracleQuote is null when attempting to capture the oracle price, restart observeQuoteNotifier Vaults detects failing priceAuthority and requests new one (Agoric#8696) * feat: make vaultManager robust against failing priceAuthority * chore: fix types, drop expect-error --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Chris-Hibbert added performance Performance related issues resource-exhaustion Threats to availability from resource exhaustion attacks oracle Related to on-chain oracles. auction labels Dec 22, 2023

Chris-Hibbert self-assigned this Dec 22, 2023

Chris-Hibbert force-pushed the 8675-auction-newPA branch from 7a1784b to 9583b28 Compare December 22, 2023 19:46

Chris-Hibbert force-pushed the 8675-auction-newPA branch 2 times, most recently from c00dbe2 to d940a99 Compare December 26, 2023 21:31

Chris-Hibbert requested a review from turadg December 26, 2023 22:06

turadg reviewed Jan 4, 2024

View reviewed changes

turadg approved these changes Jan 5, 2024

View reviewed changes

Chris-Hibbert added the automerge:squash Automatically squash merge label Jan 6, 2024

Chris-Hibbert force-pushed the 8675-auction-newPA branch from 4a6d7f4 to b08d1de Compare January 8, 2024 18:28

Chris-Hibbert removed the automerge:squash Automatically squash merge label Jan 8, 2024

Chris-Hibbert requested a review from turadg January 8, 2024 23:12

turadg reviewed Jan 8, 2024

View reviewed changes

feat: make vaultManager robust against failing priceAuthority

bb8ce8c

Chris-Hibbert force-pushed the 8675-auction-newPA branch from 7f6aec5 to bb8ce8c Compare January 9, 2024 01:49

turadg reviewed Jan 9, 2024

View reviewed changes

chore: fix types, drop expect-error

1a5dc39

Chris-Hibbert added the automerge:squash Automatically squash merge label Jan 9, 2024

Merge branch 'master' into 8675-auction-newPA

1dd65f2

mergify bot merged commit 8604b01 into master Jan 9, 2024
66 checks passed

mergify bot deleted the 8675-auction-newPA branch January 9, 2024 23:13

Chris-Hibbert mentioned this pull request Feb 7, 2024

prepare updates for Upgrade 15 #8868

Closed

14 tasks

Chris-Hibbert mentioned this pull request Mar 20, 2024

Auction and Vaults should request new PriceAuthority on failure #8675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auctioneer detects failing priceAuthority; requests new one #8691

feat: auctioneer detects failing priceAuthority; requests new one #8691

Chris-Hibbert commented Dec 22, 2023

Chris-Hibbert commented Dec 22, 2023

turadg Jan 4, 2024

Chris-Hibbert Jan 4, 2024

turadg Jan 5, 2024

turadg Jan 4, 2024

Chris-Hibbert Jan 4, 2024

turadg Jan 4, 2024

turadg commented Jan 4, 2024

turadg Jan 5, 2024

Chris-Hibbert Jan 6, 2024

turadg commented Jan 6, 2024

github-actions bot commented Jan 7, 2024

Chris-Hibbert commented Jan 8, 2024

turadg Jan 8, 2024

Chris-Hibbert Jan 9, 2024

turadg left a comment

turadg Jan 9, 2024

Chris-Hibbert Jan 9, 2024

	facets.helper.observeQuoteNotifier();
	// start a fresh observer
	facets.helper.observeQuoteNotifier();

feat: auctioneer detects failing priceAuthority; requests new one #8691

feat: auctioneer detects failing priceAuthority; requests new one #8691

Conversation

Chris-Hibbert commented Dec 22, 2023

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Chris-Hibbert commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Jan 6, 2024

github-actions bot commented Jan 7, 2024

Chris-Hibbert commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment