Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

AuthorityRound: finalize blocks #9113

Closed
wants to merge 6 commits into from
Closed

AuthorityRound: finalize blocks #9113

wants to merge 6 commits into from

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Jul 12, 2018 •
edited
Loading

Implement ancestry_actions to mark finalized blocks using the existing rolling finality. I've also changed the is_epoch_end method so that it gets the recently finalized blocks as argument and can check the transition store using that.

@andresilva andresilva added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Jul 12, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 13, 2018
@sorpaas sorpaas mentioned this pull request Jul 16, 2018
Merged
@andresilva
Copy link
Contributor Author

The changes I did to Engine::is_epoch_end are not compatible with the way the light client works.

@5chdn 5chdn modified the milestones: 2.1, 2.2 Jul 17, 2018
@5chdn
Copy link
Contributor

5chdn commented Aug 24, 2018

@andresilva are you still working on this?

@5chdn 5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 24, 2018
@rphmeier
Copy link
Contributor

Seems like nobody reviewed it. @sorpaas can you take a look? I'll try to as well but I'm out-of-the-loop with parity-ethereum stuff.

@sorpaas
Copy link
Collaborator

sorpaas commented Aug 27, 2018

I remember the issue is with light client. Probably @andresilva can clarify more.

I didn't find anywhere that we write finalization info to light client currently (or do we?), so for the purpose of this refactoring PR, it may be okay to continue to ignore finality info for light client?

@rphmeier
Copy link
Contributor

The light client does track finality, but only for blocks which register validator set changes. Requires finality proofs though.

@andresilva
Copy link
Contributor Author

andresilva commented Aug 29, 2018 •
edited
Loading

I haven't worked on this further after submitting the original PR. I think the changes for the full client make sense, but I changed signature of check_epoch_end to check_epoch_end(Self, Header, [H256], BlockChain, Client), where the hashes are for recently finalized blocks since finality is tracked by the engine regardless of epoch change signals, but this is incompatible with the light client. The light client does track finality, but it tracks finality for all blocks in the current epoch and "starting from scratch" on each block import.

I think the easiest way to integrate this is to keep the existing finality tracking logic for the light client, this probably requires us to introduce a special light version of check_epoch_end that uses the same logic as before. WDYT?

@5chdn 5chdn added B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. and removed A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Sep 4, 2018
@andresilva
aura: emit ancestry actions for finalizing blocks
a9f3112
@andresilva
aura: refactor is_epoch_end to get finalized blocks as argument
7d417fa
@andresilva
ethcore: add is_epoch_end_light method to Engine
a758f68 
The full client now tracks finality by querying the engine on each block import,
and it also persists the finalization state to the DB. For the light client
current it doesn't persist finality information and only keeps track of finality
for epoch signals, by calling `is_epoch_end_light`. This method implements the
previously existing logic of building finality for all the blocks in the current
epoch and then checking the finalized blocks against the transition store.
@andresilva
ethcore: allow finalizing current block
10d5df4
@andresilva
aura: fix construction of finality proof
48df25b
@andresilva
aura: fix warnings
8d53074 
- missing docs for is_epoch_end_light
- unused method unfinalized_hashes in RollingFinality
@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 10, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 10, 2018

Adding patch label to make sure this ends up in 2.1-beta once branched out.

@5chdn 5chdn modified the milestones: 2.1, 2.2 Sep 11, 2018
@5chdn 5chdn mentioned this pull request Sep 11, 2018
Merged
21 tasks
@5chdn 5chdn removed B0-patchthis 🕷 B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. labels Sep 13, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 27, 2018

Please rebase on master

@5chdn 5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 29, 2018
@5chdn
Copy link
Contributor

5chdn commented Oct 2, 2018

Please reopen when ready

@5chdn 5chdn closed this Oct 2, 2018
@andresilva andresilva mentioned this pull request Oct 3, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sorpaas sorpaas Awaiting requested review from sorpaas

@rphmeier rphmeier Awaiting requested review from rphmeier

Assignees
No one assigned
Labels
A0-pleasereview 🤓 Pull request needs code review. A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@andresilva @5chdn @rphmeier @sorpaas