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

Remove snowball.Initialize and snowball.Factory #2104

Merged
merged 7 commits into from
Sep 27, 2023
Merged

Remove snowball.Initialize and snowball.Factory #2104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c75ed6
WIP remove Initiailize from consensus
Sep 27, 2023
dbfecb9
remove factories
Sep 27, 2023
4c2519e
fix tests
Sep 27, 2023
dc8c9f2
comment
Sep 27, 2023
e216f81
nit
Sep 27, 2023
6dd625d
fix formatting
Sep 27, 2023
faf79b3
fix test
Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions snow/consensus/snowball/consensus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ import (
type Consensus interface {
fmt.Stringer

// Takes in alpha, beta1, beta2, and the initial choice
Initialize(params Parameters, initialPreference ids.ID)

// Adds a new choice to vote on
Add(newChoice ids.ID)

Expand Down
4 changes: 2 additions & 2 deletions snow/consensus/snowball/consensus_performance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ func TestSnowballOptimized(t *testing.T) {

sampler.Seed(seed)
for i := 0; i < numNodes; i++ {
nBitwise.AddNode(&Tree{})
nBitwise.AddNode(NewTree)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's possible for us to pass parameters into NewTree and NewFlat so we can avoid passing in this function directly

}

sampler.Seed(seed)
for i := 0; i < numNodes; i++ {
nNaive.AddNode(&Flat{})
nNaive.AddNode(NewFlat)
}

numRounds := 0
Expand Down
6 changes: 3 additions & 3 deletions snow/consensus/snowball/consensus_reversibility_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,19 @@ func TestSnowballGovernance(t *testing.T) {

sampler.Seed(seed)
for i := 0; i < numRed; i++ {
nBitwise.AddNodeSpecificColor(&Tree{}, 0, []int{1})
nBitwise.AddNodeSpecificColor(NewTree, 0, []int{1})
}

for _, node := range nBitwise.nodes {
require.Equal(nBitwise.colors[0], node.Preference())
}

for i := 0; i < numNodes-numByzantine-numRed; i++ {
nBitwise.AddNodeSpecificColor(&Tree{}, 1, []int{0})
nBitwise.AddNodeSpecificColor(NewTree, 1, []int{0})
}

for i := 0; i < numByzantine; i++ {
nBitwise.AddNodeSpecificColor(&Byzantine{}, 1, []int{0})
nBitwise.AddNodeSpecificColor(NewByzantine, 1, []int{0})
}

for !nBitwise.Finalized() {
Expand Down
10 changes: 6 additions & 4 deletions snow/consensus/snowball/consensus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,18 @@ var (
_ Consensus = (*Byzantine)(nil)
)

func NewByzantine(_ Parameters, choice ids.ID) Consensus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of the unused parameter here? Looks like we only had it previous for interface signature

return &Byzantine{
preference: choice,
}
}

// Byzantine is a naive implementation of a multi-choice snowball instance
type Byzantine struct {
// Hardcode the preference
preference ids.ID
}

func (b *Byzantine) Initialize(_ Parameters, choice ids.ID) {
b.preference = choice
}

func (*Byzantine) Add(ids.ID) {}

func (b *Byzantine) Preference() ids.ID {
Expand Down
9 changes: 0 additions & 9 deletions snow/consensus/snowball/factory.go
View file
Open in desktop

This file was deleted.

21 changes: 7 additions & 14 deletions snow/consensus/snowball/flat.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@ import (
"github.com/ava-labs/avalanchego/utils/bag"
)

var (
_ Factory = (*FlatFactory)(nil)
_ Consensus = (*Flat)(nil)
)

// FlatFactory implements Factory by returning a flat struct
type FlatFactory struct{}
var _ Consensus = (*Flat)(nil)

func (FlatFactory) New() Consensus {
return &Flat{}
func NewFlat(params Parameters, choice ids.ID) Consensus {
f := &Flat{
params: params,
}
f.nnarySnowball.Initialize(params.BetaVirtuous, params.BetaRogue, choice)
Copy link
Contributor

Choose a reason for hiding this comment

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

q: why not dropping this Initialize as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is being done in a followup

return f
}

// Flat is a naive implementation of a multi-choice snowball instance
Expand All @@ -29,11 +27,6 @@ type Flat struct {
params Parameters
}

func (f *Flat) Initialize(params Parameters, choice ids.ID) {
f.nnarySnowball.Initialize(params.BetaVirtuous, params.BetaRogue, choice)
f.params = params
}

func (f *Flat) RecordPoll(votes bag.Bag[ids.ID]) bool {
if pollMode, numVotes := votes.Mode(); numVotes >= f.params.Alpha {
f.RecordSuccessfulPoll(pollMode)
Expand Down
3 changes: 1 addition & 2 deletions snow/consensus/snowball/flat_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ func TestFlat(t *testing.T) {
params := Parameters{
K: 2, Alpha: 2, BetaVirtuous: 1, BetaRogue: 2,
}
f := Flat{}
f.Initialize(params, Red)
f := NewFlat(params, Red)
f.Add(Green)
f.Add(Blue)

Expand Down
42 changes: 27 additions & 15 deletions snow/consensus/snowball/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"github.com/ava-labs/avalanchego/utils/sampler"
)

type newConsensusFunc func(params Parameters, choice ids.ID) Consensus

type Network struct {
params Parameters
colors []ids.ID
Expand All @@ -26,34 +28,44 @@ func (n *Network) Initialize(params Parameters, numColors int) {
}
}

func (n *Network) AddNode(sb Consensus) {
func (n *Network) AddNode(newConsensusFunc newConsensusFunc) Consensus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for the caller to just give us an initialized Consensus instance instead of this function result?

s := sampler.NewUniform()
s.Initialize(uint64(len(n.colors)))
indices, _ := s.Sample(len(n.colors))
sb.Initialize(n.params, n.colors[int(indices[0])])

consensus := newConsensusFunc(n.params, n.colors[int(indices[0])])
for _, index := range indices[1:] {
sb.Add(n.colors[int(index)])
consensus.Add(n.colors[int(index)])
}

n.nodes = append(n.nodes, sb)
if !sb.Finalized() {
n.running = append(n.running, sb)
n.nodes = append(n.nodes, consensus)
if !consensus.Finalized() {
n.running = append(n.running, consensus)
}

return consensus
}

// AddNodeSpecificColor adds [sb] to the network which will initially prefer
// [initialPreference] and additionally adds each of the specified [options] to
// consensus.
func (n *Network) AddNodeSpecificColor(sb Consensus, initialPreference int, options []int) {
sb.Initialize(n.params, n.colors[initialPreference])
// AddNodeSpecificColor adds a new consensus instance to the network which will
// initially prefer [initialPreference] and additionally adds each of the
// specified [options] to consensus.
func (n *Network) AddNodeSpecificColor(
newConsensusFunc newConsensusFunc,
initialPreference int,
options []int,
) Consensus {
consensus := newConsensusFunc(n.params, n.colors[initialPreference])

for _, i := range options {
sb.Add(n.colors[i])
consensus.Add(n.colors[i])
}

n.nodes = append(n.nodes, sb)
if !sb.Finalized() {
n.running = append(n.running, sb)
n.nodes = append(n.nodes, consensus)
if !consensus.Finalized() {
n.running = append(n.running, consensus)
}

return consensus
}

// Finalized returns true iff every node added to the network has finished
Expand Down
33 changes: 14 additions & 19 deletions snow/consensus/snowball/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,26 @@ import (
)

var (
_ Factory = (*TreeFactory)(nil)
_ Consensus = (*Tree)(nil)
_ node = (*unaryNode)(nil)
_ node = (*binaryNode)(nil)
)

// TreeFactory implements Factory by returning a tree struct
type TreeFactory struct{}
func NewTree(params Parameters, choice ids.ID) Consensus {
snowball := &unarySnowball{}
snowball.Initialize(params.BetaVirtuous)

func (TreeFactory) New() Consensus {
return &Tree{}
t := &Tree{
params: params,
}
t.node = &unaryNode{
tree: t,
preference: choice,
commonPrefix: ids.NumBits, // The initial state has no conflicts
snowball: snowball,
}

return t
}

// Tree implements the snowball interface by using a modified patricia tree.
Expand All @@ -46,20 +55,6 @@ type Tree struct {
shouldReset bool
}

func (t *Tree) Initialize(params Parameters, choice ids.ID) {
t.params = params

snowball := &unarySnowball{}
snowball.Initialize(params.BetaVirtuous)

t.node = &unaryNode{
tree: t,
preference: choice,
commonPrefix: ids.NumBits, // The initial state has no conflicts
snowball: snowball,
}
}

func (t *Tree) Add(choice ids.ID) {
prefix := t.node.DecidedPrefix()
// Make sure that we haven't already decided against this new id
Expand Down
Loading