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

fix(lib/blocktree): reimplement BestBlockHash to take into account primary blocks in fork choice rule #2254

Merged
merged 14 commits into from
Feb 1, 2022
2 changes: 1 addition & 1 deletion dot/state/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ func (bs *BlockState) BestBlockHash() common.Hash {
return common.Hash{}
}

return bs.bt.DeepestBlockHash()
return bs.bt.BestBlockHash()
}

// BestBlockHeader returns the block header of the current head of the chain
Expand Down
24 changes: 24 additions & 0 deletions dot/types/babe.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,27 @@ func GetSlotFromHeader(header *Header) (uint64, error) {

return slotNumber, nil
}

// IsPrimary returns true if the block was authored in a primary slot, false otherwise.
func IsPrimary(header *Header) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can make this a receiver function on Header? Header.Primary() (bool, error)

if len(header.Digest.Types) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a nil check for header here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I can add it, hopefully doesn't happen though lol

return false, fmt.Errorf("chain head missing digest")
}

preDigest, ok := header.Digest.Types[0].Value().(PreRuntimeDigest)
if !ok {
return false, fmt.Errorf("first digest item is not pre-digest")
}

digest, err := DecodeBabePreDigest(preDigest.Data)
if err != nil {
return false, fmt.Errorf("cannot decode BabePreDigest from pre-digest: %s", err)
}

switch digest.(type) {
case BabePrimaryPreDigest:
return true, nil
default:
return false, nil
}
}
52 changes: 25 additions & 27 deletions lib/blocktree/blocktree.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,18 @@ func (bt *BlockTree) AddBlock(header *types.Header, arrivalTime time.Time) error
return errUnexpectedNumber
}

isPrimary, err := types.IsPrimary(header)
if err != nil {
return fmt.Errorf("failed to check if block was primary: %w", err)
}

n := &node{
hash: header.Hash(),
parent: parent,
children: []*node{},
number: number,
arrivalTime: arrivalTime,
isPrimary: isPrimary,
}

parent.addChild(n)
Expand Down Expand Up @@ -188,18 +194,6 @@ func (bt *BlockTree) String() string {
return fmt.Sprintf("%s\n%s\n", metadata, tree.Print())
}

// longestPath returns the path from the root to the deepest leaf in the blocktree
func (bt *BlockTree) longestPath() []*node {
dl := bt.deepestLeaf()
var path []*node
for curr := dl; ; curr = curr.parent {
path = append([]*node{curr}, path...)
if curr.parent == nil {
return path
}
}
}

// subChain returns the path from the node with Hash start to the node with Hash end
func (bt *BlockTree) subChain(start, end Hash) ([]*node, error) {
sn := bt.getNode(start)
Expand Down Expand Up @@ -230,27 +224,31 @@ func (bt *BlockTree) SubBlockchain(start, end Hash) ([]Hash, error) {

}

// deepestLeaf returns the deepest leaf in the block tree.
func (bt *BlockTree) deepestLeaf() *node {
return bt.leaves.deepestLeaf()
// best returns the best node in the block tree using the fork choice rule.
func (bt *BlockTree) best() *node {
return bt.leaves.bestBlock()
}

// DeepestBlockHash returns the hash of the deepest block in the blocktree
// If there is multiple deepest blocks, it returns the one with the earliest arrival time.
func (bt *BlockTree) DeepestBlockHash() Hash {
// BestBlockHash returns the hash of the block that is considered "best" based on the
// fork-choice rule. It returns the head of the chain with the most primary blocks.
// If there are multiple chains with the same number of primaries, it returns the one
// with the highest head number.
// If there are multiple chains with the same number of primaries and the same height,
// it returns the one with the head block that arrived the earliest.
func (bt *BlockTree) BestBlockHash() Hash {
bt.RLock()
defer bt.RUnlock()

if bt.leaves == nil {
// this shouldn't happen
Copy link
Contributor

Choose a reason for hiding this comment

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

should we return an error in this case?

return Hash{}
}

deepest := bt.leaves.deepestLeaf()
if deepest == nil {
return Hash{}
if len(bt.root.children) == 0 {
return bt.root.hash
}

return deepest.hash
return bt.best().hash
}

// IsDescendantOf returns true if the child is a descendant of parent, false otherwise.
Expand Down Expand Up @@ -326,13 +324,13 @@ func (bt *BlockTree) GetHashByNumber(num *big.Int) (common.Hash, error) {
bt.RLock()
defer bt.RUnlock()

deepest := bt.leaves.deepestLeaf()
if deepest.number.Cmp(num) == -1 {
best := bt.leaves.bestBlock()
if best.number.Cmp(num) == -1 {
return common.Hash{}, ErrNumGreaterThanHighest
}

if deepest.number.Cmp(num) == 0 {
return deepest.hash, nil
if best.number.Cmp(num) == 0 {
return best.hash, nil
}

if bt.root.number.Cmp(num) == 1 {
Expand All @@ -343,7 +341,7 @@ func (bt *BlockTree) GetHashByNumber(num *big.Int) (common.Hash, error) {
return bt.root.hash, nil
}

curr := deepest.parent
curr := best.parent
for {
if curr == nil {
return common.Hash{}, ErrNodeNotFound
Expand Down
Loading