[Feature] Updates to support program upgradability.

[d0cd]

For now, this will use the testing/program-updatability+staging branch of snarkVM until the PRs on snarkVM are ready.

This PR introduces updates needed to support program upgradability. Concretely:

• updates to the deployment cost to account for constructors.
• updates to the REST API to account for multiple program editions.

[vicsn]

UPDATE: a check in check_transaction should be sufficient, as this can be checked at the transaction level.

We should add a rule in both propose_batch and process_batch_propose_from_peer that before the new consensus version (still t.b.d. which one exactly), we do not create or sign a batch proposal if it contains a deployment transaction using novel components (checksum), novel syntax (constructor, Operand::checksum, Operand::edition), novel reserved keyword (constructor) (NOTE: this is t.b.d., see this discussion and this PR). This can happen in the same place where we check against the batch proposal limit, reinserting AND continuing if the check fails.

[vicsn]

Could you also try to implement the following in a unit test similar to [1] [2]: try to deploy an old and new type program, before and after the upgrade, with appropriate expectation when the respective deployments land in a proposal or block

[raychu86]

Is there a way to return the edition from the original get_program endpoint? If not, we may want to consider adding an optional Metadata to the query that surfaces it, and potentially other info like owner, tx_id, etc.

[d0cd]

I'm not sure you can without breaking the API.
The Metadata option seems reasonable.

[d0cd]

I added the option for metadata, returning the edition and TX ID.
I did not add ProgramOwner, since it is not used for anything.

[d0cd]

We should add a rule somewhere in process_batch_propose_from_peer that before the new consensus version (still t.b.d. which one exactly), we do not sign a validator's batch proposal if it contains a deployment transaction using:

* novel components (checksum)

* novel syntax (constructor, Operand::checksum, Operand::edition)

* novel reserved keyword (`constructor`)

This can happen in the same place where we check against the batch proposal limit, reinserting AND continuing if the check fails.

I took a stab at adding it in.
Do you feel like it's somewhat redundant given that this check is in VM::check_transaction?

[vicsn]

I took a stab at adding it in.

Awesome.

Do you feel like it's somewhat redundant given that this check is in VM::check_transaction?

Excellent point, you are right, especially because check_transaction currently does not implement all of the checks described above. I updated the description above to clarify what should happen. You can remove the checks from check_transaction, and instead have them live in snarkOS, you can look at the recent proposal spend limit PR for where in the code the checks can be added.

Additionally, primary.rs is getting pretty huge, so I think this would be a timely moment for a tiny refactor. Suggestion: move the 1. batch proposal limit checks and 2. the program upgradability checks into a new transmission_checks.rs or migration_checks.rs helper file.

[d0cd]

I took a stab at adding it in.

Awesome.

Do you feel like it's somewhat redundant given that this check is in VM::check_transaction?

Excellent point, you are right, especially because check_transaction currently does not implement all of the checks described above. I updated the description above to clarify what should happen. You can remove the checks from check_transaction, and instead have them live in snarkOS, you can look at the recent proposal spend limit PR for where in the code the checks can be added.

Additionally, primary.rs is getting pretty huge, so I think this would be a timely moment for a tiny refactor. Suggestion: move the 1. batch proposal limit checks and 2. the program upgradability checks into a new transmission_checks.rs or migration_checks.rs helper file.

A couple questions/points here:

• Is it safe to move the checks out from check_transaction. Is this the general pattern we are doing for migrations here on out?
• I refactored the implementation for readability, however, I think its more prudent to leave the larger refactor of primary.rs for another PR.
• check_transaction implements these checks as well.
• constructor has been removed as a keyword.

[vicsn]

I refactored the implementation for readability, however, I think its more prudent to leave the larger refactor of primary.rs for another PR.

Alright, it indeed looks more manageable right now, thank you.

Is it safe to move the checks out from check_transaction. Is this the general pattern we are doing for migrations here on out?

UPDATE: a check in snarkVM's check_transaction should be sufficient.

The key problem is that we don't call check_transaction on incoming batch proposals which we need to sign, so we need to create a new independent function to call for the checking logic. But you are right that it is conceptually easier if the checking logic lives in snarkVM. One approach to a cleaner refactor could be to create e.g. a fn check_transaction_migrations in snarkVM. Then we can call check_transaction_migrations from snarkVM::check_transaction, ensuring we don't include faulty transactions in our own batch proposal, and we can call check_transaction_migrations from snarkOS::process_batch_propose_from_peer, ensuring we don't sign faulty batch proposals

[vicsn]

Confirming after recent discussions: there is no need to add the transaction-level migration logic at the snarkOS level (because it would only offer a small performance benefit if we would avoid processing the transmissions from malicious validators, which is not a priority).

Consider closing this PR and opening a new one so future reviewers don't have to go through all the above comments. O:)

[vicsn]

@d0cd want to remove this checking logic at the snarkOS level - as we agreed snarkVM level is sufficient?

[d0cd]

Closing PR per the above recommendation to open a new one.