Skip to content
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

Valora Performance: Gas estimation logic #5432

Closed
jeanregisser opened this issue Oct 19, 2020 · 2 comments
Closed

Valora Performance: Gas estimation logic #5432

jeanregisser opened this issue Oct 19, 2020 · 2 comments

Comments

@jeanregisser
Copy link
Contributor

jeanregisser commented Oct 19, 2020

What's important about this?

Gas estimation logic is currently doing too many roundtrips and taking more time than necessary (~20secs).
Leading to a sub-optimal UX.

Definition of "done"

Gas estimation is as fast as it can be.

What's involved in doing this work?

  • Measure the difference before and after the changes.
  • Optimize current gas estimation logic

Open questions

  • ?
@jeanregisser
Copy link
Contributor Author

jeanregisser commented Nov 6, 2020

Hardcoded gas needed in the verification flow in #5679

@nategraf
Copy link
Contributor

nategraf commented Nov 6, 2020

After some initial investigation, my conclusion so far is that the largest impact to gas estimation performance is the time it takes to execute a single view call. I have not discovered any client-side changes, either in Geth or Valora, that would improve upon this portion of gas estimation.

As a result, the best improvement I can currently think of is simply to hard-code gas values wherever it is reasonable to avoid gas estimation. A major downside to this approach is that it is somewhat fragile to contract changes, and introduces risk of breaking during any contract upgrades, or hard-forks that effect gas costs.

For more substantial gains, we will need to work on adding new functionality the LES protocol, such as creating a request for remote evaluation of a function that returns all batched proofs needed to verify the result. This work could be completed in the medium to long term.

Additional context

Each gas estimation runs the transaction in its local EVM (in the same way as a view call). On the first execution of a method on a contract the client has not seem before, the client must request the contract code, then execute the code. Each time the client encounters a state variable, it must make a request to the full node for the state variable, and a Merkle proof attesting to the value. Because state variable accesses cannot be predicted ahead of time in general, each request must be made in series. As a result, the time to execute the transaction locally is dominated by the time it takes to make a number of network round-trips to the full-node peer. As an example, a simple cUSD transfer requires ~12 requests, meaning on a 200ms latency connection it can be evaluated in no less than 2.4 seconds. This value is higher for complicated transactions (e.g. select attestation issuers) and on higher latency networks, and networks with some packet loss, which severely impacts the user-perceived latency on TCP connections.

Gas estimation requires a binary search of the allowable gas values, from the block gas cap to the intrinsic gas cost of a transaction, which with a gas cap of 13M translates to ~24 executions. Each execution after the first is very likely (although not guaranteed) to reused the cached state variable values, and thus require no additional network calls. It does however require a non-trivial amount of computation. (e.g. on my laptop I observed ~2.5ms per execution of a cUSD transfer, which in total adds about 60ms to gas estimation time. On a phone this is obviously going to take longer) We do not currently have any data on how long this component of estimation takes, although if we start exporting Geth metrics we can collect that. My current hypothesis is that computation accounts for a relatively small portion of gas estimation time, but more work should be done to confirm or deny this. If it turns out computation is a significant portion of the estimation time, then we can employ some strategies such as lowering the gas ceiling for estimations (e.g. to 1M gas) and reducing the precision of the estimate (e.g. to 50k gas) which together would greater reduce the number of executions (e.g. to 5 instead of 24). Additionally, more work could be done to extract estimation information from the EVM such that a single execution would be enough to get an estimate under the assumption that neither gas value nor gas price effect the transaction flow, as long as they are sufficient.

@nityas nityas closed this as completed Dec 13, 2020
mergify bot pushed a commit to valora-inc/wallet that referenced this issue Mar 18, 2021
### Description

As described in celo-org/celo-monorepo#5432, gas estimation is a costly
operation for the mobile client. The most promising short-term solution is to simply hard-code the
gas value to be used in for simple transactions along common code paths. The most common transaction
sent by a typical user is to send cUSD, either by a direct transfer or an invite.

This PR adds the required plumbing to use a single gas value, price and currency throughout the send
and invite flows. As a result, a single constant gas value can be used from start to finish. The
major result of this is that instead of estimating gas online twice, as the app currently does, the
static value is used and online gas estimation is never used.

This represents a large improvement to the experienced transaction speed, especially in less than
ideal network conditions. Additional effects of this are that errors caused by gas estimation should
be reduced, it's possible to give a fee estimate while offline, and the fee estimate shown the user
will be respected as a maximum fee value.

### Other changes

* Remove the unused `EstimateFee.tsx`
* Refactor cancellation for `sendTransaction`
* Fix gas price cache to handle multiple currencies.
* Propagate receipts back up from `sendTransaction`
* Use a Lock for getting contractkit instance to avoid reliance on polling and flag-based locking.
* Don't clear the transaction feed when the call to the blockchain API fails, such as when the device is offline.
* Add `networkConnectedSelector` to access the network connectivity state reported by the device.

### Tested

* Updated snapshot and unit tests to cover new functionality
* Manually tests effected user flows: Sending cUSD, escrow, and reclaiming escow

### Related issues

- Related: celo-org/celo-monorepo#5432

### Backwards compatibility

No backwards compatibility concerns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants