Expand Deploy Acceptor #2081
Expand Deploy Acceptor #2081
Conversation
Co-authored-by: Ed Hastings <ed@casperlabs.io>
|
Hey @darthsiroftardis, could you include in the description of the PR exactly how the checks were expanded? i.e. what was added/changed/removed. |
|
@goral09 Yes, I am working on polishing the PR before I mark it ready, at that point I will amend the PR description |
In the spirit of doing only the minimal set of required checks prior to accepting a deploy, I think we shouldn't be validating session code for correctness. Once we know that the deploy will pay for the execution, we should accept any deploy even incorrect. |
| ) -> Self { | ||
| let session = ExecutableDeployItem::StoredVersionedContractByName { | ||
| name: "Test".to_string(), | ||
| version: Some(6u32), |
There was a problem hiding this comment.
version in session is Some, yet the function name says with_missing_contract_version_in_session..., maybe it's just a naming issue.
There was a problem hiding this comment.
Naming issue, yes, its meant to be Some version that doesn't exist in a contract package
There was a problem hiding this comment.
Renamed the tests around it
| @@ -707,7 +711,7 @@ impl Deploy { | |||
| /// * the body hash is correct (should be the hash of the body), and | |||
| /// * approvals are non empty, and | |||
| /// * all approvals are valid signatures of the deploy hash | |||
| pub fn is_valid(&mut self) -> Result<(), DeployValidationFailure> { | |||
| pub fn is_valid(&mut self) -> Result<(), DeployConfigurationFailure> { | |||
There was a problem hiding this comment.
Why did we change the name? I think the Validation was more fitting.
There was a problem hiding this comment.
We changed it cause a lot of these errors raised when a Deploy is checked against a specified DeployConfig , which is why it felt more apropos to rename it ConfigurationFailure
There was a problem hiding this comment.
My understanding is that we're validating against config so it fails during validation phase.
There was a problem hiding this comment.
All of the effect_builder.immediately().event(...) constructs in deploy_acceptor.rs can be replaced with calls to the respective methods. It's not going to be bad for performance as it's we were just continuing a synchronous method call. Performance-wise it will actually be better as we won't be creating additional messages that have to be routed through the reactor.
| } | ||
| } | ||
|
|
||
| fn verify_session_logic<REv: ReactorEventT>( |
There was a problem hiding this comment.
I don't think we should be verifying the session logic at all. If the deploy can be paid for we shouldn't really care whether deploy's creator made sure it can also be executed.
Applied suggestion and removed most of the |
| verification_start_timestamp: Timestamp, | ||
| ) -> Effects<Event> { | ||
| let mut cloned_deploy = event_metadata.deploy().clone(); | ||
| if let Err(deploy_configuration_failure) = cloned_deploy.is_valid() { |
There was a problem hiding this comment.
I am not sure if everything that happens inside the is_valid method is actually more computationally expensive than all of this back-and-forth between deploy_acceptor, storage and EE. It's only the signature verification that could be expensive and I still wouldn't be sure that's true - if there's a single signature to verify (which is probably 99% of cases) it will be faster than creating dozens of events, calling various components etc.
|
bors try |
tryBuild succeeded: |
# Conflicts: # execution_engine/src/core/engine_state/executable_deploy_item.rs
|
bors r+ |
|
Build succeeded: |
Co-authored-by: Ed Hastings ed@casperlabs.io
CHANGELOG:
DeployAcceptormakes on a singleDeployAccountHashesassociated with theDeployare valid associated keysWeightof theAccountHashesmeet the threshold to perform a deploymentAccountin the case of aDeployreceived by from aClientis sufficientDeployto be the last step once all other prerequisites are met.Closes #2085