Process Upgrade transaction inside of the Interpreter
#712
Conversation
Reduced default `MAX_SIZE` to be 110kb. Reduced default `MAX_CONTRACT_SIZE` to be 100kb.
# Conflicts: # CHANGELOG.md # fuel-tx/src/builder.rs # fuel-tx/src/tests/valid_cases/transaction.rs # fuel-tx/src/transaction/consensus_parameters.rs # fuel-vm/src/checked_transaction.rs # fuel-vm/src/tests/limits.rs # fuel-vm/src/tests/validation.rs
…ed by all chargeable transactions
…ction # Conflicts: # fuel-tx/src/transaction/consensus_parameters.rs
# Conflicts: # fuel-tx/src/transaction/consensus_parameters.rs # fuel-vm/src/tests/validation.rs
…ansaction # Conflicts: # fuel-tx/src/transaction/types/script.rs # fuel-vm/src/interpreter.rs
…de-transaction-within-interpreter
…-interpreter # Conflicts: # CHANGELOG.md # fuel-tx/src/lib.rs # fuel-tx/src/transaction/types.rs
| debug_assert!( | ||
| metadata.is_some(), | ||
| "`upgrade_inner` is called without cached metadata" | ||
| ); |
There was a problem hiding this comment.
Can't we compute the metadata here?
There was a problem hiding this comment.
We can, but it was calculated before in the Checked transaction. It is just a debug assert for the future if someone decides to change the code or behavior later.
Recalculation of it breaks the idea of the metadata=D
| .map_err(RuntimeError::Storage)?; | ||
| let next_version = current_version | ||
| .checked_add(1) | ||
| .ok_or(InterpreterError::Panic(PanicReason::ArithmeticOverflow))?; |
There was a problem hiding this comment.
Introducing a new PanicReason for this would be nice. ArithmeticOverflow is meant to be used for arithmetic opcodes only.
| .map_err(RuntimeError::Storage)?; | ||
| let next_version = current_version | ||
| .checked_add(1) | ||
| .ok_or(InterpreterError::Panic(PanicReason::ArithmeticOverflow))?; |
fuel-vm/src/memory_client.rs
Outdated
| @@ -83,6 +84,11 @@ impl<Ecal: EcalHandler> MemoryClient<Ecal> { | |||
| self.transactor.deploy(tx).ok() | |||
| } | |||
|
|
|||
| /// Executes `Upgrade` transaction. | |||
| pub fn upgrade(&mut self, tx: Checked<Upgrade>) -> Option<Upgrade> { | |||
There was a problem hiding this comment.
Why are we erasing the error type here?
| } | ||
|
|
||
| #[test] | ||
| fn transact_fails_when_try_to_override_state_bytecode() { |
There was a problem hiding this comment.
This case is confusing me. Isn't the point of the upgrade to override some previous state?
Other than the first time it's run, under what circumstances will this not fail?
There was a problem hiding this comment.
Same question for the consensus params.
There was a problem hiding this comment.
Each version contains only one associated consensus parameter or state transition function. If it was set before, it is not allowed to be overridden for the same version. You can only increase the version and set it for a new one.
Co-authored-by: Mitchell Turner <james.mitchell.turner@gmail.com>
Co-authored-by: Mitchell Turner <james.mitchell.turner@gmail.com>
xgreenx
force-pushed
the
feature/process-upgrade-transaction-within-interpreter
branch
from
9f128fa
to
e871566
Compare
| ..Default::default() | ||
| }; | ||
|
|
||
| let ready_tx = TransactionBuilder::upgrade(UpgradePurpose::StateTransition { |
There was a problem hiding this comment.
Similar to the "Upload" PR, this could be moved to a helper to declutter this test.
| // when | ||
| let mut transactor = | ||
| Transactor::<_, Upgrade>::new(MemoryStorage::default(), interpreter_params); | ||
| let err = transactor.execute_ready_upgrade_tx(ready_tx).unwrap_err(); |
There was a problem hiding this comment.
This test isn't testing Upgrade it's testing execute_ready_upgrade_tx. I know this is what upgrade uses under the hood, but is that actually the public function we want to support?
| // It shouldn't be possible since `Check<Upgrade>` guarantees that. | ||
| Err(InterpreterError::CheckError(CheckError::Validity( | ||
| ValidityError::TransactionMetadataMismatch, |
There was a problem hiding this comment.
nit: Considering just using unreachable! here if this is actually impossible
xgreenx
deleted the
feature/process-upgrade-transaction-within-interpreter
branch
The
Interpretersupports the processing of theUpgradetransaction. The change affectsInterpreterStorage, adding 5 new methods that must be implemented.