Preflight invalid program upgrades#29505
Conversation
Kuhai9801
force-pushed
the
upgrade-invalid-preflight
branch
2 times, most recently
from
7da8c95 to
cefab61
Compare
Kuhai9801
force-pushed
the
upgrade-invalid-preflight
branch
from
cefab61 to
3757b06
Compare
| let mut remote_programs = Vec::new(); | ||
|
|
||
| for Task { id, program, is_local, .. } in tasks { | ||
| if !is_local || skipped.contains(id) { |
There was a problem hiding this comment.
The warnings loop still gates on !command.action.broadcast, but this preflight runs for every upgrade mode. Should a non-broadcast (save-to-file / offline) upgrade hard-fail here too, or only the broadcast path?
| } | ||
|
|
||
| let remote_program = | ||
| fetch_program_from_network(&id.to_string(), endpoint, network, command.env_override.network_retries) |
There was a problem hiding this comment.
Previously a failed fetch was just a warning (does not exist on the network), which also covered transient network errors and not-yet-deployed programs. Here it hard-aborts the command before the plan. Is that intended, or should a fetch failure stay a warning and reserve the hard error for a proven-invalid upgrade?
| consensus_version: ConsensusVersion, | ||
| command: &LeoUpgrade, | ||
| ) -> Result<Vec<(ProgramID<N>, Program<N>)>> { | ||
| let mut remote_programs = Vec::new(); |
There was a problem hiding this comment.
Maybe Vec::with_capacity(tasks.len()) here since the size is bounded by the task count.
| get_endpoint_consensus_version(&endpoint, network, command.env_override.network_retries) | ||
| .map_or(consensus_version, |network_version| consensus_version.max(network_version)); | ||
| let remote_programs = | ||
| validate_upgrade_tasks(&endpoint, network, &local, &skipped, validation_consensus_version, command)?; |
There was a problem hiding this comment.
This hard check uses max(local, network) for the consensus version while check_tasks_for_warnings uses the plain local consensus_version. Is the asymmetry intentional (fail-closed)? A one-line comment on the rationale would help.
|
Thanks, agreed on fetch failures. I updated this so fetch/parse failures stay in the warning path; only a fetched program that is proven invalid fails early. Also added the capacity hint and consensus-version comment. For non-broadcast, do you prefer proven-invalid upgrades to fail for |
|
Non-broadcast: fail in all modes. Proven-invalid depends only on the program pair, not on how the tx leaves the CLI, so a saved/printed tx will just get rejected on broadcast later. Keep the Also, the new consensus-version comment explains the Rest looks good. |
2 participants
Motivation
Fixes #29040.
leo upgradealready checks whether a local program is a valid upgrade, but that result is only surfaced as a warning. For upgrades that can be proven invalid before transaction generation, the CLI should stop before showing the deployment plan and confirmation prompt.This adds a hard preflight for local, non-skipped upgrade targets and catches V10+ record output register changes with the same snarkVM helper used by verification.
Test Plan
Added regression coverage for rejecting an upgrade that changes a record output register and accepting a logic-only change that preserves the record output register.
Related PRs
N/A