feat(extract-proposal): organize proposals into steps

[michaelfig]

refs: #8219
related: #8441

Description

Given coreProposals: ParallelProposals | { steps: ParallelProposals[] } (with typedef ParallelProposals = Proposal[]), return codeSteps: string[] from extractCoreProposalBundles. Then execute each ParallelProposals in parallel, fully draining the kernel run queue before proceeding to the next step.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Would be useful to document the coreProposals mechanism and call out the meaning of .steps fields if present.

Testing Considerations

Tested with packages/cosmic-swingset/economy-template.json.

Upgrade Considerations

Makes coreProposals processing more flexible than simply evaluating all of them in parallel.

[mhofman]

I have only done a partial review, and have not yet reviewed the graph resolver itself.

Mostly nits, except for the context of separately injected core evals which needs to be different.

Potential for deadlock if core proposals have .needs that don't actually provide the functionality needed by the dependent to make progress.

This seems formulated backward. Did you mean that a dependent forgot to include some entry in its .needs, or did I misunderstand?

I'm also unclear about 2 things:

• Since we're going from an array to an object for the definitions, in what order are individual core evals executed if the proposals have no needs?
• How are cyclical needs resolved, if at all? I assume the proposals could be grouped together in the same eval?

At the end of the day I'm wondering if it might not be simpler to have core proposals expressed as an array of grouped proposals, and not try to do any automatic resolution.

[mhofman]

Small nit: this looks like a breaking change, which is only addressed in follow-up commits. My preference is usually to have all commits working. In this case I think it could have been done by switching to an array of steps first (aka the next commit), but containing only the current comingled proposals, then do the step extraction in this commit.

It does also raise the question if the extract proposal logic should actually support providing both?

[mhofman]

I think this nit still applies, relayering by switching to handling codeSteps first would IMO be preferable, but I understand it'd be a decent amount of work for no good reason.

That said, extract proposal should definitely not provide both forms, that comment was misguided.

[michaelfig]

Potential for deadlock if core proposals have .needs that don't actually provide the functionality needed by the dependent to make progress.

This seems formulated backward. Did you mean that a dependent forgot to include some entry in its .needs, or did I misunderstand?

I meant: if there's a declaration that "b needs a", but "a" doesn't actually do the work that "b" needs in order to progress, then that will deadlock things that legitimately need "b".

• Since we're going from an array to an object for the definitions, in what order are individual core evals executed if the proposals have no needs?

In parallel, as is the status quo. After this PR, every core proposal in a given step is grouped together for parallel evaluation.

• How are cyclical needs resolved, if at all?

Cycles cause the solver to throw instead of returning any steps.

I assume the proposals could be grouped together in the same eval?

Yes, just as they are today. If their needs are satisfied, they will run opportunistically in parallel with all such other proposals that were satisfied in the prior step.

At the end of the day I'm wondering if it might not be simpler to have core proposals expressed as an array of grouped proposals, and not try to do any automatic resolution.

Yes, that was one of my earlier suggestions. If we did this, I'd introduce a different property to distinguish the grouping-of-parallel (say, coreProposalSteps: CoreProposal[][]), rather than coreProposals parallel-only, or this PR's automatic-grouping-of-parallel. I was unwilling to break the other tooling around coreProposals, but that may be acceptable.

[mhofman]

At the end of the day I'm wondering if it might not be simpler to have core proposals expressed as an array of grouped proposals, and not try to do any automatic resolution.

Yes, that was one of my earlier suggestions. If we did this, I'd introduce a different property to distinguish the grouping-of-parallel (say, coreProposalSteps: CoreProposal[][]), rather than coreProposals parallel-only, or this PR's automatic-grouping-of-parallel. I was unwilling to break the other tooling around coreProposals, but that may be acceptable.

I'm not sure I follow. I think what I'm suggesting is to only change the template.json to be structured like:

type ProposalTemplate = Array<Record<string, { module: string, entrypoint?: string, args?: Array<unknown> }>>

With each template record resulting in a single core eval sequentially executed. The keys of the record would not have any actual use besides making it more explicit that each proposal entry in the record is parallel and not sequential.

I don't think we need to distinguish the only-parallel vs sequential-of-parallel because the former can be expressed as a single sequential step in the latter form, the same way the current PR only supports a single form.

[mhofman]

Thanks for simplifications in the extract logic from what I remember the last iteration was.

At this point I have 2 small concerns:

• Nit: I wish we didn't keep 2 paths for how core proposals may be executed at bootstrap.
• Because we don't yet have Report bridge inbound result to cosmic-swingset #8441, there is a risk that a core proposal step executed in bootstrap would behave differently than a core proposal step through the bridge for buggy core proposals that require future external input. The former would deadlock, highlighting the bug, whereas the latter would return to the host, and finish once that input arrives.

I need to think a little more about that last bit in the context of upgrades.

[mhofman]

I think this nit still applies, relayering by switching to handling codeSteps first would IMO be preferable, but I understand it'd be a decent amount of work for no good reason.

That said, extract proposal should definitely not provide both forms, that comment was misguided.

[mhofman]

I need to think a little more about that last bit in the context of upgrades.

@michaelfig reminds me that the difference goes in the other direction as well. An await of a step does not guarantee that the async side-effects of the step have gotten quiescent. Arguably that's also a bug of the steps themselves, as any errors would end up as some unhandled rejection somewhere, however that's a much harder one to detect. A series of core eval bridge messages would at least ensure these side effects would not overlap other proposal steps.

[michaelfig]

• Nit: I wish we didn't keep 2 paths for how core proposals may be executed at bootstrap.

So did I, until I discovered that eliminating the bootstrap vat path completely broke ag-solo.

there is a risk that a core proposal step executed in bootstrap would behave differently than a core proposal step through the bridge for buggy core proposals that require future external input. The former would deadlock, highlighting the bug, whereas the latter would return to the host, and finish once that input arrives.

That's one reason why I would recommend people test their core proposals by adding them to the chain config.json, which also happens to be the most expedient way to iterate on their deelopment until they work well enough to justify testing the cosgov way to propose and vote them in.

[mhofman]

I'm conditionally approving given the following:

• A comment is added to clarify in which case a coreProposal is executed directly as part of the bootstrap.
• Consider an assertion we don't have both a coreProposal from bootstrap config, and upgradePlan, at least until we decide how that situation should be resolved. Right now ignoring coreProposals from the bootstrap config seems wrong.

[michaelfig]

I'm conditionally approving given the following:

• A comment is added to clarify in which case a coreProposal is executed directly as part of the bootstrap.

👍

• Consider an assertion we don't have both a coreProposal from bootstrap config, and upgradePlan, at least until we decide how that situation should be resolved. Right now ignoring coreProposals from the bootstrap config seems wrong.

Let's follow up in: #8370 (comment)

[mhofman]

The core logic seems sane, just a couple nits.

However it does feel like we're bending backwards to recreate ENACTED_UPGRADE synthetic blocking sends, which golang no longer generates by itself anyway. @gibson042 and I were talking about possibly simplifying the logic there, which I suspect would allow us to remove some cruft here too.

[mhofman]

Also please manually rebase before merging (I know the linear history test should check, but I have seen it let things through)

[mhofman]

After discussing, we'll get rid of ENACTED_UPGRADE, and I'll re-review

[mhofman]

While functionally correct, I think we can move a few things around to make things more legible. Approving, but please consider the suggested fixes.

[mhofman]

Any reason to do the merge inside doCosmosInit instead of the caller?

[mhofman]

I think this should be named doBootstrap.

Cosmos init is currently overloaded to mean every start of the software.

[mhofman]

Let's move this inside the doCoreProposals.

[mhofman]

While correct, this condition is confusing me. Can we restore the original, and instead do the following below?

        if (coreProposals) {
          await doCoreProposals(action, coreProposals);
        }
        return true;

[mhofman]

Why not use the already plucked blockTime above?

[mhofman]

Oh also, can we remove this ActionType export altogether?