set roundId to 1 & write to vstorage at start #10291

Chris-Hibbert · 2024-10-17T00:31:47Z

refs: #9886

Description

Some tests in DevNet indicate that the Oracles need the roundID in vstorage to be reset so they can sync.

Security Considerations

Not a security issue.

Scaling Considerations

Not directly a scaling concern. (see #8400).

Documentation Considerations

not user visible. It would be nice if the protocol between oracles and fluxAggregators were documented somewhere

Testing Considerations

updated the a3p tests to increment the roundId before the upgrade and show that this upgrade resets it.

Upgrade Considerations

This change is to the fluxAggregator which is already being started afresh in this proposal.

dckc

I'm pretty sure parts of this are not right; not certain enough to request changes, though.

dckc · 2024-10-17T00:51:38Z

packages/boot/test/bootstrapTests/price-feed-replace.test.ts

  } = t.context;

  await setupVaults(collateralBrandKey, managerIndex, setup);

  const instancePre = agoricNamesRemotes.instance['ATOM-USD price feed'];

+  await priceFeedDrivers[collateralBrandKey].setPrice(15.99);
+
+  t.like(readLatest('published.priceFeed.ATOM-USD_price_feed.latestRound'), {


We should test before setPrice (and after the core-eval) to match the problem we saw in practice, right?

I added a test before that shows that roundID is 1.

The test on line 148 (it'll move, so early in '2. trigger liquidation by changing price') checks after the coreEval and before setting prices again. Do you want another check in this test ('setupVaults')?

hm. I think I wasn't reading this quite right. no, I don't see any more tests that I want.

dckc · 2024-10-17T01:00:51Z

packages/inter-protocol/src/price/fluxAggregatorKit.js

@@ -180,6 +180,9 @@ export const prepareFluxAggregatorKit = async (
        }),
      ),
  });
+  const now = await E(timerPresence).getCurrentTimestamp();


I don't think we can put a remote call like that in the critical path to defining this kind. If this doesn't cause a null-upgrade test to fail, I think we're missing some coverage.

Define all exo classes/kits before any incoming method calls from other vats -- in the first "crank".
-- https://docs.agoric.com/guides/zoe/contract-upgrade.html#crank

How about a helper.startInitializingVstorage(timerPresence) thing that returns synchronously but asynchronously gets a timestamp and initializes vstorage?

dckc · 2024-10-17T01:01:28Z

packages/inter-protocol/src/price/roundsManager.js

-const trace = makeTracer('RoundsM', false);
+const trace = makeTracer('RoundsM', true);


Do you mean to turn that on in production?
(I don't object. I'm just checking that you didn't mean to turn it back off.)

I went back and forth. I settled on yes, because we're diagnosing issues here, and it would give some added visibility.

dckc · 2024-10-17T01:05:24Z

packages/inter-protocol/src/price/roundsManager.js

+          const rounds = makeScalarBigMapStore('rounds', { durable: true });
+          const details = makeScalarBigMapStore('details', { durable: true });
+          // @ts-expect-error append-only except for reset
+          this.state.rounds = rounds;
+          // @ts-expect-error append-only except for reset
+          this.state.details = details;


Isn't this already done in the init function? Are we clobbering these 2 map stores?

I think you're right. I will drop the externally invoked function, and "improve" the initializer.

dckc · 2024-10-17T01:10:50Z

packages/inter-protocol/src/price/roundsManager.js

+          // @ts-expect-error append-only except for reset
+          this.state.details = details;
+
+          details.init(


I'm not confident that I know the invariants of the state.details map stores. @michaelfig or @turadg are you able to tell from a quick look that this is OK?

dckc · 2024-10-17T01:11:07Z

packages/inter-protocol/src/price/roundsManager.js

+            answeredInRound: 0n,
+          });
+
+          rounds.init(roundId, round);


again, I'm not confident about this.

cloudflare-workers-and-pages · 2024-10-17T06:14:19Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`1bc091a`
Status:	✅ Deploy successful!
Preview URL:	https://f37ffd68.agoric-sdk.pages.dev
Branch Preview URL:	https://9886-resetroundid.agoric-sdk.pages.dev

View logs

Use a minimum start time and roundId of 1 because zero is treated as an outlier in the code There are places in the code and tests that compare roundId and round start time to 0 and 1, so make it easier by starting from 1. Refactored some tests, mostly by adding a round before the previous tests to let state get past zero and one. refactor: set roundId to 1 & write to vstorage at start use a minimum start time and roundId of 1 because zero is treated as an outlier in the code There are places in the code and tests that compare roundId and round start time to 0 and 1, so make it easier by starting from 1. Refactored some tests, mostly by adding a round before the previous tests to let state get past zero and one.

Chris-Hibbert · 2024-10-17T22:31:42Z

I dropped the changes in fluxAggregator, and did everything in roundsManager's finish().

The one issue is that the round record wants to include the time the round started, and we can't get the time in the first crank while creating the exo. I finished initializing in an E.when(). So the first price update can't happen until the time is received. But that's okay in practice since the oracles won't provide prices for the first round until they see the write to vstorage.

The invariants on round progression are enforced in supersedable, acceptingSubmissions, isNextRound, timedOut and validateOracleRound, among other places. Bleah.

dckc

presuming we test roundid between the core-eval and setPrice, as discussed

Chris-Hibbert added contract-upgrade oracle Related to on-chain oracles. force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release labels Oct 17, 2024

Chris-Hibbert self-assigned this Oct 17, 2024

Chris-Hibbert requested a review from a team as a code owner October 17, 2024 00:31

Chris-Hibbert requested review from AgoricTriage and dckc October 17, 2024 00:31

dckc reviewed Oct 17, 2024

View reviewed changes

Chris-Hibbert force-pushed the 9886-resetRoundID branch from d7a3337 to 9fcf745 Compare October 17, 2024 18:19

Chris-Hibbert force-pushed the 9886-resetRoundID branch from 9fcf745 to 84d09c7 Compare October 17, 2024 18:30

test: repair a test that needs more time for voting.

c302226

Chris-Hibbert requested a review from dckc October 17, 2024 22:22

dckc approved these changes Oct 17, 2024

View reviewed changes

test: add a check to show that the roundId has been published

018cf59

Chris-Hibbert added the automerge:squash Automatically squash merge label Oct 17, 2024

Merge branch 'master' into 9886-resetRoundID

1bc091a

mergify bot merged commit 36d0670 into master Oct 18, 2024
80 checks passed

mergify bot deleted the 9886-resetRoundID branch October 18, 2024 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set roundId to 1 & write to vstorage at start #10291

set roundId to 1 & write to vstorage at start #10291

Chris-Hibbert commented Oct 17, 2024

dckc left a comment

dckc Oct 17, 2024

Chris-Hibbert Oct 17, 2024

dckc Oct 17, 2024

dckc Oct 17, 2024

dckc Oct 17, 2024

Chris-Hibbert Oct 17, 2024

dckc Oct 17, 2024

Chris-Hibbert Oct 17, 2024

dckc Oct 17, 2024

dckc Oct 17, 2024

cloudflare-workers-and-pages bot commented Oct 17, 2024 •

edited

Loading

Chris-Hibbert commented Oct 17, 2024

dckc left a comment

		const trace = makeTracer('RoundsM', false);
		const trace = makeTracer('RoundsM', true);

set roundId to 1 & write to vstorage at start #10291

set roundId to 1 & write to vstorage at start #10291

Conversation

Chris-Hibbert commented Oct 17, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc left a 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

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

cloudflare-workers-and-pages bot commented Oct 17, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Chris-Hibbert commented Oct 17, 2024

dckc left a comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Oct 17, 2024 •

edited

Loading