ethereum / beacon_chain

MIT License
209 stars 65 forks source link

Update pow chain deposit contract and tests #137

Closed NIC619 closed 5 years ago

NIC619 commented 5 years ago
NIC619 commented 5 years ago

@hwwhww

  1. Could you meter the gas cost of calling deposit()?

First deposit transaction takes around 980k gas while the subsequent ones take around 480k gas.

  1. More test cases: Try when CHAIN_START_FULL_DEPOSIT_THRESHOLD=16384: we might need to see the edge case? (<-- not quite sure)

modified_registration_contract fixture has CHAIN_START_FULL_DEPOSIT_THRESHOLD set to 5 and in test_chain_start test case it verifies that only with enough full_deposit_count will it trigger ChainStart event. My assumption was that from evm's point of view it does not matter if CHAIN_START_FULL_DEPOSIT_THRESHOLD is set to 16384 or 5(unless we are testing if the contract will break if it stores enormous amount of validator data?). But it may not be the case from the contract logic's point of view. Please let me know if that does not make sense or there are other edge cases that comes up your mind. Another problem with setting CHAIN_START_FULL_DEPOSIT_THRESHOLD too high is that currently it takes about 6 seconds to complete one deposit transaction in test.

NIC619 commented 5 years ago

@hwwhww @djrtwo Add TWO_TO_POWER_OF_TREE_DEPTH and use random CHAIN_START_FULL_DEPOSIT_THRESHOLD and random deposit amount in tests.

NIC619 commented 5 years ago

Update Vyper to v0.1.0-beta.6 according to spec

djrtwo commented 5 years ago

lgtm!