Routine for processing deposits #1640

hwwhww · 2018-12-18T12:35:31Z

What was wrong?

Implement routine for processing deposits

How was it fixed?

Add BeaconState.update_validator(validator_index, validator)
Fix DepositInput
Fix get_new_validator_registry_delta_chain_tip
Add eth/beacon/deposits_helpers.py <-- might be put to somewhere under state machine
- Implement def min_empty_validator_index as spec
- Impelment def process_deposit as spec

Question to the reviewers:

The state transition function will update state: BeaconState RLP/SSZ object frequently. Currently, I'm using changeset() for each snippet, but I'm not sure if that's the best practice.
Also add an instance function BeaconState.update_validator(validator_index, validator) for updating BeaconState.validator_registry: Sequence[ValidatorRecord] - immutably. Do you think it's okay for readability or any other concerns?

Cute Animal Picture

djrtwo

great! comments below

eth/beacon/deposit_helpers.py

djrtwo · 2018-12-18T18:12:14Z

tests/beacon/test_deposit_helpers.py

+        validator_registry=(),
+    )
+
+    privkey_1 = privkeys[0]


maybe should do a loop

for index in range(3): privkey = privkeys[index] pubkey = pubkeys[index] .... result_state, validator_index = process_deposit() ... check_results()

Oh but I want to make an empty slot between the tests. ("# Force the first validator exited")

Oh I see! Maybe update comment

# Add the third validator. # Should overwrite previously exited validator

or something. Would be good to make it more explicit than just the assert len(result_state.validator_registry) == 2

djrtwo · 2018-12-18T18:20:06Z

(2) seems okay to me.
Not sure about (1). We're going to be updating the shit out of state in the transition function.

pipermerriam · 2018-12-18T22:27:14Z

eth/beacon/deposit_helpers.py

+                current_slot
+        ):
+            return index
+    return None


We tend to try and stay away from return value checking. I'd convert this to raise an IndexError or a custom exception (probably the better option) here to ensure that call-sites cannot accidentally treat the return value incorrectly.

pipermerriam · 2018-12-18T22:28:02Z

eth/beacon/deposit_helpers.py

+            "BLS signature verification error"
+        )
+
+    return True


Our convention for validation functions is to have them return None (implicitly with no return statement).

pipermerriam · 2018-12-18T23:04:08Z

eth/beacon/deposit_helpers.py

+        # Update validator's balance and state
+        with validator.build_changeset() as validator_changeset:
+            validator_changeset.balance = validator.balance + deposit
+            validator = validator_changeset.commit()


This can be simplified to: validator = validator.copy(balance=validator.balance + deposit). The changeset API is better purposed for when you need to do multiple updates, potentially with some conditional logic.

Oh, you're right.

And I just benchmarked between using copy() and changeset() - I thought using copy would be slower since it's deepcopy, but, it turns out using copy is 30-40% faster(!) (with a simple RLP data with 13 fields)

p.s. I haven't tried a hugh RLP data like BeaconState. But the performance for ValidatorRecord should be better with using copy().

hwwhww · 2018-12-19T12:02:03Z

Changelog

Rebase for resolving conflicts
Add eth/beacon/exceptions.py - MinEmptyValidatorIndexNotFound
Refactors
1. Update eth.utils.bls
  1. Currently, inside the the pairing functions, there are some assert ... thing that will be fixed in Refactoring the Codebase py_ecc#35. Currently I add something to make sure the verify function returns bool.
2. Add eth.beacon.deposit_helpers.add_pending_validator
3. Add ValidatorRecord.get_pending_validator() classmethod for generating new pending validator
Resolve PR feedback

djrtwo · 2018-12-19T16:12:49Z

tests/beacon/test_deposit_helpers.py

+        validator_registry=(),
+    )
+
+    privkey_1 = privkeys[0]


Oh I see! Maybe update comment

# Add the third validator. # Should overwrite previously exited validator

or something. Would be good to make it more explicit than just the assert len(result_state.validator_registry) == 2

djrtwo · 2018-12-19T16:16:34Z

eth/beacon/deposit_helpers.py

+
+    validator_pubkeys = tuple([v.pubkey for v in state.validator_registry])
+    if pubkey not in validator_pubkeys:
+        validator = ValidatorRecord.get_pending_validator(


Would this be clearer as create_pending_validator rather than get_?
get implies to me that it is trying to retrieve something that at least might already exist.

Not sure what the codebase naming conventions are in general. Will defer to @pipermerriam

pipermerriam · 2018-12-19T16:20:48Z

eth/beacon/deposit_helpers.py

+                                  current_slot: int,
+                                  zero_balance_validator_ttl: int) -> int:
+    for index, validator in enumerate(validators):
+        if (


small formatting nitpick. I'd capture the contidional in a variable and then use it in a single line if

is_empty = (validator.balance == 0 and ...) if is_empty: return index

pipermerriam · 2018-12-19T16:23:14Z

eth/beacon/deposit_helpers.py

+    except MinEmptyValidatorIndexNotFound:
+        index = None
+
+    if index is None:


I think this would all be a little better readability if you had all of this in the try/except/else

try: index = ... except MinEmptyValidatorIndexNotFound: ... # logic from the `if index is None` else: ... # other logic. return state, index

pipermerriam · 2018-12-19T16:24:00Z

eth/beacon/deposit_helpers.py

+        randao_commitment,
+    )
+
+    validator_pubkeys = tuple([v.pubkey for v in state.validator_registry])


The inner [...] brackets can be removed as tuple(generator) works just fine.

pipermerriam · 2018-12-19T16:24:40Z

eth/beacon/deposit_helpers.py

+        index = validator_pubkeys.index(pubkey)
+        validator = state.validator_registry[index]
+
+        if validator.withdrawal_credentials != validator.withdrawal_credentials:


This conditional looks broken. Seems like you are comparing the same value with itself. Is one of these supposed to be just withdrawal_credentials.

Maybe add a test that covers this code path since this code path probably isn't being exercised in the tests right now?

Yeah, should be checking that the existing validator with the pubkey pubkey has the same withdrawal_credentials that were passed in with the deposit

pipermerriam · 2018-12-19T16:27:56Z

eth/beacon/types/states.py

+                         validator: ValidatorRecord) -> 'BeaconState':
+        validator_registry = list(self.validator_registry)
+        validator_registry[validator_index] = validator
+        self = self.copy(


I'm 👎 on overriding self in a method context. Seems like an antipattern and easily able to lead to confusion. Maybe just inline the copy in the return statement or call the variable something like updated_state

pipermerriam · 2018-12-19T16:29:28Z

eth/utils/bls.py

+            pairing(
+                FQP_point_to_FQ2_point(hash_to_G2(message, domain)),
+                neg(decompress_G1(pubkey)),
+                False


I find positional booleans to be confusing since they give no context on what is being False'd.. Maybe convert to either a keyword argument, or just use a comment to denote what argument this is for if performance is an issue.

pipermerriam · 2018-12-19T16:44:40Z

eth/utils/bls.py

+        for m_pubs in set(messages):
+            # aggregate the pubs
+            group_pub = Z1
+            for i in range(len_msgs):


This looks like you could do for pubkey, message in zip(pubkeys, messages): which removes the indexing.

Alternatively, I think the following is a functional equivalent implementation.

groups = ( pairing(hash_to_G2(m_pubs, domain), reduce(add, (decompress_G1(pubkey), Z1)), False) for (pubkey, message) in zip(pubkeys, messages) if message == m_pubs for m_pubs in set(messages) ) final_exponentiation = final_exponentiate( reduce( operator.mul, groups, FQ12([1] + [0] * 11), ) * pairing(decompress_G2(signature), neg(G1), False) ) return final_exponentiation == FQ12.one()

Also, there are a few constants in there that could be lifted out such as:

FQ12.one()

neg(G1)

The above is just hand-written code without being tested so you should verify if you choose to switch. I think the above code would be more performant... but that's just intuition.

Thanks for the feedback! 👍 That's looks great!

I'm inclined to leave this refactoring to #1592 because the current spec is still a draft, we may seek for review from academic cryptographers to help review in 1-2 months; in the meantime, the Python codes are the reference for the other implementers.

hwwhww added eth2.0 PR state: needs review labels Dec 18, 2018

djrtwo reviewed Dec 18, 2018

View reviewed changes

pipermerriam reviewed Dec 18, 2018

View reviewed changes

hwwhww force-pushed the on_startup_4 branch from 5679200 to d401df0 Compare December 19, 2018 11:13

hwwhww requested review from pipermerriam and djrtwo December 19, 2018 15:50

djrtwo approved these changes Dec 19, 2018

View reviewed changes

pipermerriam approved these changes Dec 19, 2018

View reviewed changes

hwwhww force-pushed the on_startup_4 branch from 53abc8d to f3a4b8c Compare December 21, 2018 09:31

hwwhww added 9 commits December 21, 2018 17:31

Add BeaconState.update_validator(validator_index, validator)

9c0ada5

Fix DepositInput

e0e23c2

Fix get_new_validator_registry_delta_chain_tip

9044f99

Add eth/beacon/deposits.py

f7109cd

fix path

3fc6b4c

Add test_min_empty_validator_index

ea0c27d

PR feedback

d0f3929

Add eth/beacon/exceptions.py

c65ee5b

PR feedback

c74560f

hwwhww force-pushed the on_startup_4 branch from f3a4b8c to c74560f Compare December 21, 2018 09:33

hwwhww merged commit d82b10a into ethereum:master Dec 21, 2018

hwwhww mentioned this pull request Jan 10, 2019

Reorg BLS module ethereum/trinity#137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Routine for processing deposits #1640

Routine for processing deposits #1640

hwwhww commented Dec 18, 2018

djrtwo left a comment

djrtwo Dec 18, 2018

hwwhww Dec 19, 2018

djrtwo Dec 19, 2018

djrtwo commented Dec 18, 2018

pipermerriam Dec 18, 2018

pipermerriam Dec 18, 2018

pipermerriam Dec 18, 2018

hwwhww Dec 19, 2018 •

edited

Loading

hwwhww Dec 19, 2018

hwwhww commented Dec 19, 2018

djrtwo Dec 19, 2018

djrtwo Dec 19, 2018 •

edited

Loading

pipermerriam Dec 19, 2018

pipermerriam Dec 19, 2018

pipermerriam Dec 19, 2018

pipermerriam Dec 19, 2018

djrtwo Dec 19, 2018

pipermerriam Dec 19, 2018

pipermerriam Dec 19, 2018

pipermerriam Dec 19, 2018

hwwhww Dec 21, 2018

Routine for processing deposits #1640

Routine for processing deposits #1640

Conversation

hwwhww commented Dec 18, 2018

What was wrong?

How was it fixed?

Cute Animal Picture

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented Dec 19, 2018

Changelog

Choose a reason for hiding this comment

djrtwo Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Dec 19, 2018 •

edited

Loading

djrtwo Dec 19, 2018 •

edited

Loading