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

Compact committee roots #1219

Merged
merged 11 commits into from
Jun 29, 2019
42 changes: 42 additions & 0 deletions motes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
* `BLS_WITHDRAWAL_PREFIX`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loving your motes :) Will comment on the various points later today 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no! I accidentally pushed. Haaa

djrtwo marked this conversation as resolved.
Show resolved Hide resolved
* Why int rather than bytes?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably be bytes :)

* `MIN_SEED_LOOKAHEAD`
* Is this actually tunable?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't any reason why it's not tunable

* If so, what are the reprecussions?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can think of two repercussions:

  1. It changes the lookahead (duh)
  2. For security we need to correspondingly adjust ACTIVATION_EXIT_DELAY to be at least MIN_SEED_LOOKAHEAD + 3.

* `ACTIVATION_EXIT_DELAY`
* Reaquaint with purpose
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that attackers can know the randomness beyond MIN_SEED_LOOKAHEAD, and hence bias the shuffling by strategically registering/deregistering validators in the window between MIN_SEED_LOOKAHEAD and MIN_SEED_LOOKAHEAD + 3 to "grind" the shuffling. (The number 3 is large enough to guarantee that, with high probability, an attacker's lookahead cannot go beyond MIN_SEED_LOOKAHEAD + 3.)

* AttesterSlashings
* `MAX_ATTESTER_SLASHINGS` is 1.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the reasons being:

  1. 1 is sufficiently large
  2. Attester slashings are huge objects, and could be used as a DoS vector

* Are there scenarios in which validators can create more effective slashable
messages than can be included on chain? For example, Validators split up to
create double attestations for checkpoints but different (junk) crosslink
data to prevent them from being aggregatable to the fullest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember thinking about this. One way to create more effective slashable messages is as follows. Let's say you control n validators, and you want to make bad attestations a_1, ..., a_n for each of these validators. The attacker can broadcast aggregated attestations (a_1, a_i) for i > 1 and never reveal the individual a_i. This is an argument for generalising the bitfield to a multibitfield.

* Max is for block size, no?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MAX_ATTESTER_SLASHINGS is to cap the maximum block size, yes.

* Signature domains
* Max 4byte ints
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a lot of signature domains :)

* `Version` not defined in one of the lists of custom types (2!!). ensure in spec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be cleaned up

* `PendingAttestation`
* Don't think `proposer_index` is actually necessary here because effective
balance is stable until end of epoch so can do dynamic lookups
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nice find! We only use proposer_index once so this is a nice cleanup I guess.

* is_genesis_trigger
* only run at ends of blocks to preserve invariant that eth1data.deposit_root
is the deposit root at the _end_ of an eth1 block
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* `Attestation`
* why bitfields not together?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "together"? Neither concatenation nor interleaving is nice. A mathematically clean solution would be (ternary) "trits".

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant in the field ordering

* `Transfer`
* replay mechanism... say the slot gets missed and you sign another transfer
* in a fork you could include both transfers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discussed this with Vitalik at Devcon. The current replay mechanism is quite bad and user unfriendly. The latest suggestion is to have a buffer of the latest n (where n is sufficiently large) transfer roots, and use that as replay protection.

Since we know we want to change the current transfer logic, can we please remove transfers from phase 0? Including the current logic is now actively misleading.

* `get_previous_epoch`
* do a once over on the genesis stuff
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand

Copy link
Contributor

Choose a reason for hiding this comment

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

Was personal note to dig into how previous_epoch performs near genesis epoch. Generally, we are just skipping things in 0th or 1st epoch

* `get_epoch_start_shard`
* checking next hinges upon the fact that the validator set for the next
epoch is 100% known at the current epoch. Ensure this is the case
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't understand

Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that there are no ways that during epoch N, we don't 100% know the active v-set during epoch N+1. In general is our assumption but want to go through and check.

* `get_block_root_at_slot` .. `generate_seed` can be bade into one line
function signatures
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay for one-line function signatures. This is part of the cleanup planned in #918.

* `get_shuffled_index`
* I think it should be maybe `assert index_count <= VALIDATOR_REGISTRY_LIMIT`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right

* is the `2**40` special for security of alg? probably.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, not sure.



pubkey/privkey g1 vs g2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pubkey should be in g1 since we're adding a bunch of them when aggregating.


1 change: 1 addition & 0 deletions notes.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* `BLS_WITHDRAWAL_PREFIX` --
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions specs/core/0_beacon-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ def get_compact_committees_root(state: BeaconState, epoch: Epoch) -> Hash:
"""
Return the compact committee root for the current epoch.
"""
committees = Vector[CompactCommittee, SHARD_COUNT]()
committees = [CompactCommittee() for _ in range(SHARD_COUNT)]
start_shard = get_epoch_start_shard(state, epoch)
for committee_number in range(get_epoch_committee_count(state, epoch)):
shard = (start_shard + committee_number) % SHARD_COUNT
Expand All @@ -769,7 +769,7 @@ def get_compact_committees_root(state: BeaconState, epoch: Epoch) -> Hash:
# `index` (top 6 bytes) + `slashed` (16th bit) + `compact_balance` (bottom 15 bits)
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
compact_validator = uint64(index << 16 + validator.slashed << 15 + compact_balance)
committees[shard].compact_validators.append(compact_validator)
return hash_tree_root(committees)
return hash_tree_root(Vector[CompactCommittee, SHARD_COUNT](committees))
```

### `generate_seed`
Expand Down