aggregate all block signatures as naive list [devnet-1]#67
aggregate all block signatures as naive list [devnet-1]#67unnawut merged 56 commits intoleanEthereum:mainfrom
Conversation
4e0f9f1 to
adecd99
Compare
Signed-off-by: Chen Kai <281165273grape@gmail.com>
ac47e76 to
3d2de22
Compare
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
unnawut
left a comment
There was a problem hiding this comment.
Sorry for missing the Monday's call, and also apologies if any of these comments sound stupid, still trying to catch up.
I'm also trying to grasp all the *Attestation containers and AggregatedSignatures so no thoughts on this part yet other than it seems more complex than the beacon chain.
Should we design this at all for devnet1? Even if we do, I'm thinking it could go as a separate PR so at least the individual signatures can be implemented first. Also would make the discussion scope narrower.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
jihoonsong
left a comment
There was a problem hiding this comment.
Can you rebase your PR?
docs/client/forkchoice.md
Outdated
| # sync parent chain if not available before adding block to forkchoice | ||
| assert parent_state is not None, "Parent state not found, sync parent chain first" | ||
|
|
||
| valid_signatures = validate_signatures(block.signature) |
There was a problem hiding this comment.
What was the reason that we don't do assert validate_signatures(block.signature) here and make state_transition receive valid_signatures?
There was a problem hiding this comment.
Its fine either way imo as this is spec, clients can choose to optimize/parallelize anywhich way they want but its good to be explicit in the STF that signatures have been validated
docs/client/containers.md
Outdated
| ## `Vote` | ||
| ## `AttestationData` | ||
|
|
||
| Vote is the attestation data that can be aggregated. Although note there is no aggregation yet in `devnet0`. |
There was a problem hiding this comment.
Nit: there is no Vote anymore.
There was a problem hiding this comment.
will look into it if we wanna remove all vote references,
docs/client/forkchoice.md
Outdated
| latest_known_votes: Dict[ValidatorIndex, SignedValidatorAttestation] = field(default_factory=dict) | ||
| latest_new_votes: Dict[ValidatorIndex, SignedValidatorAttestation] = field(default_factory=dict) |
There was a problem hiding this comment.
Why do these have to have SignedValidatorAttestation instead of Checkpoint?
There was a problem hiding this comment.
for block construction purposes, you need signed objects to get signatures you want to aggregate
docs/client/containers.md
Outdated
| # ValidatorAttestation created from its ProposerAttestationData. | ||
| # till we can actually consume a block signature for re-packing this block's | ||
| # proposer vote by some other future proposer | ||
| signature: List[Vector[byte, 4000], VALIDATOR_REGISTRY_LIMIT] |
There was a problem hiding this comment.
I probably missing something so I'm asking a question here. When we aggregate signatures, do we want to aggregate all different kinds of signatures? In other words, what's the best way to understand why we want to aggregate attestation signatures along with block signature?
There was a problem hiding this comment.
signatures are heavy now, also with OTS constraint proposer signature which is over the block and vote is sort of anyway not "pure" anymore so to take things to logical conclusion a block (and vote) signature will represent all valid objects in the block (and vote)
docs/client/containers.md
Outdated
| ``` | ||
|
|
||
| ## `SignedVote` | ||
| ## `ProposerAttestationData` |
There was a problem hiding this comment.
While I understand the constraints that made us bring up this approach, I believe the problem of signing multiple times in a slot remains. If we want to adopt this PG signature effort into a production-level protocol, it's highly likely that a proposer needs to sign more than one message in a slot. If we pack all those messages into one, a proposer has to broadcast all of their messages at once. In other words, there is only one deadline within a slot for a proposer. This is not flexible so I'm not sure if this would be enough to meet all the needs of the protocol.
IMO, we could do either find a proper solution for the problem or use a stopgap solution like this one that is just enough for the upcoming few devnets. If we were to go to the latter, current approach is one of solutions. But it seems it adds complexity, which is might not an ideal given that it's not a long-term solution. So I'd like to encourage to find a better one. Few things I have in my mind are:
-
Currently,
BlockBodyhas a list of unsigned votes. We can append proposer's attestation at the end of the list and append proposer's signature at theSignedBlock's signature. It's not very clean as there is an exception that proposer's signature is made over a block while others are over attestation. But this is already what we have if I'm not wrong, and this doesn't require to change originalVoteand related classes nor to addProposerAttestationData. -
We can add a rule that proposers don't vote. This would make things much simpler.
There was a problem hiding this comment.
for
-
yes we are trying to move closer to aggregation so the list there is just a temporary thing till we have leanVM coming up. also till then its just proposer votes signature, so not even a full block & vote signature since we can't validate such a signature without leanVM
-
yes can be considered actually, was in my mind, but eventually signatures will be aggregated
There was a problem hiding this comment.
infact the signature part can just be totally replaced by full block validity proof including execution
so yes design space is open and we will see how to simplify it if it turns out convoluted
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
|
|
||
| block = sample_store.produce_block(slot, validator_idx) | ||
|
|
||
| block_and_signatures = sample_store.produce_block(slot, validator_idx) |
There was a problem hiding this comment.
| block_and_signatures = sample_store.produce_block(slot, validator_idx) | |
| block, signatures = sample_store.produce_block(slot, validator_idx) |
You can remove BlockAndSignatures as well.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
| "AttestationData", | ||
| "Attestation", | ||
| "SignedAttestation", | ||
| "SignedAggreagtedAttestations", |
There was a problem hiding this comment.
| "SignedAggreagtedAttestations", | |
| "SignedAggregatedAttestations", |
also applies to other places
| @property | ||
| def slot(self) -> Slot: | ||
| """Return the attested slot.""" | ||
| return self.data.slot | ||
|
|
||
| @property | ||
| def head(self) -> Checkpoint: | ||
| """Return the attested head checkpoint.""" | ||
| return self.data.head | ||
|
|
||
| @property | ||
| def target(self) -> Checkpoint: | ||
| """Return the attested target checkpoint.""" | ||
| return self.data.target | ||
|
|
||
| @property | ||
| def source(self) -> Checkpoint: | ||
| """Return the attested source checkpoint.""" | ||
| return self.data.source |
There was a problem hiding this comment.
Not sure if we need these accessor functions in the specs, seems more like implementation's decision to make than the specs
There was a problem hiding this comment.
Agreed, this makes the spec heavier and we can always come and get them with self.data.source for example
| @property | ||
| def data(self) -> Attestation: | ||
| """Expose the message for backwards compatibility with SignedVote.""" | ||
| return self.message |
There was a problem hiding this comment.
We don't need backward compatibility right now so this can be removed
| committee assignments. This structure is defined for future use and is not | ||
| currently exercised by devnets. |
There was a problem hiding this comment.
| committee assignments. This structure is defined for future use and is not | |
| currently exercised by devnets. | |
| committee assignments. |
I think it's needed in devnet1 where aggregation_bits will used to map AggregatedSignatures to their validator id.
| # Ensure forkchoice is current before processing gossip | ||
| time_slots = self.time // INTERVALS_PER_SLOT | ||
| assert vote.slot <= time_slots, "Attestation from future slot" | ||
| time_slots = self.time // SECONDS_PER_INTERVAL |
There was a problem hiding this comment.
Similar to previous comment:
| time_slots = self.time // SECONDS_PER_INTERVAL | |
| time_slots = self.time // SLOT_DURATION_MS // 1000 |
There was a problem hiding this comment.
merge issue, it should be INTERVALS_PER_SLOT
| # TODO: Integrate actual aggregated signature verification. | ||
| return all(self._is_valid_signature(signature) for signature in signatures) | ||
|
|
||
| def process_block(self, signed_block_vote: SignedBlockWithAttestation) -> None: |
There was a problem hiding this comment.
Since the type name changed
| def process_block(self, signed_block_vote: SignedBlockWithAttestation) -> None: | |
| def process_block(self, signed_block_with_attestation: SignedBlockWithAttestation) -> None: |
There was a problem hiding this comment.
Agreed, we should modify the doc string as well if we change this
| for validator_id, checkpoint in self.latest_known_votes.items(): | ||
| new_attestations: list[Attestation] = [] | ||
| new_signatures: list[Bytes4000] = [] | ||
| for signed in self.latest_known_votes.values(): |
There was a problem hiding this comment.
| for signed in self.latest_known_votes.values(): | |
| for signed_attestation in self.latest_known_votes.values(): |
naming ambiguity
| parent_root=head_root, | ||
| state_root=Bytes32.zero(), # Will be updated with computed hash | ||
| body=BlockBody(attestations=Attestations(data=attestations)), | ||
| body=BlockBody(attestations=Attestations(data=list(attestations))), # need copy |
There was a problem hiding this comment.
Not sure what the comment means?
| class Bytes4000(BaseBytes): | ||
| """Fixed-size byte array of exactly 4000 bytes.""" | ||
|
|
||
| LENGTH = 4000 |
There was a problem hiding this comment.
This type is very unusual and will probably be removed in the future.
As this file is about types that should be a bit more stable than the rest of the code, can you put a TODO or something to indicate that this will be removed in the future?
| @property | ||
| def slot(self) -> Slot: | ||
| """Return the attested slot.""" | ||
| return self.data.slot | ||
|
|
||
| @property | ||
| def head(self) -> Checkpoint: | ||
| """Return the attested head checkpoint.""" | ||
| return self.data.head | ||
|
|
||
| @property | ||
| def target(self) -> Checkpoint: | ||
| """Return the attested target checkpoint.""" | ||
| return self.data.target | ||
|
|
||
| @property | ||
| def source(self) -> Checkpoint: | ||
| """Return the attested source checkpoint.""" | ||
| return self.data.source |
There was a problem hiding this comment.
Agreed, this makes the spec heavier and we can always come and get them with self.data.source for example
| @dataclass(slots=True) | ||
| class BlockAndSignatures: | ||
| """Internal bundle pairing a block with its attestation signatures.""" | ||
|
|
||
| block: Block | ||
| signatures: list[Bytes4000] |
There was a problem hiding this comment.
Why not BlockAndAttestations here to be consistent with the rest?
| # TODO: Integrate actual aggregated signature verification. | ||
| return all(self._is_valid_signature(signature) for signature in signatures) | ||
|
|
||
| def process_block(self, signed_block_vote: SignedBlockWithAttestation) -> None: |
There was a problem hiding this comment.
Agreed, we should modify the doc string as well if we change this
| validator_idx = ValidatorIndex(2) # Proposer for slot 2 | ||
|
|
||
| block = sample_store.produce_block(slot, validator_idx) | ||
| block_and_signatures = sample_store.produce_block(slot, validator_idx) |
There was a problem hiding this comment.
| block_and_signatures = sample_store.produce_block(slot, validator_idx) | |
| block_and_attestations = sample_store.produce_block(slot, validator_idx) |
There was a problem hiding this comment.
@jihoonsong suggest removing this type and just return multi values, is that preference
There was a problem hiding this comment.
Yeah the type is just for the return value and is not necessary. Clients can choose to use such intermediary types, if they prefer.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Co-authored-by: Thomas Coratger <60488569+tcoratger@users.noreply.github.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
|
@unnawut @tcoratger fix almost comments |
Co-authored-by: JihoonSong <jihoonsong@users.noreply.github.com>
jihoonsong
left a comment
There was a problem hiding this comment.
This PR is exactly 1 month old. This can go forever. Let's merge this and iterate fast.
one of the important lean-ing step especially for STF proving considerations as well as to lighten the block network payload
Also the proposer vote isn't casted separate (so validator doesn't emit vote for a validator id if its that validator id's turn to vote)
but is directly bundled with the block because of the OTS scheme limitation