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.

Sign up for GitHub

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

Jump to bottom

Merge big subdags in pick virtual parents #1574

Merged
merged 7 commits into from
Mar 3, 2021
Merged

Merge big subdags in pick virtual parents #1574

merged 7 commits into from
Mar 3, 2021

Conversation

elichai
Copy link
Member

@elichai elichai commented Mar 2, 2021

This should:

  1. Improve the performance of processing blocks when there's a big subdag.
  2. Slowly merge said subdag by slowly climbing down the merge set

@elichai elichai added this to the 0.9.0 milestone Mar 2, 2021
@elichai elichai added this to In progress in Mainnet via automation Mar 2, 2021
@codecov
Copy link

codecov bot commented Mar 2, 2021 •
edited

Codecov Report

Merging #1574 (ae08761) into v0.9.0-dev (2016462) will decrease coverage by 1.69%.
The diff coverage is 35.60%.

Impacted file tree graph

@@              Coverage Diff               @@
##           v0.9.0-dev    #1574      +/-   ##
==============================================
- Coverage       61.67%   59.98%   -1.70%     
==============================================
  Files             512      515       +3     
  Lines           19503    20319     +816     
==============================================
+ Hits            12029    12188     +159     
- Misses           5756     6200     +444     
- Partials         1718     1931     +213
Impacted Files Coverage Δ
app/app.go 0.00% <0.00%> (ø)
app/appmessage/rpc_get_block.go 0.00% <ø> (ø)
app/appmessage/rpc_get_block_dag_info.go 0.00% <ø> (ø)
app/protocol/flowcontext/errors.go 42.85% <0.00%> (ø)
...rotocol/flows/blockrelay/handle_request_headers.go 68.29% <ø> (ø)
app/protocol/flows/handshake/handshake.go 45.83% <0.00%> (ø)
...ws/transactionrelay/handle_relayed_transactions.go 48.35% <0.00%> (ø)
app/protocol/protocol.go 65.30% <0.00%> (-3.76%) ⬇️
app/rpc/rpchandlers/get_block_dag_info.go 0.00% <0.00%> (ø)
app/rpc/rpchandlers/submit_block.go 35.00% <0.00%> (-1.85%) ⬇️
... and 118 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 18274c2...ae08761. Read the comment docs.

svarogg
svarogg requested changes Mar 2, 2021
View reviewed changes
domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
selectedVirtualParents.Add(candidate)
mergeSetSize += mergeSetIncrease
log.Tracef("Added block %s to the virtual parents set", candidate)
// Remove all candidates in the future of newCandidate (https://github.com/golang/go/wiki/SliceTricks#filter-in-place)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be nice if this was a separate function

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm implementing a HashSlice type now, can be replaced later

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract it nontheless. It detracts from the core logic of the current function.

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
@@ -154,58 +178,55 @@ func (csm *consensusStateManager) selectVirtualSelectedParent(
}

func (csm *consensusStateManager) mergeSetIncrease(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment explaining the newCandidate thing

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
@@ -154,58 +178,55 @@ func (csm *consensusStateManager) selectVirtualSelectedParent(
}

func (csm *consensusStateManager) mergeSetIncrease(
candidate *externalapi.DomainHash, selectedVirtualParents hashset.HashSet) (uint64, error) {
candidate *externalapi.DomainHash, selectedVirtualParents []*externalapi.DomainHash, mergeSetSize uint64) (canBeParent bool, newCandidate *externalapi.DomainHash, mergeSetIncrease uint64, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please break down line

stasatdaglabs
stasatdaglabs requested changes Mar 2, 2021
View reviewed changes
domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go
@@ -46,7 +45,12 @@ func (csm *consensusStateManager) pickVirtualParents(tips []*externalapi.DomainH
end--
}
}
// Limit to 30 candidates, that way we don't go over thousands of tips when the network isn't healthy.
if len(candidates) > int(csm.maxBlockParents)*3 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract maxBlockParent*3 to a constant (in constants.go) and add a comment over it explaining why that number was chosen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well there's no reason it was chosen other than my whims

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even more so.

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
selectedVirtualParents.Add(candidate)
mergeSetSize += mergeSetIncrease
log.Tracef("Added block %s to the virtual parents set", candidate)
// Remove all candidates in the future of newCandidate (https://github.com/golang/go/wiki/SliceTricks#filter-in-place)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract it nontheless. It detracts from the core logic of the current function.

svarogg
svarogg requested changes Mar 3, 2021
View reviewed changes
domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go
@@ -46,7 +45,12 @@ func (csm *consensusStateManager) pickVirtualParents(tips []*externalapi.DomainH
end--
}
}
// Limit to 30 candidates, that way we don't go over thousands of tips when the network isn't healthy.
if len(candidates) > int(csm.maxBlockParents)*3 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even more so.

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
return nil, err
}
candidates = append(candidates, newCandidate)
log.Debugf("Cannot add block %s, instead added new candidate: %s", candidate, newCandidate)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd more specific in the log, a.k.a. "block %s increases merge set too much, instead adding it's ancestor %s"

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
}
}
}
log.Tracef("The virtual parents resolved to be: %s", selectedVirtualParents)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put this in Debugf

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go Outdated
onEnd := logger.LogAndMeasureExecutionTime(log, "mergeSetIncrease")
defer onEnd()

visited := hashset.New()
queue := csm.dagTraversalManager.NewDownHeap()
err := queue.Push(candidate)
// Start with the parents in the queue as we already know the candidate isn't an ancestor of the parents.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You use "parents" for two different things
I'd revise to "Start with candidate's parents ... we already know the candidate isn't an ancestor of selectedVirtualParents"

Copy link
Member Author

Choose a reason for hiding this comment

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

ops

@stasatdaglabs stasatdaglabs merged commit 7909480 into v0.9.0-dev Mar 3, 2021
Mainnet automation moved this from In progress to Done Mar 3, 2021
@stasatdaglabs stasatdaglabs deleted the merge-big-subdags branch March 3, 2021 14:17
@elichai elichai linked an issue Mar 3, 2021 that may be closed by this pull request
The virtual ignores a whole sub-DAG if its tip makes the virtual violate a consensus rule #1517
Open
@elichai elichai mentioned this pull request Mar 3, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svarogg svarogg svarogg approved these changes

@stasatdaglabs stasatdaglabs stasatdaglabs approved these changes

Assignees
No one assigned
Labels
consensus performance
Projects
No open projects
Mainnet
Done
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

The virtual ignores a whole sub-DAG if its tip makes the virtual violate a consensus rule
3 participants
@elichai @svarogg @stasatdaglabs