Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Fixed multiple blocks finalization#1601

Merged
rphmeier merged 1 commit intomasterfrom
fix_multiple_finalization
Jan 28, 2019
Merged

Fixed multiple blocks finalization#1601
rphmeier merged 1 commit intomasterfrom
fix_multiple_finalization

Conversation

@svyatonik
Copy link
Contributor

Seems like it has been broken recently when atomic operations were introduced. So when GRANDPA finalizes several blocks at once they're enumerated here and when 2nd block is finalized, this check fails, because meta still contain previous best-finalized block.

I've also noticed that now we can't import block + finalize it in single operation, because commit first tries to finalize not-yet-inserted block. Also seems like there could be an issue with internal Backend' state because Backend' meta is updated first and then we could fail for example here => in-memory meta will be updated, while the database will not. So probably we need to do some refactoring (like introducing in-memory snapshots of Backend' state or separating finalize and import operations) to avoid issues like these in future. But I'm not sure if we need to support atomic import-and-finalize operations. If there's anyone with this knowledge, please let me know - I'll file an issue(s) for that ^^^.

@svyatonik svyatonik added A0-please_review Pull request needs code review. M4-core labels Jan 28, 2019
@svyatonik
Copy link
Contributor Author

Also probably it is better to replace in_mem backend in 'big' tests (that are using test_client) with a in-memory-database backend here, because in_mem backend is too specific && not used in production

@rphmeier
Copy link
Contributor

rphmeier commented Jan 28, 2019

Filed #1603 for improving the tests.

We will want to support atomic import-and-finalize ops, definitely

@rphmeier rphmeier added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 28, 2019
@rphmeier rphmeier requested a review from sorpaas January 28, 2019 16:18
@rphmeier rphmeier merged commit 72499f2 into master Jan 28, 2019
@rphmeier rphmeier deleted the fix_multiple_finalization branch January 28, 2019 17:16
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants