executable light client patch: beacon-chain.md #2141

hwwhww · 2020-12-01T18:07:50Z

Patch for #2130

This PR

Bump remerkleable to 0.1.18 ae86517 Thanks @protolambda!
Use new optional_fast_aggregate_verify f0864b8 /cc @JustinDrake
Add LIGHTCLIENT fork to test/context.py
Make it pass the most phase0 tests except for the inactivity leak tests (see below)

TODO in this PR:

Rename LIGHTCLIENT fork to another code name that is more like a "fork". Does LIGHTCLIENT_PATCH work? /cc @djrtwo

TODOs in other PRs:

Fix inactivity leak issues (see Added standalone light client patch #2130 (comment) discussions)

failed tests

FAILED eth2spec/test/phase0/epoch_processing/test_process_rewards_and_penalties.py::test_almost_empty_attestations_with_leak
FAILED eth2spec/test/phase0/epoch_processing/test_process_rewards_and_penalties.py::test_random_fill_attestations_with_leak
FAILED eth2spec/test/phase0/epoch_processing/test_process_rewards_and_penalties.py::test_almost_full_attestations_with_leak
FAILED eth2spec/test/phase0/epoch_processing/test_process_rewards_and_penalties.py::test_full_attestation_participation_with_leak

This PR disabled sync-protocol.md for now. Let's make it executable with other PRs.
Add light client patch tests

specs/phase0/validator.md

specs/lightclient/beacon-chain.md

hwwhww · 2020-12-07T19:12:33Z

Changelog

As discussed during the call and with @djrtwo in chat, do not cancel the sync committee rewards when dealing inactivity leak.
Rebased.

Now trying to fix the new error after rebasing:

eth2spec/test/phase0/sanity/test_blocks.py:319: in test_empty_epoch_transition
    signed_block = state_transition_and_sign_block(spec, state, block)
eth2spec/test/helpers/state.py:83: in state_transition_and_sign_block
    transition_unsigned_block(spec, state, block)
eth2spec/test/helpers/block.py:62: in transition_unsigned_block
    spec.process_block(state, block)
eth2spec/lightclient_patch/spec.py:1148: in process_block
    process_sync_committee(state, block)
eth2spec/lightclient_patch/spec.py:1825: in process_sync_committee
    committee_pubkeys = state.current_sync_committee.pubkeys
../../../venv/lib/python3.8/site-packages/remerkleable/complex.py:820: in __getattr__
    return super().get(i)
../../../venv/lib/python3.8/site-packages/remerkleable/subtree.py:29: in get
    self.get_backing().getter(to_gindex(i, self.tree_depth())), lambda v: self.set(i, v))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = 0x0000000000000000000000000000000000000000000000000000000000000000
target = 2

    def getter(self, target: Gindex) -> Node:
        if target != 1:
>           raise NavigationError
E           remerkleable.tree.NavigationError

../../../venv/lib/python3.8/site-packages/remerkleable/tree.py:292: NavigationError

I still could not reproduce a similar error case in writing remerkleable tests. Investigating if something is wrong on the spec side. 👀

/cc @protolambda do you have any hints of why this error happened? I tried to comment out raise NavigationError, then some tests passed (haven't run all the tests locally with this version yet).

hwwhww · 2020-12-07T19:24:38Z

specs/lightclient/beacon-chain.md

@@ -165,30 +186,59 @@ def process_block(state: BeaconState, block: BeaconBlock) -> None:
 ```python
 def process_sync_committee(state: BeaconState, block: BeaconBlock) -> None:
    # Verify sync committee aggregate signature signing over the previous slot block root
-    previous_slot = Slot(max(state.slot, 1) - 1)
+    previous_slot = Slot(max(int(state.slot), 1) - 1)


@djrtwo

This line was previous_slot = max(state.slot, Slot(1)) - Slot(1) earlier.

But since max needs to compare the same type, either we cast to Slot or cast to int.

protolambda · 2020-12-07T19:24:40Z

@hwwhww it looks like it was accessing a field in a container, recognized the wrong count of container fields (probably related to new extended types), created a generalized-index that wasn't deep enough (e.g. 9 fields instead of 8 = subtree twice as deep) and then failed to retrieve the node at the generalized index that was too shallow. Interesting! I'll run through this and see if it's a usage problem or a new bug due to OOP changes in remerkleable. Edit: that could have been it, but it wasn't: the tree was not far enough expanded in width to cover all new fields, since it was a phase 0 state.

hwwhww · 2020-12-07T19:25:37Z

specs/lightclient/beacon-chain.md

    # Reward sync committee participants
-    participant_rewards = Gwei(0)
+    proposer_reward = Gwei(0)
    active_validator_count = uint64(len(get_active_validator_indices(state, get_current_epoch(state))))
    for participant_index in participant_indices:
        base_reward = get_base_reward(state, participant_index)
-        reward = Gwei(base_reward * active_validator_count // len(committee_indices) // SLOTS_PER_EPOCH)
+        max_participant_reward = base_reward - base_reward // PROPOSER_REWARD_QUOTIENT
+        reward = Gwei(max_participant_reward * active_validator_count // len(committee_indices) // SLOTS_PER_EPOCH)
        increase_balance(state, participant_index, reward)
-        participant_rewards += reward
+        proposer_reward += base_reward // PROPOSER_REWARD_QUOTIENT

    # Reward beacon proposer
-    increase_balance(state, get_beacon_proposer_index(state), Gwei(participant_rewards // PROPOSER_REWARD_QUOTIENT))
+    increase_balance(state, get_beacon_proposer_index(state), proposer_reward)


Based on https://github.com/ethereum/eth2.0-specs/pull/2130/files#r532294580

protolambda · 2020-12-07T19:56:14Z

Looked deeper into that navigation error: the state is backed by a tree that does not contain any nodes for current_sync_committee or next_sync_committee. For the first one, it happens to maintain a padding node to make it an even number (but the padding node has no child nodes, so you got a navigation error on access). The 2nd field doesn't have a node at all and will straight away result in a navigation error.

TLDR: need to look at state initialization now, see if it properly builds a tree suitable for use in the lighthclient-patch.

protolambda · 2020-12-07T20:02:43Z

Update: in the context.py (literally creates context for tests) it creates the test state. As part of testing, it can upgrade states to later phases. It is not doing this for the light-client-patch, and just plugs the old incomplete state tree into the expected beacon-state-type.
This typing step is an optimization: whenever it can, test preparation re-uses the same immutable state data (based on fork name, fork version, and initialization arguments).
I'll see if I can make it do that upgrade step to get a proper state.

protolambda · 2020-12-07T20:36:00Z

Attempt to fix it: the test state preparation now runs a new function upgrade_to_lightclient_patch, which essentially takes a phase0 state, and outputs a lightclient-patch state. Similar to how a phase1 state is built.

The upgrade_to_lightclient_patch is not pretty, since remerkleable does not support dict-like item retrieval yet. It could have been BeaconState(**pre.items()), or I can try make FooBar(Foo(example=123), more=42) -> FooBar {example: 123, more: 42} a thing for upgrades. The current BeaconState.fields() returns a dict of keys and field types, not values. Style suggestions welcome, we have a working placeholder now, so we have time to discuss.
Or we are more explicit about state upgrades (which IMHO is not a bad thing), and write it all out like with phase 1.

Edit: tests run mostly now, but there are some tests that fail for regular reasons, very close to executable spec though, nice work!

`process_block_header`

hwwhww · 2020-12-08T05:54:11Z

@protolambda

Thanks a lot for the help!!! That makes perfect sense.

I'm okay with BeaconState(**{k: getattr(pre, k) for k in pre.fields().keys()}) style, but I realized that we need to update the state.fork and state.latest_block_header explicitly so I added lightclient-fork.md as phase 1.

Still have some known issues that our tests didn't find, but finally see the green lights for all CI jobs! 🍏

hwwhww · 2020-12-08T07:37:06Z

specs/lightclient/lightclient-fork.md

+            epoch=epoch,
+        ),
+        # History
+        latest_block_header=pre.latest_block_header,


@djrtwo @vbuterin

Since sync_committee_bits and sync_committee_signature fields are added to block header instead of block body in 3b7c025, it would be more tricky to handle the SSZ objects at the fork boundary.

The latest_block_header field here should be transformed from phase0.BeaconBlockBody to lightclient_patch.BeaconBlockBody.

However, the next slot (the first slot after the light client patch) will have a problem when caching the previous_block_root = hash_tree_root(state.latest_block_header) in process_slot function because the previous root should have been computed with phase 0 form at phase 0.

I think it makes more sense to have sync_committee_bits and sync_committee_signature in BeaconBlockBody although they are not "operations".

Fixed in 2c86b43

JustinDrake · 2020-12-08T09:29:34Z

specs/lightclient/lightclient-fork.md

+
+- [Introduction](#introduction)
+- [Configuration](#configuration)
+- [Fork to Light-client patch](#fork-to-light-client-patch)


We write "light client" in three different ways 😂

light client

lightclient

light-client

I guess we want to be consistent. (Small preference for "light client" on my side.)

👍 on "light client".

Once we have a better hard fork name, we can replace the folder name lightclient with it.
We can scan it again after merging this PR to avoid conflicts.

1) Add a summary note for every function that is changed. 2) Avoid changing `process_block` (instead only change `process_block_header`). 3) Rename `G2_INFINITY_POINT_SIG` to `G2_POINT_AT_INFINITY` to avoid `SIG` contraction. 4) Misc cleanups

configs/mainnet/lightclient_patch.yaml

Co-authored-by: Marin Petrunić <[email protected]>

configs/minimal/lightclient_patch.yaml

…aconBlockBody`

hwwhww · 2020-12-15T05:02:58Z

@vbuterin @djrtwo @JustinDrake @protolambda

upgrade_to_lightclient_patch may still have some typing issues but it's not substantive. We can fix it later.

This PR is ready to be merged into upstream #2130.

hwwhww commented Dec 1, 2020

View reviewed changes

specs/phase0/validator.md Outdated Show resolved Hide resolved

hwwhww requested review from vbuterin, JustinDrake and djrtwo December 3, 2020 14:05

vbuterin reviewed Dec 3, 2020

View reviewed changes

specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved

vbuterin reviewed Dec 3, 2020

View reviewed changes

specs/lightclient/beacon-chain.md Outdated Show resolved Hide resolved

djrtwo mentioned this pull request Dec 3, 2020

WIP: Make light-client-sync spec executable #1992

Closed

2 tasks

hwwhww changed the title ~~executable light client patch~~ executable light client patch: beacon-chain.md Dec 3, 2020

hwwhww added the scope:light-clients label Dec 7, 2020

hwwhww added 15 commits December 8, 2020 00:25

Bump remerkleable to 0.1.18

762f66e

Disable sync-protocol.md for now. Make linter pass

13a7ae5

Enable lightclient tests

027aef7

Use *new* optional_fast_aggregate_verify

56a34f8

Fix ToC and codespell

99f6338

Do not run phase1 tests with Lightclient patch

0c85e36

Fix the Eth1Data casting bug. Add a workaround.

11606d0

Fix run_on_attestation testing helper

5aa0efb

Revert

51d2b95

Rename optional_fast_aggregate_verify to eth2_fast_aggregate_verify

445cdf7

Apply Proto's suggestion

615dda7

Apply Danny's suggestion

1b7b842

Fixing tests

e8bf518

Fix after rebasing

bda72e4

Rename LIGHTCLIENT -> LIGHTCLIENT_PATCH

6ee73a1

hwwhww force-pushed the lightclient-exe branch from b5374b6 to 6ee73a1 Compare December 7, 2020 18:13

hwwhww added 2 commits December 8, 2020 02:54

New doctoc

878553f

Add lightclient patch configs

92ebbca

hwwhww commented Dec 7, 2020

View reviewed changes

hwwhww mentioned this pull request Dec 7, 2020

Added standalone light client patch #2130

Merged

protolambda added 2 commits December 7, 2020 21:25

fix gitignore light client patch generator output

7f5e2d8

Upgrade state for light client patch

a97037e

Add lightclient-fork.md to deal the fork boundary and fix

c9603eb

`process_block_header`

hwwhww commented Dec 8, 2020

View reviewed changes

JustinDrake reviewed Dec 8, 2020

View reviewed changes

JustinDrake added 3 commits December 8, 2020 10:40

Misc cleanups

979fdbf

1) Add a summary note for every function that is changed. 2) Avoid changing `process_block` (instead only change `process_block_header`). 3) Rename `G2_INFINITY_POINT_SIG` to `G2_POINT_AT_INFINITY` to avoid `SIG` contraction. 4) Misc cleanups

Update block.py

4bbf304

Update beacon-chain.md

060619d

mpetrunic reviewed Dec 9, 2020

View reviewed changes

configs/mainnet/lightclient_patch.yaml Outdated Show resolved Hide resolved

hwwhww mentioned this pull request Dec 9, 2020

Make sync-protocol.md pass the linter and add basic tests #2147

Merged

Fix typo "minimal" -> "mainnet"

c5efae4

Co-authored-by: Marin Petrunić <[email protected]>

mpetrunic reviewed Dec 9, 2020

View reviewed changes

configs/minimal/lightclient_patch.yaml Show resolved Hide resolved

hwwhww added 3 commits December 11, 2020 16:07

Use the new BeaconBlockHeader instead of phase 0 version

b4571db

Update config files

2440f5a

Move sync_committee_bits and sync_committee_signature back to `Be…

2c86b43

…aconBlockBody`

hwwhww merged commit acfe49e into vbuterin-patch-2 Dec 15, 2020

hwwhww mentioned this pull request Mar 15, 2021

[minor] Fix missing --cov-report #2237

Merged

hwwhww deleted the lightclient-exe branch February 21, 2023 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executable light client patch: beacon-chain.md #2141

executable light client patch: beacon-chain.md #2141

hwwhww commented Dec 1, 2020 •

edited

Loading

hwwhww commented Dec 7, 2020

hwwhww Dec 7, 2020 •

edited

Loading

protolambda commented Dec 7, 2020 •

edited

Loading

hwwhww Dec 7, 2020

protolambda commented Dec 7, 2020

protolambda commented Dec 7, 2020

protolambda commented Dec 7, 2020 •

edited

Loading

hwwhww commented Dec 8, 2020

hwwhww Dec 8, 2020

hwwhww Dec 15, 2020

JustinDrake Dec 8, 2020

hwwhww Dec 8, 2020

hwwhww commented Dec 15, 2020

executable light client patch: beacon-chain.md #2141

executable light client patch: beacon-chain.md #2141

Conversation

hwwhww commented Dec 1, 2020 • edited Loading

This PR

TODO in this PR:

TODOs in other PRs:

hwwhww commented Dec 7, 2020

Changelog

hwwhww Dec 7, 2020 • edited Loading

Choose a reason for hiding this comment

protolambda commented Dec 7, 2020 • edited Loading

hwwhww Dec 7, 2020

Choose a reason for hiding this comment

protolambda commented Dec 7, 2020

protolambda commented Dec 7, 2020

protolambda commented Dec 7, 2020 • edited Loading

hwwhww commented Dec 8, 2020

hwwhww Dec 8, 2020

Choose a reason for hiding this comment

hwwhww Dec 15, 2020

Choose a reason for hiding this comment

JustinDrake Dec 8, 2020

Choose a reason for hiding this comment

hwwhww Dec 8, 2020

Choose a reason for hiding this comment

hwwhww commented Dec 15, 2020

hwwhww commented Dec 1, 2020 •

edited

Loading

hwwhww Dec 7, 2020 •

edited

Loading

protolambda commented Dec 7, 2020 •

edited

Loading

protolambda commented Dec 7, 2020 •

edited

Loading