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.

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

feat: make software upgrade coreProposals conditional on upgrade plan name #9575

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jun 24, 2024

closes: #9571

Description

Parametrize the price feed core proposals by the upgrade handler name. The proposal for a3p remains with the deprecated ambient (from environment) oracle addresses.

Additionally fix and add some logging to the upgrade vault proposal

Security Considerations

None

Scaling Considerations

Not a typical scaling consideration, but this introduces a "basic" upgrade name that skips all vaults related proposals. The correctly parametrized info can be provided in the chain software upgrade proposal's upgrade info for any chain that has different configuration.

Documentation Considerations

Need to communicate to validators that the pattern of upgrade names has changed.

Testing Considerations

A3P and manually tested on mainfork

Upgrade Considerations

See above

Copy link

cloudflare-workers-and-pages bot commented Jun 24, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: ea568a2
Status: ✅  Deploy successful!
Preview URL: https://6870a5d8.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-cond-software-upgrade.agoric-sdk.pages.dev

View logs

@michaelfig michaelfig force-pushed the mfig-cond-software-upgrade branch 2 times, most recently from b3dc1de to c5d8757 Compare June 25, 2024 03:54
@michaelfig michaelfig marked this pull request as ready for review June 25, 2024 04:17
mhofman added a commit that referenced this pull request Jun 25, 2024
Comment on lines 59 to 60
"agoric19d6gnr9fyp6hev4tlrg87zjrzsd5gzr5qlfq2p01", // Stakin
"agoric19uscwxdac6cf6z7d5e26e0jm0lgwstc47cpll8", // node
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"agoric19d6gnr9fyp6hev4tlrg87zjrzsd5gzr5qlfq2p01", // Stakin
"agoric19uscwxdac6cf6z7d5e26e0jm0lgwstc47cpll8", // node
"agoric19d6gnr9fyp6hev4tlrg87zjrzsd5gzr5qlfq2p", // Stakin
"agoric19uscwxdac6cf6z7d5e26e0jm0lgwstc47cpll8", // 01node

Copy link
Member Author

Choose a reason for hiding this comment

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

Awk!

Comment on lines 68 to 69
"agoric1qj07c7vfk3knqdral0sej7fa6eavkdn8vd8etf01", // Simply Staking
"agoric1ra0g6crtsy6r3qnpu7ruvm7qd4wjnznyzg5nu4", // node
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"agoric1qj07c7vfk3knqdral0sej7fa6eavkdn8vd8etf01", // Simply Staking
"agoric1ra0g6crtsy6r3qnpu7ruvm7qd4wjnznyzg5nu4", // node
"agoric1qj07c7vfk3knqdral0sej7fa6eavkdn8vd8etf", // Simply Staking
"agoric1ra0g6crtsy6r3qnpu7ruvm7qd4wjnznyzg5nu4", // 01node

Copy link
Member Author

Choose a reason for hiding this comment

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

And again!

@mhofman mhofman added the force:integration Force integration tests to run on PR label Jun 25, 2024
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

javascript changes LGTM. I have no opinion on the go code.

@@ -33,6 +33,9 @@ func isFirstTimeUpgradeOfThisVersion(app *GaiaApp, ctx sdk.Context) bool {
// price feed upgrade and associated changes to scaledPriceAuthority and
// vaultManager.
func upgradePriceFeedCoreProposalSteps(upgradeName string) ([]vm.CoreProposalStep, error) {
isThisUpgrade := func(expectedUpgradeName string) bool {
return upgradeName == expectedUpgradeName
Copy link
Member

Choose a reason for hiding this comment

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

Nice, can we also panic if expectedUpgradeName doesn't exist in upgradeNamesOfThisVersion, it's so easy to fat finger these names without enum typing

@mhofman mhofman dismissed their stale review June 25, 2024 19:09

Addressed

@mhofman
Copy link
Member

mhofman commented Jun 25, 2024

FYI I'm investigating why this is failing on mainfork, holding approval until then

@mhofman mhofman force-pushed the mfig-cond-software-upgrade branch from 337eb23 to 79f7713 Compare June 25, 2024 20:17
@mhofman mhofman changed the base branch from master to cth-repairUpgradeVaultsTest June 26, 2024 01:47
mhofman added a commit that referenced this pull request Jun 26, 2024
Base automatically changed from cth-repairUpgradeVaultsTest to master June 26, 2024 02:33
@mhofman mhofman force-pushed the mfig-cond-software-upgrade branch from 46636b0 to ea568a2 Compare June 26, 2024 02:46
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Jun 26, 2024
@mergify mergify bot merged commit cbe061c into master Jun 26, 2024
80 of 85 checks passed
@mergify mergify bot deleted the mfig-cond-software-upgrade branch June 26, 2024 03:19
mhofman pushed a commit that referenced this pull request Jun 26, 2024
…an name (#9575)

closes: #9571

## Description

Parametrize the price feed core proposals by the upgrade handler name. The proposal for a3p remains with the deprecated ambient (from environment) oracle addresses.

Additionally fix and add some logging to the upgrade vault proposal

### Security Considerations
None

### Scaling Considerations
Not a typical scaling consideration, but this introduces a "basic" upgrade name that skips all vaults related proposals. The correctly parametrized info can be provided in the chain software upgrade proposal's upgrade info for any chain that has different configuration.

### Documentation Considerations
Need to communicate to validators that the pattern of upgrade names has changed.

### Testing Considerations
A3P and manually tested on mainfork

### Upgrade Considerations
See above
mhofman added a commit that referenced this pull request Jun 26, 2024
Rebase todo:

```
# Branch fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487-
label base-fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487-
pick bcecf52 test(vow): add test of more vow upgrade scenarios
pick d7135b2 test: switch vow test to run under xs for metering
pick 99fccca test(vow): add test for resolving vow to external promise
pick 6d3f88c test(vow): add test for vow based infinite vat ping pong
pick c78ff0e test(vow): check vow consumers for busy loops or hangs
pick 3c63cba fix(vow): prevent loops and hangs from watching promises
pick 188c810 chore(vat-data): remove the deprecated `@agoric/vat-data/vow.js`
pick 44a6d16 fix(vow): allow resolving vow to external promise
label fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487-
reset base-fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487-
merge -C 4fca040 fix-vow-make-watch-when-more-robust-against-loops-and-hangs-9487- # fix(vow): make watch/when more robust against loops and hangs (#9487)

# Branch ci-mergify-strip-merge-commit-HTML-comments-9499-
label base-ci-mergify-strip-merge-commit-HTML-comments-9499-
pick 63e21ab ci(mergify): strip merge commit HTML comments
label ci-mergify-strip-merge-commit-HTML-comments-9499-
reset base-ci-mergify-strip-merge-commit-HTML-comments-9499-
merge -C 7b93671 ci-mergify-strip-merge-commit-HTML-comments-9499- # ci(mergify): strip merge commit HTML comments (#9499)

# Pull request #9506
pick 249a5d4 fix(SwingSet): Undo deviceTools behavioral change from #9153 (#9506)

# Pull request #9507
pick a19a964 fix(liveslots): promise watcher to cause unhandled rejection if no handler (#9507)

# Branch feat-make-vat-localchain-resumable-9488-
label base-feat-make-vat-localchain-resumable-9488-
pick 76c17c6 feat: make vat-localchain resumable
pick 40ccba1 fix(vow): correct the typing of `unwrap`
pick 90e062c fix(localchain): work around TypeScript mapped tuple bug
pick 3027adf fix(network): use new `ERef` and `FarRef`
label feat-make-vat-localchain-resumable-9488-
reset base-feat-make-vat-localchain-resumable-9488-
merge -C 5856dc0 feat-make-vat-localchain-resumable-9488- # feat: make vat-localchain resumable (#9488)

# Branch ci-mergify-clarify-queue-conditions-and-merge-conditions-9510-
label base-ci-mergify-clarify-queue-conditions-and-merge-conditions-9510-
pick 7247bd9 ci(mergify): clarify `queue_conditions` and `merge_conditions`
label ci-mergify-clarify-queue-conditions-and-merge-conditions-9510-
reset base-ci-mergify-clarify-queue-conditions-and-merge-conditions-9510-
merge -C 30e56ae ci-mergify-clarify-queue-conditions-and-merge-conditions-9510- # ci(mergify): clarify `queue_conditions` and `merge_conditions` (#9510)

# Branch fix-liveslots-cache-delete-does-not-return-a-useful-value-9509-
label base-fix-liveslots-cache-delete-does-not-return-a-useful-value-9509-
pick 42ea8a3 fix(liveslots): cache.delete() does not return a useful value
label fix-liveslots-cache-delete-does-not-return-a-useful-value-9509-
reset base-fix-liveslots-cache-delete-does-not-return-a-useful-value-9509-
merge -C a2e54e1 fix-liveslots-cache-delete-does-not-return-a-useful-value-9509- # fix(liveslots): cache.delete() does not return a useful value (#9509)

# Branch chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694-
label base-chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694-
pick 9930bd3 chore(swingset): re-enable test of unrecognizable orphan cleanup
label chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694-
reset base-chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694-
merge -C bc53ef7 chore-swingset-re-enable-test-of-unrecognizable-orphan-cleanup-8694- # chore(swingset): re-enable test of unrecognizable orphan cleanup (#8694)

# Pull request #9508
pick 513adc9 refactor(internal): move async helpers using AggregateError to node (#9508)

# Branch report-bundle-sizing-in-agoric-run-9503-
label base-report-bundle-sizing-in-agoric-run-9503-
pick 68af59c refactor: inline addRunOptions
pick a0115ed feat: writeCoreEval returns plan
pick bd0edcb feat: stat-bundle and stat-plan scripts
pick 0405202 feat: agoric run --verbose
pick 22b43da feat(stat-bundle): show CLI to explode the bundle
label report-bundle-sizing-in-agoric-run-9503-
reset base-report-bundle-sizing-in-agoric-run-9503-
merge -C 7b30169 report-bundle-sizing-in-agoric-run-9503- # report bundle sizing in agoric run (#9503)

# Branch ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511-
label base-ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511-
pick 5f3c1d1 test(boot): use a single bundle directory for all tests
pick 50229bd ci(all-packages): split tests according to AVA recipe
label ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511-
reset base-ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511-
merge -C 5375019 ci-test-boot-split-up-test-jobs-via-AVA-recipe-9511- # ci(test-boot): split up test jobs via AVA recipe (#9511)

# Pull request #9514
pick f908f89 fix: endow with original unstructured `assert` (#9514)

# Pull request #9535
pick 989aa19 fix(swingset): log vat termination and upgrade better (#9535)

# Branch types-zoe-setTestJig-param-type-optional-9533-
label base-types-zoe-setTestJig-param-type-optional-9533-
pick 426a3be types(zoe): setTestJig param type optional
label types-zoe-setTestJig-param-type-optional-9533-
reset base-types-zoe-setTestJig-param-type-optional-9533-
merge -C bf9f03b types-zoe-setTestJig-param-type-optional-9533- # types(zoe): setTestJig param type optional (#9533)

# Branch adopt-TypeScript-5-5-9476-
label base-adopt-TypeScript-5-5-9476-
pick 381b6a8 chore(deps): bump Typescript to 5.5 release
label adopt-TypeScript-5-5-9476-
reset base-adopt-TypeScript-5-5-9476-
merge -C 0cfea88 adopt-TypeScript-5-5-9476- # adopt TypeScript 5.5 (#9476)

# Branch retry-flaky-agoric-cli-integration-test-9550-
label base-retry-flaky-agoric-cli-integration-test-9550-
pick 2a68510 ci: retry agoric-cli integration test
label retry-flaky-agoric-cli-integration-test-9550-
reset base-retry-flaky-agoric-cli-integration-test-9550-
merge -C c5c52ec retry-flaky-agoric-cli-integration-test-9550- # retry flaky agoric-cli integration test (#9550)

# Pull Request #9556
pick 0af876f fix(vow): watcher args instead of context (#9556)

# Pull Request #9561
pick a4f86eb fix(vow): handle resolution loops in vows (#9561)

# Branch Restore-a3p-tests-9557-
label base-Restore-a3p-tests-9557-
pick d36382d chore(a3p): restore localchain test
pick 5ff628e Revert "test: drop or clean-up old Tests"
pick b5cf8bd fix(localchain): `callWhen`s return `PromiseVow`
label Restore-a3p-tests-9557-
reset base-Restore-a3p-tests-9557-
merge -C c65915e Restore-a3p-tests-9557- # Restore a3p tests (#9557)

# Pull Request #9559
pick 6073b2b fix(agoric): convey tx opts to `agoric wallet` and subcommands (#9559)

# Branch explicit-heapVowTools-9548-
label base-explicit-heapVowTools-9548-
pick 100de68 feat!: export heapVowTools
pick 8cb1ee7 refactor: use heapVowTools import
pick 0ac6774 docs: vow vat utils
pick 9128f27 feat: export heapVowE
pick 3b0c8c1 refactor: use heapVowE
pick 9b84bfa BREAKING CHANGE: drop V export
pick 6623af5 chore(types): concessions to prepack
label explicit-heapVowTools-9548-
reset base-explicit-heapVowTools-9548-
merge -C 4440ce1 explicit-heapVowTools-9548- # explicit heapVowTools (#9548)

# Pull Request #9564
pick 44926a7 fix(bn-patch): fix bad html evasion (#9564)

# Branch Fix-upgrade-behaviors-9526-
label base-Fix-upgrade-behaviors-9526-
pick ef1e0a2 feat: Upgrade Zoe
pick e4cc97c Revert "fix(a3p-integration): workaround zoe issues"
pick 84dd229 feat: repair KREAd contract on zoe upgrade
pick cb77160 test: validate KREAd character purchase
pick e1d961e test: move vault upgrade from test to use phase (#9536)
label Fix-upgrade-behaviors-9526-
reset base-Fix-upgrade-behaviors-9526-
merge -C 8d05faf Fix-upgrade-behaviors-9526- # Fix upgrade behaviors (#9526)

# Branch Support-for-snapshots-export-command-9563-
label base-Support-for-snapshots-export-command-9563-
pick 2a3976e refactor(cosmos): use DefaultBaseappOptions for newApp
pick 84208e9 fix(cosmos): use dedicated dedicate app creator for non start commands
pick 8c1a62d chore(cosmos): refactor cosmos command extension
pick 4386f8e feat(cosmos): support snapshot export
pick 2dabb52 test(a3p): add test for snapshots export and restore
label Support-for-snapshots-export-command-9563-
reset base-Support-for-snapshots-export-command-9563-
merge -C 309c7e1 Support-for-snapshots-export-command-9563- # Support for `snapshots export` command (#9563)

# Branch Swing-store-export-data-outside-of-genesis-file-9549-
label base-Swing-store-export-data-outside-of-genesis-file-9549-
pick f1eacbe fix(x/swingset): handle defer errors on export write
pick 496a430 feat(cosmos): add hooking kv reader
pick f476bd5 feat(cosmos): separate swing-store export data from genesis file
pick 17a5374 test(a3p): add genesis fork acceptance test
label Swing-store-export-data-outside-of-genesis-file-9549-
reset base-Swing-store-export-data-outside-of-genesis-file-9549-
merge -C 3aa5d66 Swing-store-export-data-outside-of-genesis-file-9549- # Swing-store export data outside of genesis file (#9549)

# Branch Remove-scaled-price-authority-upgrade-9585-
label base-Remove-scaled-price-authority-upgrade-9585-
pick bce49e3 test: add test during upgradeVaults; vaults detect new prices
pick 88f6500 test: repair test by dropping upgrade of scaledPriceAuthorities
label Remove-scaled-price-authority-upgrade-9585-
reset base-Remove-scaled-price-authority-upgrade-9585-
merge -C 8376991 Remove-scaled-price-authority-upgrade-9585- # Remove scaled price authority upgrade (#9585)

# Branch feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575-
label base-feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575-
pick 95174a2 feat(builders): non-ambient `strictPriceFeedProposalBuilder` in `priceFeedSupport.js`
pick 5cc190d feat(app): establish mechanism for adding core proposals by `upgradePlan.name`
pick 52f02b7 fix(builders): use proper `oracleBrand` subkey case
pick ddc072d chore(cosmos): extract `app/upgrade.go`
pick b3182a4 chore: fix error handling of upgrade vaults proposal
pick ea568a2 fix: retry upgrade vaults price quote
label feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575-
reset base-feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575-
merge -C cbe061c feat-make-software-upgrade-coreProposals-conditional-on-upgrade-plan-name-9575- # feat: make software upgrade `coreProposals` conditional on upgrade plan name (#9575)
```

This is followed by a commit to remove the `orchestration` and
`async-flow` packages from the release, as these are not baked in yet
and are not deployed anyway.
@mhofman mhofman mentioned this pull request Jul 12, 2024
Comment on lines +155 to +175
const getQuote = async lastRejectionReason => {
await null;
try {
return await E(priceAuthority).quoteGiven(
AmountMath.make(brand, 10n),
istBrand,
);
} catch (reason) {
if (
isUpgradeDisconnection(reason) &&
(!lastRejectionReason ||
reason.incarnationNumber >
lastRejectionReason.incarnationNumber)
) {
return getQuote(reason);
}
throw reason;
}
};
return getQuote(null);
}),
Copy link
Collaborator

@anilhelvaci anilhelvaci Jul 19, 2024

Choose a reason for hiding this comment

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

What is the reason for waiting for a quote when upgrading vaultFactory? @mhofman @Chris-Hibbert

My guess is that in vaultDirector's makePriceLockWaker;

makePriceLockWaker() {
return makeWaker('priceLockWaker', () => {
allManagersDo(vm => vm.lockOraclePrices());
});

allManagersDo iterates over all vault managers to invoke their lockOraclePrices sequentially. If there's a case such as no price updates are pushed for a certain vault manager when priceLockWaker is awake, then lockOraclePrices for that certain vault manager will fail. Which will result with vault managers added later than the failing one will not be able to lock their prices because allManagersDo invokes lockOraclePrices sequentially, see:

const allManagersDo = fn => {
for (const managerIndex of collateralManagers.values()) {
const vm = vaultManagers.get(managerIndex).self;
fn(vm);
}
};

I have already opened an issue about this, please see #9706. In the issue I have a possible fix which worked for me quite nicely.

However, I am very curios to find out what other reasons there may be for waiting a price feed update before upgrading vaultFactory?

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for waiting for a quote when upgrading vaultFactory? @mhofman @Chris-Hibbert

When we run the coreEval to do the upgrade, we'll be upgrading vaultFactory, and replacing auctions, as well as replacing all the priceAuthorities. After replacing the priceAuthorities, the oracle operators have to manually intervene in order to accept new invitations, and do some reconfiguration in order to start feeding information to the new priceAuthorities. I don't have any indication of whether that will take a half hour or a couple of days.

During the interim, it's best that the vaults and auctions continue to listen to the old priceAuthorities. The indication that the switchover is complete is that any price starts coming via the new priceAuthorities. At that point, we upgrade the vaultFactory, which will then start paying attention to the new priceAuthorities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Chris!

mergify bot added a commit that referenced this pull request Aug 31, 2024
closes: #9682

## Description
Ports the change from #9683 to master, adding some extra checks since master has a more complex upgrade name structure.
Update some docs following the refactor in #9575

### Security Considerations
None

### Scaling Considerations
None

### Documentation Considerations
None

### Testing Considerations
This PR originally added an a3p-integration test, but it cannot work without modification to the agd binary just for the purpose of testing this (if the original binary already contains support for the upgrade name, it attempts an upgrade in place, which fails as we don't intend to support this)

### Upgrade Considerations
Ensure we have the ability to release and apply multiple release candidates for the same version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MainNet proposals should dynamically provide oracle addresses
4 participants