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

[Consensus] Approval Processing Core -> Sealing Core #736

Merged
merged 84 commits into from
Jun 4, 2021

Conversation

durkmurder
Copy link
Member

This PR implements next issues:

Main changes:

  • Sealing engine was replaced with new event driven approval processing engine
  • Matching engine is back again and performs pending receipts processing
  • New distributor for hotstuff events. Performs delivery of finalization events to subscribers.
  • Big cleanup of not needed modules, mempools, etc

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2021

Codecov Report

Merging #736 (70cd038) into master (4682be4) will decrease coverage by 0.25%.
The diff coverage is 57.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #736      +/-   ##
==========================================
- Coverage   56.64%   56.38%   -0.26%     
==========================================
  Files         423      423              
  Lines       25072    24783     -289     
==========================================
- Hits        14203    13975     -228     
+ Misses       8963     8913      -50     
+ Partials     1906     1895      -11     
Flag Coverage Δ
unittests 56.38% <57.46%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/consensus/approvals/assignment_collector_tree.go 0.00% <0.00%> (-74.74%) ⬇️
...e/consensus/approvals/incorporated_result_seals.go 0.00% <0.00%> (ø)
engine/consensus/approvals/request_tracker.go 76.74% <ø> (ø)
model/flow/incorporated_result.go 61.11% <ø> (+25.21%) ⬆️
module/mempool/consensus/execution_tree.go 72.97% <ø> (ø)
engine/consensus/matching/core.go 52.43% <52.43%> (ø)
engine/consensus/sealing/engine.go 51.97% <59.37%> (+14.57%) ⬆️
engine/consensus/matching/engine.go 60.56% <60.56%> (ø)
engine/consensus/approvals/approval_collector.go 78.84% <61.53%> (-8.66%) ⬇️
module/mempool/stdmap/pending_receipts.go 75.00% <63.63%> (+0.75%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4682be4...70cd038. Read the comment docs.

durkmurder and others added 5 commits June 1, 2021 14:46
• renamed method `AggregatedSignatures.CollectChunksWithMissingApprovals` to `ChunksWithoutAggregatedSignature()`
• shifted the location of a few lines of code
• renamed internal variable
…ls are required;

• ApprovalCollector: using consistently uint64 to refer to chunk indices and related quantities;
• sealing.Engine: separated logic for creating inbound queues for trusted inputs from message handler for untrusted inputs;
Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

🎉 This PR is finally ready to be merged 🥳 🍾

I provided some minor revisions in PR #774 targeting this branch. Mostly revisions of goDoc and and inline comments and a tiny bit of polishing. The only noteworthy algorithmic addition is the implementation of my comment below.

This really turned out nicely. Great work. Thanks

numberOfChunks: numberOfChunks,
chunkCollectors: chunkCollectors,
aggregatedSignatures: NewAggregatedSignatures(uint64(numberOfChunks)),
seals: seals,
}
Copy link
Member

@AlexHentschel AlexHentschel Jun 2, 2021

Choose a reason for hiding this comment

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

I am wondering how this code will behave, when requiredApprovalsForSealConstruction = 0. In this case, sealing should proceed, even if all verifiers are offline.

My theory on how the current implementation will behave is:

  • the only code paths reaching ApprovalCollector.SealResult() are:

    • emergency sealing
      func (ac *AssignmentCollector) CheckEmergencySealing(finalizedBlockHeight uint64) error {
      for _, collector := range ac.allCollectors() {
      sealable := ac.emergencySealable(collector, finalizedBlockHeight)
      if sealable {
      err := collector.SealResult()
    • processing an approval
      func (c *ApprovalCollector) ProcessApproval(approval *flow.ResultApproval) error {
      chunkIndex := approval.Body.ChunkIndex
      if chunkIndex >= uint64(len(c.chunkCollectors)) {
      return engine.NewInvalidInputErrorf("approval collector chunk index out of range: %v", chunkIndex)
      }
      // there is no need to process approval if we have already enough info for sealing
      if c.aggregatedSignatures.HasSignature(chunkIndex) {
      return nil
      }
      collector := c.chunkCollectors[chunkIndex]
      aggregatedSignature, collected := collector.ProcessApproval(approval)
      if !collected {
      return nil
      }
      approvedChunks := c.aggregatedSignatures.PutSignature(chunkIndex, aggregatedSignature)
      if approvedChunks < c.numberOfChunks {
      return nil // still missing approvals for some chunks
      }
      return c.SealResult()
  • Hence, if all verifiers are offline, we will not generate seals until the result is emergency sealed, even if requiredApprovalsForSealConstruction = 0.

I think the better behaviour would be produce a candidate seal right away, as soon as we construct the ApprovalCollector

This is already implemented in PR #774

durkmurder and others added 17 commits June 2, 2021 03:20
…ine_-_suggestions_pt4

PR 736: suggestions pt4
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
…collector-tree

[Consensus] Repopulating of assignment collectors tree
@durkmurder durkmurder merged commit f1bc65c into master Jun 4, 2021
@durkmurder durkmurder deleted the yurii/5417-approval-processing-engine branch June 4, 2021 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants