Make snowman use snowflake directly instead of snowball #3403
Conversation
yacovm
force-pushed
the
removeSnowball
branch
2 times, most recently
from
484d447 to
e3597d8
Compare
michaelkaplan13
requested review from
aaronbuchwald,
ceyonur and
joshua-kim
| sb := newBinarySnowball(alphaPreference, terminationConditions, red) | ||
| require.Equal(red, sb.Preference()) | ||
| require.False(sb.Finalized()) | ||
| sf := newBinarySnowflake(alphaPreference, terminationConditions, red) |
There was a problem hiding this comment.
Why is this test modified? It seems to be a duplicate of TestBinarySnowflake now.
There was a problem hiding this comment.
I'm not sure why this file is being modified. We aren't changing the code that it was previously testing... If we feel like the binarySnowflake struct needs additional coverage shouldn't we be adding new tests to the binary_snowflake_test.go file rather than here?
There was a problem hiding this comment.
Because we aren't deleting the code that this was testing, nnary_snowball.go, why are we deleting the tests?
There was a problem hiding this comment.
Because we aren't deleting the code that this was testing, unary_snowball.go, why are we deleting the tests?
snow/consensus/snowman/mixed_test.go
Outdated
| var sm Consensus | ||
| if i%2 == 0 { | ||
| factory := TopologicalFactory{factory: snowball.SnowflakeFactory} | ||
| sm = factory.New() | ||
| } else { | ||
| factory := TopologicalFactory{factory: snowball.SnowballFactory} | ||
| sm = factory.New() | ||
| } | ||
|
|
||
| require.NoError(n.AddNode(t, sm)) |
There was a problem hiding this comment.
nit: Seems like we can cleanup the if block a bit
| var sm Consensus | |
| if i%2 == 0 { | |
| factory := TopologicalFactory{factory: snowball.SnowflakeFactory} | |
| sm = factory.New() | |
| } else { | |
| factory := TopologicalFactory{factory: snowball.SnowballFactory} | |
| sm = factory.New() | |
| } | |
| require.NoError(n.AddNode(t, sm)) | |
| var sbFactory snowball.Factory | |
| if i%2 == 0 { | |
| sbFactory = snowball.SnowflakeFactory | |
| } else { | |
| sbFactory = snowball.SnowballFactory | |
| } | |
| factory := TopologicalFactory{factory: sbFactory} | |
| sm := factory.New() | |
| require.NoError(n.AddNode(t, sm)) |
snow/consensus/snowball/tree_test.go
Outdated
| // * | ||
| // 1/ \ | ||
| // R * | ||
| // / \ | ||
| // G B |
There was a problem hiding this comment.
What do the numbers on the edges mean? It seems like they are referencing Snowball counters but we are using snowflake here and not snowball.
yacovm
force-pushed
the
removeSnowball
branch
3 times, most recently
from
1c161b2 to
be4ccba
Compare
Snowball was introduced in order to improve the stability of snowflake, by taking into account the history when returning the preference. However, if deployed with a configuration where the preference is small enough, and a network partition causes a 50-50 split of the stake, it can actually backfire, as described in 6e1a905. Additionally, latest research [1] points out that snowflake suffices for snowman. This commit removes snowball from snowman and adds a test that simulates a mixed network which runs snowman with and without snowball, and ensures that a mixed network still converges. [1] https://arxiv.org/abs/2404.14250 Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
yacovm
force-pushed
the
removeSnowball
branch
from
be4ccba to
90fd999
Compare
StephenButtolph
left a comment
StephenButtolph
left a comment
There was a problem hiding this comment.
Code changes LGTM. Going to hold off on approving/merging until we've canaried this out on Fuji + Mainnet
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
3 participants
Why this should be merged
Snowball was introduced in order to improve the stability of snowflake, by taking into account the history when returning the preference.
However, if deployed with a configuration where the preference is small enough, and a network partition causes a 50-50 split of the stake, it can actually backfire, as described in 6e1a905.
Additionally, latest research [1] points out that snowflake suffices for snowman.
[1] https://arxiv.org/abs/2404.14250
How this works
This commit removes snowball from snowman by substituting the snowball factory snowman uses, with a snowflake factory.
How this was tested
Added a test that simulates a mixed network which runs snowman with and without snowball, and ensures that a mixed network still converges.
Also ran a modified node on Fuji and monitored its consensus related metrics, and it seemed functioning well.