Calculate gas for deployment transaction#9840
Conversation
grbIzl
added
A0-pleasereview 🤓
sorpaas
left a comment
sorpaas
left a comment
There was a problem hiding this comment.
Note that Client::estimate_gas would have to execute the transaction, so with this change we're probably executing private transactions twice, which can slow things down noticeably. On the other hand, having overestimated gas would probably do no harm if the contract is somewhat "trusted", or if we need a successful execution of the code anyway.
| .and_then(|h| h.decode().map_err(|_| ErrorKind::StateIncorrect).into())?; | ||
| let (executed_code, executed_state) = (executed.code.unwrap_or_default(), executed.state); | ||
| let tx_data = Self::generate_constructor(validators, executed_code.clone(), executed_state.clone()); | ||
| let gas = match self.client.estimate_gas(&Transaction{ |
There was a problem hiding this comment.
For the deployment transaction the speed is not so important as it's one-time action. At the same time overestimation for the gas can (and actually is, see the corresponding bug) cause problems as wrapper contract is pretty complex and stores private contracts inside. In case, when the private contract is big enough, this overestimation exceeds gas_limit.
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
I don't know a lot about private transactions to have any strong opinions regarding what @sorpaas mentioned but Client::estimate_gas may need to execute the transaction more than once (since we do a binary search for minimum gas required). If we want to stick with the naive estimate we could cap it to the maximum gas limit to fix #9708.
| gas_price: gas_price, | ||
| value: source.value, | ||
| data: tx_data.clone(), | ||
| }.fake_sign(sender), |
There was a problem hiding this comment.
Nit: I think indentation is a bit weird like this
|
@andresilva yes, usage of max_gas will address the issue, but IMHO such solution will damage "informativeness" of this API call (I mean, create deployment transaction method). Right now this call returns estimation for the gas and it makes a lot of sense since it may be totally unclear for the external user, how much gas it can take (because a lot of things hidden under the hood). At the same time, as I've already explained, this call is one-time action and will be done only once during system setup. So IMHO the performance is less important here, than the mentioned "informativeness" |
|
@grbIzl Sorry for not moving this PR forward, I pushed a commit to fix the style nit, approve and merge if you agree 👍. |
|
@grbIzl I still worry about use cases such as chains that may want to deploy new private transaction account for every user. In those cases we potentially can have a lot of deployments! I understand that probably no one uses it like that currently, but it indeed feels feasible to have this use cases in certain situations, and just want to make sure we don't shot ourselves in the foot!
|
sorpaas
left a comment
sorpaas
left a comment
There was a problem hiding this comment.
Discussed with @grbIzl. Looks like this change only affects compose_deployment_transaction RPC call. While not strictly required, most common use cases would need to estimateGas anyway. So the current solution might just be the best one!
5chdn
added
A8-looksgood 🦄
* Calculate gas for deployment transaction * Space fixed * ethcore: style fix in public_creation_transaction
4 participants
Gas for the deployment transaction for the private contract was overestimated. More accurate method with client.estimate_gas implemented.
Closes #9708