search

AdChain / AdChainRegistry

adChain registry smart contracts
Apache License 2.0
30 stars 10 forks source link

Reduce gas costs by drying the amount of sha3 invocations #24

Closed kangarang closed 7 years ago

kangarang commented 7 years ago

By reducing the number of times that the registry contract performs sha3, we can reduce the net gas usage of the contract's lifetime.

Gas costs to run all tests Previously: 68,252,306 After refactor: 67,862,086 Difference: 390,220 (0.57%)

The contract can be further refactored, extracting sha3 from apply, deposit,withdraw, exit, challenge, and updateStatus. This would however require events to emit domain hashes instead of (currently) domain strings.

kangarang commented 7 years ago

6762be4 is a logging snippet I used to get these numbers. I'm happy to drop that commit during rebase.

miguelmota commented 7 years ago

Cost savings are good but we do need the actual domain name to be emitted in the events, for database storage and querying. The domain name can be passed as a parameter along with the hashed domain but then that would require the contract to check if the domain matches that hash which invalidates the cost savings we we were aiming for in the first place.

@skmgoldin @kangarang

miguelmota commented 7 years ago

maybe we can have a hash table that maps domain hashes with their domain. The only method requiring the actual domain can be the apply method. In there, the domains hash table is populated with the domain hash and domain. The events can pull the actual domain from this map based on the hash.

kangarang commented 7 years ago

Hi @skmgoldin . Consulted @miguelmota on this issue today, and he brought up some excellent points.

If we pursue any further in reducing sha3 (i.e. forcing the frontend to submit both domain and domainHash), that would cause inconveniences such as:

  1. frontend needs to program and submit an extra argument
  2. contract needs to make sure that the submitted domainHash is correct
  3. events will emit hashes instead of domain strings..
  4. ..which means frontend needs to manage the correlations of each domain with its hash

I like miguel's advice - make it easy for the frontend. I do like the cost-savings, I don't like the hash event emissions (although this could be solved with more arguments), and I don't like the added having-to-make-sure the user submitted the right arguments.

As this PR stands, there are no hash event emissions and the only function that takes both domain and domainHash as arguments is resolveChallenge, which is private so it's a non-issue.

In terms of frontend refactoring, as this PR stands, the smart contract functions that would require sha3 arguments from the client are the getters - canBeWhitelisted, isWhitelisted, appExists, challengeExists, and challengeCanBeResolved.

skmgoldin commented 7 years ago

I appreciate your work and good thinking on this PR @kangarang, but I think we should drop it in light of the issues raised by Miguel.

skmgoldin commented 7 years ago

Could you put your code snippet for making testRPC log gas usage in a gist though? That's a good snippet.

kangarang commented 7 years ago

Sounds good, will do that @skmgoldin