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

Add test_genesis.py and cleanup genesis trigger #1202

Merged
merged 52 commits into from
Jun 30, 2019
Merged

Add test_genesis.py and cleanup genesis trigger #1202

merged 52 commits into from
Jun 30, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jun 21, 2019

  1. Fix is_genesis_trigger fails to verify deposit merkle branch #1199
  • modify to initialize_beacon_state_from_eth
  • add is_valid_genesis_state
  1. Add test_libs/pyspec/eth2spec/test/genesis/ tests
  2. Update deposit contract (per Make deposit root the root of an SSZ list #1229)

@hwwhww hwwhww changed the title [wip] add test_genesis Add test_genesis.py and fix is_genesis_trigger Jun 22, 2019
@hwwhww hwwhww marked this pull request as ready for review June 22, 2019 04:40
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@JustinDrake JustinDrake added the milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet label Jun 22, 2019
@JustinDrake
Copy link
Collaborator

@hwwhww What do you think of a3f8f50?

@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 24, 2019

@JustinDrake

In practice, a client, they would probably:

  1. Build deposit_data_list: List[DepositData] from deposit contract log.
  2. Generate proof against the latest deposit_root (can be gotten from the contract, or just merkleize the deposit_data_list).
  3. Build the deposits: List[Deposit] with these proof.
  4. Check if it triggers the genesis starting point.

So it's unlikely they would need to regenerate deposit_root after they have created the Deposits, in practice. That's why I'd say we don't need to regenerate the deposit_root again.

Another way to describe the logic is changing the function to def is_genesis_trigger(deposit_data_list: List[DepositData], timestamp: uint64) -> bool, and generate the proof in this function. I hope to balance between (i) making the spec describe the protocol in details and (ii) making it too far from practice. 😄

What do you think about it? 😸

@JustinDrake
Copy link
Collaborator

Right. In practice I expect implementers to have a cache (e.g. a Store mapping deposits to beacon states) and an incremental version of is_genesis_trigger. This would avoid recomputing things from genesis, and handle Eth1 reorgs. I'd favour spec readability over efficiency, but don't feel too strongly either way :)

@protolambda
Copy link
Collaborator

  • fixed the merge from Justin, it was broken:
    • introduced back an invalid deposit test, we don't have deposit.index anymore
    • syntax problem, decorator was over empty line
    • confusing big merge, few merge conflicts, but know not to pick the old(!!!) diff parts, when they changed intentionally...
  • Moved genesis tests out of sanity. The idea is that the shape of each test is the same within one file.
  • Annotated the yielded data, so that it actually gets into a dict of key-value for a future test generator
  • Fixed the genesis-trigger-true tests partially. Date was the same as the false case...
  • Introduced local SECONDS_PER_DAY constant. You can make it a real global, but if it's for an "example" part of the code, we shouldn't include it in the main constants imho.
  • Fixed problems with python syntax, map was used wrong, and generator (... for ... in ...) results were used as if it were lists (they are types.GeneratorType)
  • The Deposit-root for the truee-trigger is still wrong somehow, but something for you to figure out, may be the expected root that is wrong, or some other funky reason more familiar to the PR authors

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
…t, 2**DEPOSIT_CONTRACT_TREE_DEPTH` and fix tests
hwwhww added 2 commits June 30, 2019 03:27
1. `Deposit` log -> `DepositEvent` log
2. `get_deposit_root` -> `get_hash_tree_root`
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
hwwhww and others added 3 commits June 30, 2019 06:38
1. Rename the current `get_genesis_beacon_state(...)` to `initialize_beacon_state_from_eth1(...)`
2. Extract `is_valid_genesis_state(state: BeaconState) -> bool` from `initialize_beacon_state_from_eth1(...)`
@protolambda
Copy link
Collaborator

  • Added signed=True args to make it work with BLS
  • Added a few remarks to avoid confusion in genesis/deposit verification

Ready to merge imho. We can add a few additional tests after spec-freeze. Possibly even improve the efficiency of genesis construction. As clients are (very) likely not implementing it like this anyway, and state output will be the same. This description and pseudocode will at least set a baseline, and set the interface to be compatible with SSZ partials in the future.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

this cleaned up nicely. Great work everyone!

@djrtwo djrtwo merged commit 2f43f9c into dev Jun 30, 2019
@djrtwo djrtwo deleted the test_genesis branch June 30, 2019 04:31
@djrtwo djrtwo changed the title Add test_genesis.py and fix is_genesis_trigger Add test_genesis.py and cleanup genesis trigger Jun 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet scope:CI/tests/pyspec scope:deposit contract
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_genesis_trigger fails to verify deposit merkle branch
4 participants