-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Feature] Support program_owner
as an operand.
#2717
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
base: feat/program-updatability
Are you sure you want to change the base?
Conversation
@@ -91,7 +91,7 @@ impl<N: Network, C: ConsensusStorage<N>> VM<N, C> { | |||
|
|||
// Compute the Merkle root of the transaction. | |||
// Debug-mode only, as the `Transaction` constructor recomputes the transaction ID at initialization. | |||
#[cfg(debug_assertions)] | |||
// Attention - This check is mandatory. This is the only way to ensure that the transaction ID is well-formed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier reviews had concerns with keeping and removing this check.
For keeping. Transaction contents can be modified, which would result in an incorrect transaction ID.
Against keeping. The constructors from_deployment
and from_execution
recompute the transaction ID at initialization, which involves computing the root of the transaction tree. We know from manual code review that validators use those constructors. Removing this check was needed to scale up the validator set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On looking through the code again, I could not find any constructors that did not recompute the root. If that's the case, then an ID for a Transaction
in memory is correct-by-construction for an honest validator.
@reviewers do you agree?
Tangent. I noticed that get_transaction
on the Ledger performs this check, making it a pretty expensive operation to do, especially for our REST API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for the public record, I agree, but you knew :)
[NO NEW COMMITS ARE ALLOWED AS OF 5/27/25]
This PR:
Address
to theDeployment
and enforces that it exists and is well-formed after the migration height.Operand::ProgramOwner
to programs.partially_verified_transactions
to handle upgrades.Operand::ProgramOwner
program_owner
andcredits.aleo/program_owner
respectively.ConsensusVersion::V5
do not have a program owner, includingcredits.aleo
.Open Items
check_transaction
.program.owner
orthis.owner
or enforcing<this_program.aleo>/owner
to unify operands over program metadata.