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.

Sign up for GitHub

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

Jump to bottom

Enable contract optimization #1496

Merged
merged 2 commits into from
Jul 29, 2021
Merged

Enable contract optimization #1496

merged 2 commits into from
Jul 29, 2021

Conversation

palango
Copy link
Contributor

@palango palango commented Jul 29, 2021 •
edited
Loading

What this PR does

Enable solidity optimizations

Why I'm making this PR

This lowers the contract code size dramatically (-34% for TokenNetworkRegistry), making it possible to add error strings (#81)

It also might make #1435 unnecessary.

What's tricky about this PR (if any)

Historically we don't use the optimizer, but I cannot find any reference for why. I also don't find any arguments that speak against using it.

Any reviewer can check these:

  • If the PR is fixing a bug or adding a feature, add an entry to CHANGELOG.md.
  • If the PR changed a Solidity source, run make compile_contracts and add the resulting raiden_contracts/data/contracts.json in the PR.
  • If the PR is changing documentation only, add [skip ci] in the commit message so Travis does not waste time.
    • But, if the PR changes comments in a Solidity source, do not add [skip ci] and let Travis check the hash of the source.
  • In Python, use keyword arguments
  • Squash unnecessary commits
  • Comment commits
  • Follow naming conventions
    • solidityFunction
    • _solidity_argument
    • solidity_variable
    • python_variable
    • PYTHON_CONSTANT
  • Follow the Signature Convention in CONTRIBUTING.md
  • For each new contract
    • The deployment script deploys the new contract.
    • etherscan_verify.py runs on the new contract.
  • Bookkeep
    • The gas cost of new functions are stored in gas.json.
  • Solidity specific conventions
    • Document arguments of functions in natspec
    • Care reentrancy problems
    • if the PR adds or removes require() or assert()
      • add an entry in Changelog
      • open an issue in the client, light client, service repos so the change is reflected there
      • Just adding a message in require() doesn't require these steps.
  • When you catch a require() failure in Solidity, look for a specific error message like pytest.raises(TransactionFailed, match="error message"):

And before "merge" all checkboxes have to be checked. If you find redundant points, remove them.

Measurements

The first numbers behind the contract names are the bytecode sizes.

1 optimizer run

(venv) ➜  raiden-contracts git:(optimize-contracts) ✗ make compile_contracts && make update_gas_costs
python setup.py compile_contracts
running compile_contracts
TokenNetworkUtils 892
TokenNetwork 12438
TokenNetworkRegistry 14084
TokenNetworkInternalStorageTest 13189
TokenNetworkSignatureTest 12928
TokenNetworkUtilsTest 13084
pytest "raiden_contracts/tests/test_print_gas.py::test_print_gas" -s
=================================================================== test session starts ====================================================================
platform darwin -- Python 3.7.11, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /Users/paul/Projects/brainbot/raiden-contracts, configfile: pytest.ini
plugins: cov-2.12.1, web3-5.21.0, requests-mock-1.9.3, xdist-2.3.0, pyfakefs-4.5.0, forked-1.3.0
collected 1 item

raiden_contracts/tests/test_print_gas.py TokenNetworkUtils 892
TokenNetwork 12438
TokenNetworkRegistry 14084
TokenNetworkInternalStorageTest 13189
TokenNetworkSignatureTest 12928
TokenNetworkUtilsTest 13084
----------------------------------
GAS USED TokenNetwork.openChannel 130432
----------------------------------
----------------------------------
GAS USED TokenNetwork.openChannel 113332
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 73257
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 56156
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalWithdraw 117360
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46177
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46177
----------------------------------
----------------------------------
GAS USED TokenNetwork.closeChannel 121169
----------------------------------
----------------------------------
GAS USED TokenNetwork.updateNonClosingBalanceProof 93569
----------------------------------
----------------------------------
GAS USED TokenNetwork.settleChannel 111302
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 1 locks 41920
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 6 locks 56525
----------------------------------

200 optimizer runs

(venv) ➜  raiden-contracts git:(optimize-contracts) ✗ make compile_contracts && make update_gas_costs
python setup.py compile_contracts
running compile_contracts
TokenNetworkUtils 898
TokenNetwork 12591
TokenNetworkRegistry 14253
TokenNetworkInternalStorageTest 13342
TokenNetworkSignatureTest 13071
TokenNetworkUtilsTest 13239
pytest "raiden_contracts/tests/test_print_gas.py::test_print_gas" -s
=================================================================== test session starts ====================================================================
platform darwin -- Python 3.7.11, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /Users/paul/Projects/brainbot/raiden-contracts, configfile: pytest.ini
plugins: cov-2.12.1, web3-5.21.0, requests-mock-1.9.3, xdist-2.3.0, pyfakefs-4.5.0, forked-1.3.0
collected 1 item

raiden_contracts/tests/test_print_gas.py TokenNetworkUtils 898
TokenNetwork 12591
TokenNetworkRegistry 14253
TokenNetworkInternalStorageTest 13342
TokenNetworkSignatureTest 13071
TokenNetworkUtilsTest 13239
----------------------------------
GAS USED TokenNetwork.openChannel 130357
----------------------------------
----------------------------------
GAS USED TokenNetwork.openChannel 113258
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 72862
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 55763
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalWithdraw 116669
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46177
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46177
----------------------------------
----------------------------------
GAS USED TokenNetwork.closeChannel 120816
----------------------------------
----------------------------------
GAS USED TokenNetwork.updateNonClosingBalanceProof 92938
----------------------------------
----------------------------------
GAS USED TokenNetwork.settleChannel 110960
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 1 locks 41687
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 6 locks 56053
----------------------------------

10000 optimizer runs

(venv) ➜  raiden-contracts git:(optimize-contracts) ✗ make compile_contracts && make update_gas_costs
python setup.py compile_contracts
running compile_contracts
TokenNetworkUtils 970
TokenNetwork 13575
TokenNetworkRegistry 15517
TokenNetworkInternalStorageTest 14390
TokenNetworkSignatureTest 14100
TokenNetworkUtilsTest 14383
pytest "raiden_contracts/tests/test_print_gas.py::test_print_gas" -s
=================================================================== test session starts ====================================================================
platform darwin -- Python 3.7.11, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /Users/paul/Projects/brainbot/raiden-contracts, configfile: pytest.ini
plugins: cov-2.12.1, web3-5.21.0, requests-mock-1.9.3, xdist-2.3.0, pyfakefs-4.5.0, forked-1.3.0
collected 1 item

raiden_contracts/tests/test_print_gas.py TokenNetworkUtils 970
TokenNetwork 13575
TokenNetworkRegistry 15517
TokenNetworkInternalStorageTest 14390
TokenNetworkSignatureTest 14100
TokenNetworkUtilsTest 14383
----------------------------------
GAS USED TokenNetwork.openChannel 130315
----------------------------------
----------------------------------
GAS USED TokenNetwork.openChannel 113204
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 72745
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalDeposit 55646
----------------------------------
----------------------------------
GAS USED TokenNetwork.setTotalWithdraw 116571
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46171
----------------------------------
----------------------------------
GAS USED SecretRegistry.registerSecret 46171
----------------------------------
----------------------------------
GAS USED TokenNetwork.closeChannel 120782
----------------------------------
----------------------------------
GAS USED TokenNetwork.updateNonClosingBalanceProof 92934
----------------------------------
----------------------------------
GAS USED TokenNetwork.settleChannel 110929
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 1 locks 41640
----------------------------------
----------------------------------
GAS USED TokenNetwork.unlock 6 locks 55912
----------------------------------

@auto-assign auto-assign bot requested a review from konradkonrad July 29, 2021 09:58
@palango palango requested review from karlb and istankovic and removed request for konradkonrad July 29, 2021 09:58
@codecov
Copy link

codecov bot commented Jul 29, 2021 •
edited
Loading

Codecov Report

Merging #1496 (545c875) into master (8291b19) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1496   +/-   ##
=======================================
  Coverage   80.85%   80.85%           
=======================================
  Files          22       22           
  Lines        1567     1567           
  Branches      190      190           
=======================================
  Hits         1267     1267           
  Misses        253      253           
  Partials       47       47
Impacted Files Coverage Δ
raiden_contracts/contract_source_manager.py 94.38% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8291b19...545c875. Read the comment docs.

@palango
Enable solidity optimization
6cfb5f1 
This improves contract code size by 34% and also slightly lowers gas
costs.

We default to 200 runs, as this is the solidity default value. This
seems to be a good compromis between code size and gas costs.
@palango
chore: Compile contracts & gas measurements
545c875
istankovic
istankovic approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@istankovic istankovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice savings!

@karlb
Copy link
Contributor

karlb commented Jul 29, 2021

Awesome! Should we try a deploy + verify once before merging this?

@palango
Copy link
Contributor Author

palango commented Jul 29, 2021

Awesome! Should we try a deploy + verify once before merging this?

verify doesn't currently work (see #1434), so let's merge and then check verification later.

@palango palango mentioned this pull request Jul 29, 2021
Merged
19 tasks
@palango palango merged commit 742ece6 into raiden-network:master Jul 29, 2021
@palango palango deleted the optimize-contracts branch July 29, 2021 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@istankovic istankovic istankovic approved these changes

@karlb karlb Awaiting requested review from karlb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@palango @karlb @istankovic