[FastX adapter/verifier] Followup to the module initializers PR (#337) #436
Conversation
| @@ -198,6 +198,8 @@ impl Storage for AuthorityTemporaryStore { | |||
| } | |||
|
|
|||
| fn read_object(&self, id: &ObjectID) -> Option<Object> { | |||
| // there should be no read after delete | |||
| assert!(self.deleted.get(id) == None); | |||
There was a problem hiding this comment.
Let's use debug_assert for consistency.
There was a problem hiding this comment.
Got it! As I (think I) understand it now, assert! is only for tests.
There was a problem hiding this comment.
assert! is turned on in release build, while debug_assert! is only turned on in debug build.
| // (before the call) must be larger than gas_used (after | ||
| // the call) in order for the call to succeed in the first | ||
| // place. | ||
| assert!(current_gas_budget > gas_used); |
There was a problem hiding this comment.
| assert!(current_gas_budget > gas_used); | |
| assert!(current_gas_budget >= gas_used); |
There was a problem hiding this comment.
Done! Also changed to debug_assert!.
| total_gas_budget: u64, // gas budget for all operations in this transaction | ||
| current_gas_budget: u64, // gas budget for the current call operation | ||
| gas_previously_used: u64, |
There was a problem hiding this comment.
This seems really complicated. Not sure if I fully understand why we need all these three, but execute_internal shouldn't need to worry about the total budget or previously used gas. All it needs to know is the current remaining budget, and if fail, return the amount used (which will always be <= current budget). And the caller handle the final numbers
There was a problem hiding this comment.
I think that the logic is sound, and the additional parameters are used to create the correct error values in execute_internal. I agree that this is not easy to follow, though, and I will try to refactor it further to post-process failure results coming from execute_internal to reflect total gas values in its caller, rather than handling these directly in execute_internal and mostly propagating the errors up the call chain. This should hopefully simplify execute_internal and make the whole gas handling more intuitive.
There was a problem hiding this comment.
An attempt to clean this up is in. Hopefully it's easier to read (and makes some sense) - it's surprisingly subtle (particularly to uninitiated not used to thinking in these terms).
| @@ -44,12 +45,18 @@ use std::{ | |||
|
|
|||
| pub use move_vm_runtime::move_vm::MoveVM; | |||
|
|
|||
| macro_rules! exec_failure { | |||
| macro_rules! exec_error { | |||
There was a problem hiding this comment.
This should not be a macro. The intention of macro is to express something that could not be expressed through function calls (e.g. embedded a return like exec_failure). exec_error seems something that can be done with a simple function. I recommend adding a function to ExecutionStatus for this
There was a problem hiding this comment.
Changed! I really appreciate all the Rust-related suggestions as I am still learning what the best practices are - please keep them coming for as long as you have patience(in this case I simply thought it would be cleaner to use a macro within a macro rather than defining a new function)!
This is a followup to the #337 PR providing support for module initializers (Issue #90) with some additional checks and fixes, based on @lxfind's feedback.