diff --git a/common/blacklist.canoto.go b/common/blacklist.canoto.go index 4ec91732..6e3c2565 100644 --- a/common/blacklist.canoto.go +++ b/common/blacklist.canoto.go @@ -23,13 +23,15 @@ const ( var _ = io.ErrUnexpectedEOF const ( - canotoNumber_Blacklist__NodeCount = 1 - canotoNumber_Blacklist__SuspectedNodes = 2 - canotoNumber_Blacklist__Updates = 3 - - canotoTag_Blacklist__NodeCount = "\x08" // canoto.Tag(canotoNumber_Blacklist__NodeCount, canoto.Varint) - canotoTag_Blacklist__SuspectedNodes = "\x12" // canoto.Tag(canotoNumber_Blacklist__SuspectedNodes, canoto.Len) - canotoTag_Blacklist__Updates = "\x1a" // canoto.Tag(canotoNumber_Blacklist__Updates, canoto.Len) + canotoNumber_Blacklist__nodeCount = 1 + canotoNumber_Blacklist__maxBlacklistedOrbits = 2 + canotoNumber_Blacklist__suspectedNodes = 3 + canotoNumber_Blacklist__updates = 4 + + canotoTag_Blacklist__nodeCount = "\x08" // canoto.Tag(canotoNumber_Blacklist__nodeCount, canoto.Varint) + canotoTag_Blacklist__maxBlacklistedOrbits = "\x10" // canoto.Tag(canotoNumber_Blacklist__maxBlacklistedOrbits, canoto.Varint) + canotoTag_Blacklist__suspectedNodes = "\x1a" // canoto.Tag(canotoNumber_Blacklist__suspectedNodes, canoto.Len) + canotoTag_Blacklist__updates = "\x22" // canoto.Tag(canotoNumber_Blacklist__updates, canoto.Len) ) type canotoData_Blacklist struct { @@ -44,15 +46,21 @@ func (*Blacklist) CanotoSpec(types ...reflect.Type) *canoto.Spec { Name: "Blacklist", Fields: []canoto.FieldType{ { - FieldNumber: canotoNumber_Blacklist__NodeCount, - Name: "NodeCount", + FieldNumber: canotoNumber_Blacklist__nodeCount, + Name: "nodeCount", + OneOf: "", + TypeUint: canoto.SizeOf(zero.nodeCount), + }, + { + FieldNumber: canotoNumber_Blacklist__maxBlacklistedOrbits, + Name: "maxBlacklistedOrbits", OneOf: "", - TypeUint: canoto.SizeOf(zero.NodeCount), + TypeUint: canoto.SizeOf(zero.maxBlacklistedOrbits), }, canoto.FieldTypeFromField( - /*type inference:*/ (canoto.MakeEntryNilPointer(zero.SuspectedNodes)), - /*FieldNumber: */ canotoNumber_Blacklist__SuspectedNodes, - /*Name: */ "SuspectedNodes", + /*type inference:*/ (canoto.MakeEntryNilPointer(zero.suspectedNodes)), + /*FieldNumber: */ canotoNumber_Blacklist__suspectedNodes, + /*Name: */ "suspectedNodes", /*FixedLength: */ 0, /*Repeated: */ true, /*OneOf: */ "", @@ -60,9 +68,9 @@ func (*Blacklist) CanotoSpec(types ...reflect.Type) *canoto.Spec { /*types: */ types, ), canoto.FieldTypeFromField( - /*type inference:*/ (canoto.MakeEntryNilPointer(zero.Updates)), - /*FieldNumber: */ canotoNumber_Blacklist__Updates, - /*Name: */ "Updates", + /*type inference:*/ (canoto.MakeEntryNilPointer(zero.updates)), + /*FieldNumber: */ canotoNumber_Blacklist__updates, + /*Name: */ "updates", /*FixedLength: */ 0, /*Repeated: */ true, /*OneOf: */ "", @@ -107,18 +115,29 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { } switch field { - case canotoNumber_Blacklist__NodeCount: + case canotoNumber_Blacklist__nodeCount: + if wireType != canoto.Varint { + return canoto.ErrUnexpectedWireType + } + + if err := canoto.ReadUint(&r, &c.nodeCount); err != nil { + return err + } + if canoto.IsZero(c.nodeCount) { + return canoto.ErrZeroValue + } + case canotoNumber_Blacklist__maxBlacklistedOrbits: if wireType != canoto.Varint { return canoto.ErrUnexpectedWireType } - if err := canoto.ReadUint(&r, &c.NodeCount); err != nil { + if err := canoto.ReadUint(&r, &c.maxBlacklistedOrbits); err != nil { return err } - if canoto.IsZero(c.NodeCount) { + if canoto.IsZero(c.maxBlacklistedOrbits) { return canoto.ErrZeroValue } - case canotoNumber_Blacklist__SuspectedNodes: + case canotoNumber_Blacklist__suspectedNodes: if wireType != canoto.Len { return canoto.ErrUnexpectedWireType } @@ -134,13 +153,13 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { r.Unsafe = originalUnsafe // Count the number of additional entries after the first entry. - countMinus1, err := canoto.CountBytes(r.B, canotoTag_Blacklist__SuspectedNodes) + countMinus1, err := canoto.CountBytes(r.B, canotoTag_Blacklist__suspectedNodes) if err != nil { return err } - c.SuspectedNodes = canoto.MakeSlice(c.SuspectedNodes, countMinus1+1) - field := c.SuspectedNodes + c.suspectedNodes = canoto.MakeSlice(c.suspectedNodes, countMinus1+1) + field := c.suspectedNodes additionalField := field[1:] if len(msgBytes) != 0 { remainingBytes := r.B @@ -153,7 +172,7 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { // Read the rest of the entries, stripping the tag each time. for i := range additionalField { - r.B = r.B[len(canotoTag_Blacklist__SuspectedNodes):] + r.B = r.B[len(canotoTag_Blacklist__suspectedNodes):] r.Unsafe = true if err := canoto.ReadBytes(&r, &msgBytes); err != nil { return err @@ -170,7 +189,7 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { } r.B = remainingBytes } - case canotoNumber_Blacklist__Updates: + case canotoNumber_Blacklist__updates: if wireType != canoto.Len { return canoto.ErrUnexpectedWireType } @@ -186,13 +205,13 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { r.Unsafe = originalUnsafe // Count the number of additional entries after the first entry. - countMinus1, err := canoto.CountBytes(r.B, canotoTag_Blacklist__Updates) + countMinus1, err := canoto.CountBytes(r.B, canotoTag_Blacklist__updates) if err != nil { return err } - c.Updates = canoto.MakeSlice(c.Updates, countMinus1+1) - field := c.Updates + c.updates = canoto.MakeSlice(c.updates, countMinus1+1) + field := c.updates additionalField := field[1:] if len(msgBytes) != 0 { remainingBytes := r.B @@ -205,7 +224,7 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { // Read the rest of the entries, stripping the tag each time. for i := range additionalField { - r.B = r.B[len(canotoTag_Blacklist__Updates):] + r.B = r.B[len(canotoTag_Blacklist__updates):] r.Unsafe = true if err := canoto.ReadBytes(&r, &msgBytes); err != nil { return err @@ -240,7 +259,7 @@ func (c *Blacklist) UnmarshalCanotoFrom(r canoto.Reader) error { // 3. All custom fields are ValidCanoto. func (c *Blacklist) ValidCanoto() bool { { - field := c.SuspectedNodes + field := c.suspectedNodes for i := range field { if !(&field[i]).ValidCanoto() { return false @@ -248,7 +267,7 @@ func (c *Blacklist) ValidCanoto() bool { } } { - field := c.Updates + field := c.updates for i := range field { if !(&field[i]).ValidCanoto() { return false @@ -264,23 +283,26 @@ func (c *Blacklist) ValidCanoto() bool { // It is not safe to copy this struct concurrently. func (c *Blacklist) CalculateCanotoCache() { var size uint64 - if !canoto.IsZero(c.NodeCount) { - size += uint64(len(canotoTag_Blacklist__NodeCount)) + canoto.SizeUint(c.NodeCount) + if !canoto.IsZero(c.nodeCount) { + size += uint64(len(canotoTag_Blacklist__nodeCount)) + canoto.SizeUint(c.nodeCount) + } + if !canoto.IsZero(c.maxBlacklistedOrbits) { + size += uint64(len(canotoTag_Blacklist__maxBlacklistedOrbits)) + canoto.SizeUint(c.maxBlacklistedOrbits) } { - field := c.SuspectedNodes + field := c.suspectedNodes for i := range field { (&field[i]).CalculateCanotoCache() fieldSize := (&field[i]).CachedCanotoSize() - size += uint64(len(canotoTag_Blacklist__SuspectedNodes)) + canoto.SizeUint(fieldSize) + fieldSize + size += uint64(len(canotoTag_Blacklist__suspectedNodes)) + canoto.SizeUint(fieldSize) + fieldSize } } { - field := c.Updates + field := c.updates for i := range field { (&field[i]).CalculateCanotoCache() fieldSize := (&field[i]).CachedCanotoSize() - size += uint64(len(canotoTag_Blacklist__Updates)) + canoto.SizeUint(fieldSize) + fieldSize + size += uint64(len(canotoTag_Blacklist__updates)) + canoto.SizeUint(fieldSize) + fieldSize } } atomic.StoreUint64(&c.canotoData.size, size) @@ -321,22 +343,26 @@ func (c *Blacklist) MarshalCanoto() []byte { // // It is not safe to copy this struct concurrently. func (c *Blacklist) MarshalCanotoInto(w canoto.Writer) canoto.Writer { - if !canoto.IsZero(c.NodeCount) { - canoto.Append(&w, canotoTag_Blacklist__NodeCount) - canoto.AppendUint(&w, c.NodeCount) + if !canoto.IsZero(c.nodeCount) { + canoto.Append(&w, canotoTag_Blacklist__nodeCount) + canoto.AppendUint(&w, c.nodeCount) + } + if !canoto.IsZero(c.maxBlacklistedOrbits) { + canoto.Append(&w, canotoTag_Blacklist__maxBlacklistedOrbits) + canoto.AppendUint(&w, c.maxBlacklistedOrbits) } { - field := c.SuspectedNodes + field := c.suspectedNodes for i := range field { - canoto.Append(&w, canotoTag_Blacklist__SuspectedNodes) + canoto.Append(&w, canotoTag_Blacklist__suspectedNodes) canoto.AppendUint(&w, (&field[i]).CachedCanotoSize()) w = (&field[i]).MarshalCanotoInto(w) } } { - field := c.Updates + field := c.updates for i := range field { - canoto.Append(&w, canotoTag_Blacklist__Updates) + canoto.Append(&w, canotoTag_Blacklist__updates) canoto.AppendUint(&w, (&field[i]).CachedCanotoSize()) w = (&field[i]).MarshalCanotoInto(w) } diff --git a/common/blacklist.go b/common/blacklist.go index a438ff44..58f75c52 100644 --- a/common/blacklist.go +++ b/common/blacklist.go @@ -5,25 +5,33 @@ package common import ( "fmt" + "math" "slices" "strings" ) var ( - errBlacklistTooManyUpdates = fmt.Errorf("too many blacklist updates") - errBlacklistInvalidNodeIndex = fmt.Errorf("invalid node index in blacklist update") - errBlacklistNodeIndexAlreadyUpdated = fmt.Errorf("node index already updated") - errBlacklistNodeAlreadyBlacklisted = fmt.Errorf("node index already blacklisted") - errBlacklistNodeNotBlacklisted = fmt.Errorf("node index not blacklisted, cannot be redeemed") - errBlacklistInvalidUpdateType = fmt.Errorf("invalid blacklist update type") - errBlacklistInvalidNodeCount = fmt.Errorf("block contains an invalid blacklist node count") - errBlacklistInvalidUpdates = fmt.Errorf("block contains invalid blacklist updates") - errBlacklistInvalidBlacklist = fmt.Errorf("block contains an invalid blacklist") + errBlacklistTooManyUpdates = fmt.Errorf("too many blacklist updates") + errBlacklistInvalidNodeIndex = fmt.Errorf("invalid node index in blacklist update") + errBlacklistNodeIndexAlreadyUpdated = fmt.Errorf("node index already updated") + errBlacklistNodeAlreadyBlacklisted = fmt.Errorf("node index already blacklisted") + errBlacklistNodeNotBlacklisted = fmt.Errorf("node index not blacklisted, cannot be redeemed") + errBlacklistInvalidUpdateType = fmt.Errorf("invalid blacklist update type") + errBlacklistInvalidNodeCount = fmt.Errorf("block contains an invalid blacklist node count") + errBlacklistInvalidMaxBlacklistedOrbits = fmt.Errorf("block contains an invalid max blacklisted orbits") + errBlacklistInvalidUpdates = fmt.Errorf("block contains invalid blacklist updates") + errBlacklistInvalidBlacklist = fmt.Errorf("block contains an invalid blacklist") ) -// Orbit returns the total number of times the given node has been selected as leader +// GenesisMaxBlacklistedRounds is the max blacklisted rounds a blacklist starts with when the chain has no previous +// max blacklisted orbits to derive it from. +// It is configured to be 3 * 4 * 5 * 7 * 11 because the max blacklisted orbits is computed as rounds / nodeCount, +// so this value is divisible by common validator counts. +const GenesisMaxBlacklistedRounds = 3 * 4 * 5 * 7 * 11 + +// orbit returns the total number of times the given node has been selected as leader // from genesis up to and including the specified round. -func Orbit(round uint64, nodeIndex uint16, nodeCount uint16) uint64 { +func orbit(round uint64, nodeIndex uint16, nodeCount uint16) uint64 { inCycle := round % uint64(nodeCount) cycles := round / uint64(nodeCount) @@ -35,43 +43,141 @@ func Orbit(round uint64, nodeIndex uint16, nodeCount uint16) uint64 { } // Blacklist stores the state of the blacklist. -// It can be derived by applying the recorded Updates to the parent block's blacklist. +// It can be derived by applying the recorded updates to the parent block's blacklist. type Blacklist struct { - // NodeCount is the configuration of the blacklist. - NodeCount uint16 `canoto:"uint,1"` + // Configuration: Below are configuration fields that + // are present in every Blacklist. + + // nodeCount is the configuration of the blacklist. + nodeCount uint16 `canoto:"uint,1"` + + // maxBlacklistedOrbits is the maximum number of orbits a node can be blacklisted for. + // Each block builder may move it by at most one orbit relative to the parent block, + // towards the value in its local configuration, which allows it to be changed via a rolling upgrade. + maxBlacklistedOrbits uint16 `canoto:"uint,2"` - // SuspectedNodes is the list of nodes that are currently suspected. + // State: Below are fields corresponding to state. + // They are optional fields and may not be present + // in a Blacklist. + + // suspectedNodes is the list of nodes that are currently suspected. // it's the inner state of the blacklist. - SuspectedNodes SuspectedNodes `canoto:"repeated value,2"` + suspectedNodes suspectedNodes `canoto:"repeated value,3"` - // Updates is the list of modifications that a block builder is proposing + // updates is the list of modifications that a block builder is proposing // to perform to the blacklist. - Updates BlacklistUpdates `canoto:"repeated value,3"` + updates BlacklistUpdates `canoto:"repeated value,4"` canotoData canotoData_Blacklist } -func NewBlacklist(nodeCount uint16) Blacklist { +func NewBlacklist(nodeCount uint16, maxBlacklistedRounds uint16) Blacklist { + return Blacklist{ + nodeCount: nodeCount, + maxBlacklistedOrbits: maxBlacklistedOrbitsForRounds(nodeCount, maxBlacklistedRounds), + } +} + +// NodeCount returns the number of nodes the blacklist is configured for. +func (bl *Blacklist) NodeCount() uint16 { + return bl.nodeCount +} + +// MaxBlacklistedOrbits returns the maximum number of orbits a node can be blacklisted for. +func (bl *Blacklist) MaxBlacklistedOrbits() uint16 { + return bl.maxBlacklistedOrbits +} + +// maxBlacklistedOrbitsForRounds converts the given amount of rounds into whole orbits of the given node count. +// The result is at least one orbit. +func maxBlacklistedOrbitsForRounds(nodeCount uint16, maxBlacklistedRounds uint16) uint16 { + denominator := nodeCount + + // Protection against division by zero + if denominator == 0 { + denominator = 1 + } + + // If the node count is bigger than maxBlacklistedRounds, + // we return 1. + return max(maxBlacklistedRounds/denominator, 1) +} + +// Stateless returns a new blacklist with the same configuration as the current blacklist, but without any state. +// It derives the maxBlacklistedOrbits from the current blacklist, rescaled to the new node count, if possible. +// Otherwise it returns a new blacklist with the given node count and GenesisMaxBlacklistedRounds. +func (bl *Blacklist) Stateless(nodeCount uint16) Blacklist { + if bl.maxBlacklistedOrbits == 0 { + return NewBlacklist(nodeCount, GenesisMaxBlacklistedRounds) + } return Blacklist{ - NodeCount: nodeCount, + nodeCount: nodeCount, + maxBlacklistedOrbits: rescaleMaxBlacklistedOrbits(bl.maxBlacklistedOrbits, bl.nodeCount, nodeCount), } } +// Next returns the next blacklist for the given round, based on the current blacklist and the given timed out and redeemed nodes. +func (bl *Blacklist) Next(round uint64, maxBlacklistedRounds uint16, timedOut, redeemed map[uint16]uint64) Blacklist { + updates := bl.computeBlacklistUpdates(round, bl.nodeCount, timedOut, redeemed) + newBlacklist := bl.applyUpdates(updates, round) + targetOrbits := maxBlacklistedOrbitsForRounds(bl.nodeCount, maxBlacklistedRounds) + newBlacklist.maxBlacklistedOrbits = bl.nextMaxBlacklistedOrbits(targetOrbits) + return newBlacklist +} + +// rescaleMaxBlacklistedOrbits converts the given amount of orbits of prevNodeCount into whole orbits of nodeCount, +// such that it corresponds to (roughly) the same number of rounds. +// The result is at least one orbit, and at most math.MaxUint16. +func rescaleMaxBlacklistedOrbits(orbits uint16, prevNodeCount uint16, nodeCount uint16) uint16 { + // Protection against division by zero, and if we don't know the previous node count, we keep the orbits as is. + if nodeCount == 0 || prevNodeCount == 0 { + return max(orbits, 1) + } + + rounds := uint64(orbits) * uint64(prevNodeCount) + rescaled := min(rounds/uint64(nodeCount), math.MaxUint16) + return max(uint16(rescaled), 1) +} + +// nextMaxBlacklistedOrbits returns the maxBlacklistedOrbits that a block built on top of this blacklist should have. +// It moves the current value by a single orbit towards the given target, or keeps it if it is already equal to the target. +func (bl *Blacklist) nextMaxBlacklistedOrbits(target uint16) uint16 { + current := bl.maxBlacklistedOrbits + switch { + case current < target: + return current + 1 + case current > target && current > 1: + return current - 1 + default: + return current + } +} + +// isValidMaxBlacklistedOrbitsTransition returns whether the maxBlacklistedOrbits can change from prev to next +// between a block and its child. The value can change by at most one orbit, and it must be at least one orbit. +func isValidMaxBlacklistedOrbitsTransition(prev, next uint16) bool { + if next == 0 { + return false + } + return next == prev || uint32(next) == uint32(prev)+1 || uint32(next)+1 == uint32(prev) +} + func (bl *Blacklist) String() string { - sn := make([]string, len(bl.SuspectedNodes)) - for i := range bl.SuspectedNodes { - sn[i] = bl.SuspectedNodes[i].String() + sn := make([]string, len(bl.suspectedNodes)) + for i := range bl.suspectedNodes { + sn[i] = bl.suspectedNodes[i].String() } return fmt.Sprintf( - "Blacklist(nodeCount=%d, SuspectedNodes=[%s], updates=[%s])", - bl.NodeCount, + "Blacklist(nodeCount=%d, maxBlacklistedOrbits=%d, suspectedNodes=[%s], updates=[%s])", + bl.nodeCount, + bl.maxBlacklistedOrbits, strings.Join(sn, ","), - BlacklistUpdatesAsString(bl.Updates), // helper receives array + blacklistUpdatesAsString(bl.updates), // helper receives array ) } -func BlacklistUpdatesAsString(updates []BlacklistUpdate) string { +func blacklistUpdatesAsString(updates []BlacklistUpdate) string { out := make([]string, len(updates)) for i := range updates { out[i] = updates[i].String() @@ -80,25 +186,28 @@ func BlacklistUpdatesAsString(updates []BlacklistUpdate) string { } func (bl *Blacklist) Equals(b2 *Blacklist) bool { - if bl.NodeCount != b2.NodeCount { + if bl.nodeCount != b2.nodeCount { + return false + } + if bl.maxBlacklistedOrbits != b2.maxBlacklistedOrbits { return false } - if len(bl.SuspectedNodes) != len(b2.SuspectedNodes) { + if len(bl.suspectedNodes) != len(b2.suspectedNodes) { return false } - for i := range bl.SuspectedNodes { - sn1 := &bl.SuspectedNodes[i] - sn2 := &b2.SuspectedNodes[i] + for i := range bl.suspectedNodes { + sn1 := &bl.suspectedNodes[i] + sn2 := &b2.suspectedNodes[i] if !sn1.Equals(sn2) { return false } } - if len(bl.Updates) != len(b2.Updates) { + if len(bl.updates) != len(b2.updates) { return false } - for i := range bl.Updates { - u1 := &bl.Updates[i] - u2 := &b2.Updates[i] + for i := range bl.updates { + u1 := &bl.updates[i] + u2 := &b2.updates[i] if !u1.Equals(u2) { return false } @@ -146,23 +255,24 @@ func (bu *BlacklistUpdate) String() string { } func (bl *Blacklist) IsEmpty() bool { - return bl.NodeCount == 0 + return bl.nodeCount == 0 } // ApplyUpdates applies the given updates in the given round to the current blacklist and returns a new blacklist. // The current blacklist is not modified. -func (bl *Blacklist) ApplyUpdates(updates []BlacklistUpdate, round uint64) Blacklist { +func (bl *Blacklist) applyUpdates(updates []BlacklistUpdate, round uint64) Blacklist { newBlacklist := Blacklist{ - NodeCount: bl.NodeCount, - SuspectedNodes: make(SuspectedNodes, len(bl.SuspectedNodes)), - Updates: make([]BlacklistUpdate, len(updates)), + nodeCount: bl.nodeCount, + maxBlacklistedOrbits: bl.maxBlacklistedOrbits, + suspectedNodes: make(suspectedNodes, len(bl.suspectedNodes)), + updates: make([]BlacklistUpdate, len(updates)), } - newBlacklist.SuspectedNodes = bl.garbageCollectSuspectedNodes(round) + newBlacklist.suspectedNodes = bl.garbageCollectSuspectedNodes(round) for i := range updates { update := &updates[i] - orbit := Orbit(round, update.NodeIndex, bl.NodeCount) + orbit := orbit(round, update.NodeIndex, bl.nodeCount) switch update.Type { case BlacklistOpType_NodeSuspected: newBlacklist.setNodeSuspected(orbit, update.NodeIndex) @@ -171,7 +281,7 @@ func (bl *Blacklist) ApplyUpdates(updates []BlacklistUpdate, round uint64) Black } } - copy(newBlacklist.Updates, updates) + copy(newBlacklist.updates, updates) return newBlacklist } @@ -180,18 +290,23 @@ func (bl *Blacklist) ApplyUpdates(updates []BlacklistUpdate, round uint64) Black // NodeIDs that are no longer suspected or have been redeemed, will not be included in the returned suspected nodes. // It will also garbage-collect any redeem votes from past orbits, unless hey have surpassed the threshold of f+1. // It does not modify the current blacklist. -func (bl *Blacklist) garbageCollectSuspectedNodes(round uint64) SuspectedNodes { - newSuspectedNodes := make([]SuspectedNode, 0, len(bl.SuspectedNodes)) +func (bl *Blacklist) garbageCollectSuspectedNodes(round uint64) suspectedNodes { + newSuspectedNodes := make([]SuspectedNode, 0, len(bl.suspectedNodes)) threshold := bl.computeThreshold() - for i := range bl.SuspectedNodes { - sn := &bl.SuspectedNodes[i] - orbit := Orbit(round, sn.NodeIndex, bl.NodeCount) + for i := range bl.suspectedNodes { + sn := &bl.suspectedNodes[i] + orbit := orbit(round, sn.NodeIndex, bl.nodeCount) // Node is no longer suspected, just skip it. if !sn.isStillSuspected(threshold, orbit) { continue } + if orbit > sn.OrbitSuspected && orbit-sn.OrbitSuspected >= uint64(bl.maxBlacklistedOrbits) { + // The node has been blacklisted for too many orbits, so we remove it from the suspected list. + continue + } + newSuspectedNode := sn.Clone() // Reset the redeemingCount and orbitToRedeem if it was a past orbit. @@ -213,7 +328,7 @@ func (bl *Blacklist) setNodeSuspected(orbit uint64, nodeIndex uint16) { // Sanity check in case we get a too high node index. // This shouldn't happen, but if it does, we should just ignore it. - if nodeIndex >= bl.NodeCount { + if nodeIndex >= bl.nodeCount { return } // First, look to see if the node is already suspected. @@ -233,23 +348,23 @@ func (bl *Blacklist) setNodeSuspected(orbit uint64, nodeIndex uint16) { // Otherwise, we add the node to the suspected list with a count of 1. // Count how many total nodes are suspected. - for i := range bl.SuspectedNodes { - sn := &bl.SuspectedNodes[i] + for i := range bl.suspectedNodes { + sn := &bl.suspectedNodes[i] if sn.SuspectingCount >= threshold { suspectedCount++ } } - f := (bl.NodeCount - 1) / 3 + f := (bl.nodeCount - 1) / 3 if suspectedCount >= f { // We already have f or more nodes suspected, abort because we cannot suspect any more nodes. return } - for i := range bl.SuspectedNodes { - sn := &bl.SuspectedNodes[i] + for i := range bl.suspectedNodes { + sn := &bl.suspectedNodes[i] // Only increment the count if the orbit is the same. if sn.NodeIndex == nodeIndex { if sn.OrbitSuspected == orbit { @@ -263,7 +378,7 @@ func (bl *Blacklist) setNodeSuspected(orbit uint64, nodeIndex uint16) { } // If we reached here, the node isn't in the suspected list, and it is small enough, so add it instead. - bl.SuspectedNodes = append(bl.SuspectedNodes, SuspectedNode{ + bl.suspectedNodes = append(bl.suspectedNodes, SuspectedNode{ NodeIndex: nodeIndex, SuspectingCount: 1, OrbitSuspected: orbit, @@ -280,7 +395,7 @@ func (bl *Blacklist) setNodeRedeemed(orbit uint64, nodeIndex uint16) { // Sanity check in case we get a too high node index. // This shouldn't happen, but if it does, we should just ignore it. - if nodeIndex >= bl.NodeCount { + if nodeIndex >= bl.nodeCount { return } // First, look to see if the node is not suspected. @@ -290,8 +405,8 @@ func (bl *Blacklist) setNodeRedeemed(orbit uint64, nodeIndex uint16) { } // Else, the node is suspected. Proceed to redeem it. - for i := range bl.SuspectedNodes { - sn := &bl.SuspectedNodes[i] + for i := range bl.suspectedNodes { + sn := &bl.suspectedNodes[i] // Only increment the count if the orbit is the same. if sn.NodeIndex == nodeIndex { if sn.OrbitSuspected == orbit { @@ -308,7 +423,7 @@ func (bl *Blacklist) setNodeRedeemed(orbit uint64, nodeIndex uint16) { // If we have found a threshold of redeeming nodes, then we need to remove the node // from the suspected list. if sn.RedeemingCount >= threshold { - bl.SuspectedNodes = slices.Delete(bl.SuspectedNodes, i, i+1) + bl.suspectedNodes = slices.Delete(bl.suspectedNodes, i, i+1) return } // Else, the orbit is different, so just return early. @@ -318,7 +433,7 @@ func (bl *Blacklist) setNodeRedeemed(orbit uint64, nodeIndex uint16) { } func (bl *Blacklist) computeThreshold() uint16 { - f := (bl.NodeCount - 1) / 3 + f := (bl.nodeCount - 1) / 3 threshold := f + 1 return threshold } @@ -326,8 +441,8 @@ func (bl *Blacklist) computeThreshold() uint16 { func (bl *Blacklist) IsNodeSuspected(nodeIndex uint16) bool { threshold := bl.computeThreshold() - for i := range bl.SuspectedNodes { - sn := &bl.SuspectedNodes[i] + for i := range bl.suspectedNodes { + sn := &bl.suspectedNodes[i] if sn.NodeIndex == nodeIndex { return sn.SuspectingCount >= threshold && sn.RedeemingCount < threshold } @@ -338,15 +453,16 @@ func (bl *Blacklist) IsNodeSuspected(nodeIndex uint16) bool { func (bl *Blacklist) Clone() Blacklist { clone := Blacklist{ - NodeCount: bl.NodeCount, + nodeCount: bl.nodeCount, + maxBlacklistedOrbits: bl.maxBlacklistedOrbits, } - clone.SuspectedNodes = make(SuspectedNodes, 0, len(bl.SuspectedNodes)) - for i := range bl.SuspectedNodes { - clone.SuspectedNodes = append(clone.SuspectedNodes, bl.SuspectedNodes[i].Clone()) + clone.suspectedNodes = make(suspectedNodes, 0, len(bl.suspectedNodes)) + for i := range bl.suspectedNodes { + clone.suspectedNodes = append(clone.suspectedNodes, bl.suspectedNodes[i].Clone()) } - clone.Updates = make(BlacklistUpdates, 0, len(bl.Updates)) - for i := range bl.Updates { - clone.Updates = append(clone.Updates, bl.Updates[i].Clone()) + clone.updates = make(BlacklistUpdates, 0, len(bl.updates)) + for i := range bl.updates { + clone.updates = append(clone.updates, bl.updates[i].Clone()) } return clone } @@ -363,16 +479,22 @@ func (bl *Blacklist) FromBytes(buff []byte) error { } func (bl *Blacklist) VerifyProposedBlacklist(candidateBlacklist Blacklist, round uint64) error { - if candidateBlacklist.NodeCount != bl.NodeCount { - return fmt.Errorf("%s, expected %d, got %d", errBlacklistInvalidNodeCount, bl.NodeCount, candidateBlacklist.NodeCount) + if candidateBlacklist.nodeCount != bl.nodeCount { + return fmt.Errorf("%s, expected %d, got %d", errBlacklistInvalidNodeCount, bl.nodeCount, candidateBlacklist.nodeCount) + } + if !isValidMaxBlacklistedOrbitsTransition(bl.maxBlacklistedOrbits, candidateBlacklist.maxBlacklistedOrbits) { + return fmt.Errorf("%s, expected a non-zero value within one orbit of %d, got %d", errBlacklistInvalidMaxBlacklistedOrbits, bl.maxBlacklistedOrbits, candidateBlacklist.maxBlacklistedOrbits) } // 1) First thing we check that the updates even make sense. - if err := bl.verifyBlacklistUpdates(candidateBlacklist.Updates); err != nil { + if err := bl.verifyBlacklistUpdates(candidateBlacklist.updates); err != nil { return fmt.Errorf("%s: %w", errBlacklistInvalidUpdates, err) } - updates := candidateBlacklist.Updates + updates := candidateBlacklist.updates // 2) We then proceed by applying the updates to the blacklist of the previous round. - expectedBlacklist := bl.ApplyUpdates(updates, round) + // Garbage collection of expired suspected nodes uses the maxBlacklistedOrbits of the previous round, + // and the new maxBlacklistedOrbits only takes effect in the next round. + expectedBlacklist := bl.applyUpdates(updates, round) + expectedBlacklist.maxBlacklistedOrbits = candidateBlacklist.maxBlacklistedOrbits if !candidateBlacklist.Equals(&expectedBlacklist) { return fmt.Errorf("%s, expected %s, got %s", errBlacklistInvalidBlacklist, expectedBlacklist.String(), candidateBlacklist.String()) @@ -383,14 +505,14 @@ func (bl *Blacklist) VerifyProposedBlacklist(candidateBlacklist Blacklist, round func (bl *Blacklist) verifyBlacklistUpdates(updates []BlacklistUpdate) error { seen := make(map[uint16]struct{}) - if len(updates) > int(bl.NodeCount) { - return fmt.Errorf("%w: %d, only %d nodes exist", errBlacklistTooManyUpdates, len(updates), bl.NodeCount) + if len(updates) > int(bl.nodeCount) { + return fmt.Errorf("%w: %d, only %d nodes exist", errBlacklistTooManyUpdates, len(updates), bl.nodeCount) } for i := range updates { update := &updates[i] - if update.NodeIndex >= bl.NodeCount { + if update.NodeIndex >= bl.nodeCount { return fmt.Errorf("%w: %d, needs to be in [%d, %d]", - errBlacklistInvalidNodeIndex, update.NodeIndex, 0, bl.NodeCount-1) + errBlacklistInvalidNodeIndex, update.NodeIndex, 0, bl.nodeCount-1) } if _, exists := seen[update.NodeIndex]; exists { @@ -417,7 +539,7 @@ func (bl *Blacklist) verifyBlacklistUpdates(updates []BlacklistUpdate) error { return nil } -func (bl *Blacklist) ComputeBlacklistUpdates(round uint64, nodeCount uint16, timedOut, redeemed map[uint16]uint64) []BlacklistUpdate { +func (bl *Blacklist) computeBlacklistUpdates(round uint64, nodeCount uint16, timedOut, redeemed map[uint16]uint64) []BlacklistUpdate { updates := make([]BlacklistUpdate, 0, nodeCount) for nodeIndex := uint16(0); nodeIndex < nodeCount; nodeIndex++ { @@ -486,7 +608,7 @@ func (bl *Blacklist) ComputeBlacklistUpdates(round uint64, nodeCount uint16, tim return updates } -type SuspectedNodes []SuspectedNode +type suspectedNodes []SuspectedNode // SuspectedNode is the information we keep for each suspected node. // A suspected node records the number of accusations and when it was accused in. @@ -536,7 +658,7 @@ func (sn *SuspectedNode) String() string { // isStillSuspected returns true if the suspected node is still suspected in the given orbit. func (sn *SuspectedNode) isStillSuspected(threshold uint16, orbit uint64) bool { if sn.OrbitSuspected >= orbit { - // Orbit hasn't advanced. + // orbit hasn't advanced. return true } diff --git a/common/blacklist_test.go b/common/blacklist_test.go index a1224a91..9e4befbb 100644 --- a/common/blacklist_test.go +++ b/common/blacklist_test.go @@ -6,6 +6,7 @@ package common import ( "crypto/rand" "fmt" + "math" mathrand "math/rand" "testing" @@ -25,67 +26,127 @@ func TestBlacklistVerifyProposedBlacklist(t *testing.T) { name: "node count mismatch", nodeCount: 5, proposedBlacklist: Blacklist{ - NodeCount: 13, + nodeCount: 13, }, expectedErr: errBlacklistInvalidNodeCount, }, + { + name: "max blacklisted orbits increased by more than one", + blacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 10, + }, + nodeCount: 4, + proposedBlacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 12, + }, + expectedErr: errBlacklistInvalidMaxBlacklistedOrbits, + }, + { + name: "max blacklisted orbits decreased by more than one", + blacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 10, + }, + nodeCount: 4, + proposedBlacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 8, + }, + expectedErr: errBlacklistInvalidMaxBlacklistedOrbits, + }, + { + name: "max blacklisted orbits decreased to zero", + blacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 1, + }, + nodeCount: 4, + proposedBlacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 0, + }, + expectedErr: errBlacklistInvalidMaxBlacklistedOrbits, + }, + { + name: "max blacklisted orbits overflows", + blacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: math.MaxUint16, + }, + nodeCount: 4, + proposedBlacklist: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 0, + }, + expectedErr: errBlacklistInvalidMaxBlacklistedOrbits, + }, { name: "invalid blacklist update", blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, }, nodeCount: 4, round: 29, proposedBlacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, }, expectedErr: errBlacklistInvalidUpdates, }, { name: "node redeemed in the same orbit it was suspected in", blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 7, RedeemingCount: 0, OrbitToRedeem: 0}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 7, RedeemingCount: 0, OrbitToRedeem: 0}}, }, nodeCount: 4, round: 29, proposedBlacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 7, RedeemingCount: 1, OrbitToRedeem: 7}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 7, RedeemingCount: 1, OrbitToRedeem: 7}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, }, expectedErr: errBlacklistInvalidBlacklist, }, { name: "blacklist mismatch", blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 0}}, }, nodeCount: 4, round: 29, proposedBlacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 7}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 6, RedeemingCount: 0, OrbitToRedeem: 7}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, }, expectedErr: errBlacklistInvalidBlacklist, }, { name: "blacklist mismatch - too many blacklisted", blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 2}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 2}}, }, nodeCount: 4, round: 29, proposedBlacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 2}, {NodeIndex: 1, SuspectingCount: 1}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 1}}, + nodeCount: 4, + maxBlacklistedOrbits: 1, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 2}, {NodeIndex: 1, SuspectingCount: 1}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 1}}, }, expectedErr: errBlacklistInvalidBlacklist, }, @@ -215,7 +276,7 @@ func TestOrbit(t *testing.T) { } { t.Run(fmt.Sprintf("%d", testCase.round), func(t *testing.T) { for node, expectedOrbit := range testCase.expectedNodeToOrbit { - require.Equal(t, expectedOrbit, Orbit(testCase.round, node, uint16(len(nodes))), "unexpected OrbitSuspected for node %d", node) + require.Equal(t, expectedOrbit, orbit(testCase.round, node, uint16(len(nodes))), "unexpected OrbitSuspected for node %d", node) } }) } @@ -223,9 +284,10 @@ func TestOrbit(t *testing.T) { func TestBlacklistFromBytes(t *testing.T) { populated := Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1}}, - Updates: BlacklistUpdates{{NodeIndex: 3, Type: BlacklistOpType_NodeSuspected}}, + nodeCount: 4, + maxBlacklistedOrbits: 7, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1}}, + updates: BlacklistUpdates{{NodeIndex: 3, Type: BlacklistOpType_NodeSuspected}}, } for _, testCase := range []struct { @@ -236,8 +298,8 @@ func TestBlacklistFromBytes(t *testing.T) { }{ { name: "empty blacklist", - data: (&Blacklist{NodeCount: 100}).Bytes(), - expectedBlacklist: Blacklist{NodeCount: 100}, + data: (&Blacklist{nodeCount: 100}).Bytes(), + expectedBlacklist: Blacklist{nodeCount: 100}, }, { name: "populated blacklist", @@ -265,7 +327,7 @@ func TestBlacklistFromBytes(t *testing.T) { func TestBlacklistSimpleFlow(t *testing.T) { bl := Blacklist{ - NodeCount: 4, + nodeCount: 4, } require.False(t, bl.IsNodeSuspected(2)) @@ -275,7 +337,7 @@ func TestBlacklistSimpleFlow(t *testing.T) { bl.setNodeSuspected(1, 2) require.True(t, bl.IsNodeSuspected(2)) - bl.SuspectedNodes = bl.garbageCollectSuspectedNodes(5) + bl.suspectedNodes = bl.garbageCollectSuspectedNodes(5) require.True(t, bl.IsNodeSuspected(2), "Suspected node is still suspected even if its OrbitSuspected advanced") bl.setNodeRedeemed(3, 2) @@ -302,12 +364,12 @@ func TestBlacklistCapAppliesToPendingSuspects(t *testing.T) { } t.Run("setNodeSuspected", func(t *testing.T) { - bl := Blacklist{NodeCount: nodeCount} + bl := Blacklist{nodeCount: nodeCount} // One accusation each: both nodes are pending, neither is blacklisted. bl.setNodeSuspected(1, 2) bl.setNodeSuspected(1, 3) - require.Len(t, bl.SuspectedNodes, 2) + require.Len(t, bl.suspectedNodes, 2) require.Zero(t, countBlacklisted(bl)) // A second accusation blacklists node 2 and fills the cap. @@ -320,24 +382,20 @@ func TestBlacklistCapAppliesToPendingSuspects(t *testing.T) { require.LessOrEqual(t, countBlacklisted(bl), f, "%s", bl.String()) }) - t.Run("ApplyUpdates", func(t *testing.T) { + t.Run("Next", func(t *testing.T) { // Two consecutive rounds within one orbit for both accused nodes, as two successive leaders // would produce when accusing the same pair. const firstRound, secondRound = 2, 3 for _, node := range []uint16{0, 1} { - require.Equal(t, Orbit(firstRound, node, nodeCount), Orbit(secondRound, node, nodeCount)) - } - accuse := []BlacklistUpdate{ - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 0}, - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 1}, + require.Equal(t, orbit(firstRound, node, nodeCount), orbit(secondRound, node, nodeCount)) } - bl := Blacklist{NodeCount: nodeCount} - bl = bl.ApplyUpdates(accuse, firstRound) - require.Len(t, bl.SuspectedNodes, 2) + bl := Blacklist{nodeCount: nodeCount} + bl = bl.Next(firstRound, 0, map[uint16]uint64{0: firstRound, 1: firstRound}, nil) + require.Len(t, bl.suspectedNodes, 2) require.Zero(t, countBlacklisted(bl)) - bl = bl.ApplyUpdates(accuse, secondRound) + bl = bl.Next(secondRound, 0, map[uint16]uint64{0: secondRound, 1: secondRound}, nil) require.LessOrEqual(t, countBlacklisted(bl), f, "%s", bl.String()) }) } @@ -357,12 +415,12 @@ func FuzzBlacklistFromBytes(f *testing.F) { func TestBlacklistBytes(t *testing.T) { bl := Blacklist{ - NodeCount: 5, - SuspectedNodes: []SuspectedNode{ + nodeCount: 5, + suspectedNodes: []SuspectedNode{ {NodeIndex: 1, SuspectingCount: 2, OrbitSuspected: 3, RedeemingCount: 4, OrbitToRedeem: 5}, {NodeIndex: 6, SuspectingCount: 7, OrbitSuspected: 8, RedeemingCount: 9, OrbitToRedeem: 10}, }, - Updates: []BlacklistUpdate{ + updates: []BlacklistUpdate{ {Type: BlacklistOpType_NodeSuspected, NodeIndex: 11}, {Type: BlacklistOpType_NodeRedeemed, NodeIndex: 12}, }, @@ -381,7 +439,7 @@ func TestComputeBlacklistUpdates(t *testing.T) { name string round uint64 suspectedNodes []SuspectedNode - expectedUpdates []BlacklistUpdate + expectedUpdates BlacklistUpdates timedOut, redeemed map[uint16]uint64 }{ { @@ -445,13 +503,12 @@ func TestComputeBlacklistUpdates(t *testing.T) { } { t.Run(testCase.name, func(t *testing.T) { blacklist := Blacklist{ - NodeCount: 4, - SuspectedNodes: testCase.suspectedNodes, + nodeCount: 4, + suspectedNodes: testCase.suspectedNodes, } - update := blacklist.ComputeBlacklistUpdates(testCase.round, 4, testCase.timedOut, testCase.redeemed) - require.Equal(t, testCase.expectedUpdates, update) - + next := blacklist.Next(testCase.round, 0, testCase.timedOut, testCase.redeemed) + require.Equal(t, testCase.expectedUpdates, next.updates) }) } } @@ -462,7 +519,7 @@ func TestAdvanceRound(t *testing.T) { // NodeIDs 0, 2 are suspected. // NodeIDs 1 and 3 are not suspected. // Node 2 can be redeemed. - suspectedNodesBefore := SuspectedNodes{ + suspectedNodesBefore := suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 1, OrbitToRedeem: 1}, {NodeIndex: 1, SuspectingCount: 1, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, {NodeIndex: 2, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 2, OrbitToRedeem: 1}, @@ -470,7 +527,7 @@ func TestAdvanceRound(t *testing.T) { } testCases := []struct { - suspectedNodesAfter SuspectedNodes + suspectedNodesAfter suspectedNodes noChange bool }{ { @@ -491,7 +548,7 @@ func TestAdvanceRound(t *testing.T) { }, { // round 4 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, {NodeIndex: 1, SuspectingCount: 1, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, {NodeIndex: 2, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 2, OrbitToRedeem: 1}, @@ -500,7 +557,7 @@ func TestAdvanceRound(t *testing.T) { }, { // round 5 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, {NodeIndex: 2, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 2, OrbitToRedeem: 1}, {NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, @@ -508,26 +565,26 @@ func TestAdvanceRound(t *testing.T) { }, { // round 6 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, {NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, }, }, { // round 7 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, }, }, { // round 8 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, }, }, { // round 9 - suspectedNodesAfter: SuspectedNodes{ + suspectedNodesAfter: suspectedNodes{ {NodeIndex: 0, SuspectingCount: 2, OrbitSuspected: 1, RedeemingCount: 0, OrbitToRedeem: 0}, }, }, @@ -536,8 +593,9 @@ func TestAdvanceRound(t *testing.T) { for round := uint64(0); round < uint64(len(testCases)); round++ { t.Run(fmt.Sprintf("round %d", round), func(t *testing.T) { blacklist := Blacklist{ - NodeCount: uint16(len(nodes)), - SuspectedNodes: suspectedNodesBefore, + nodeCount: uint16(len(nodes)), + maxBlacklistedOrbits: 10, + suspectedNodes: suspectedNodesBefore, } suspectingNodes := blacklist.garbageCollectSuspectedNodes(round) @@ -553,7 +611,7 @@ func TestAdvanceRound(t *testing.T) { func TestVerifyBlacklistUpdates(t *testing.T) { testBlacklist := Blacklist{ - NodeCount: 4, + nodeCount: 4, } for _, testCase := range []struct { @@ -602,8 +660,8 @@ func TestVerifyBlacklistUpdates(t *testing.T) { { name: "already blacklisted", Blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: []SuspectedNode{{NodeIndex: 3, SuspectingCount: 2}}, + nodeCount: 4, + suspectedNodes: []SuspectedNode{{NodeIndex: 3, SuspectingCount: 2}}, }, updates: []BlacklistUpdate{ {Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}, @@ -614,8 +672,8 @@ func TestVerifyBlacklistUpdates(t *testing.T) { { name: "not blacklisted, cannot be redeemed", Blacklist: Blacklist{ - NodeCount: 4, - SuspectedNodes: []SuspectedNode{{NodeIndex: 3, SuspectingCount: 1}}, + nodeCount: 4, + suspectedNodes: []SuspectedNode{{NodeIndex: 3, SuspectingCount: 1}}, }, updates: []BlacklistUpdate{ {Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}, @@ -635,16 +693,16 @@ func TestBlacklistMultipleAttemptsOverSeveralOrbits(t *testing.T) { // as long as the rounds are too far from each other, the node will not be blacklisted. blacklist := Blacklist{ - NodeCount: 4, + nodeCount: 4, } - blacklist = blacklist.ApplyUpdates([]BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, 4) + blacklist = blacklist.Next(4, 0, map[uint16]uint64{3: 4}, nil) require.False(t, blacklist.IsNodeSuspected(3)) - blacklist = blacklist.ApplyUpdates([]BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, 7) + blacklist = blacklist.Next(7, 0, map[uint16]uint64{3: 7}, nil) require.False(t, blacklist.IsNodeSuspected(3)) - blacklist = blacklist.ApplyUpdates([]BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, 8) + blacklist = blacklist.Next(8, 0, map[uint16]uint64{3: 8}, nil) require.True(t, blacklist.IsNodeSuspected(3)) } @@ -665,7 +723,8 @@ func testBlacklistSimulateNetwork(t *testing.T, nodeCount int, rounds int) { // can be verified by other nodes. var blacklist Blacklist - blacklist.NodeCount = uint16(nodeCount) + blacklist.nodeCount = uint16(nodeCount) + blacklist.maxBlacklistedOrbits = 1 crashedNodes := make(map[uint16]struct{}) @@ -748,11 +807,12 @@ func simulateRound(t *testing.T, blrsi blacklistRoundSimulationInput) Blacklist if _, leaderCrashed := crashedNodes[leader]; leaderCrashed { // leader is crashed, nothing further that can be done. newBlacklist := Blacklist{ - NodeCount: prevBlacklist.NodeCount, - SuspectedNodes: make([]SuspectedNode, len(prevBlacklist.SuspectedNodes)), - Updates: make([]BlacklistUpdate, 0, len(prevBlacklist.Updates)), + nodeCount: prevBlacklist.nodeCount, + maxBlacklistedOrbits: prevBlacklist.maxBlacklistedOrbits, + suspectedNodes: make([]SuspectedNode, len(prevBlacklist.suspectedNodes)), + updates: make([]BlacklistUpdate, 0, len(prevBlacklist.updates)), } - copy(newBlacklist.SuspectedNodes, prevBlacklist.SuspectedNodes) + copy(newBlacklist.suspectedNodes, prevBlacklist.suspectedNodes) return newBlacklist } @@ -771,9 +831,8 @@ func simulateRound(t *testing.T, blrsi blacklistRoundSimulationInput) Blacklist } } - updates := prevBlacklist.ComputeBlacklistUpdates(round, uint16(nodeCount), timedOut, redeemed) - - newBlacklist := prevBlacklist.ApplyUpdates(updates, round) + // The blacklist starts with a single max blacklisted orbit, which is the target for zero max blacklisted rounds. + newBlacklist := prevBlacklist.Next(round, 0, timedOut, redeemed) err := prevBlacklist.VerifyProposedBlacklist(newBlacklist, round) require.NoError(t, err, "round %d", round) @@ -790,71 +849,82 @@ func TestBlacklistEquals(t *testing.T) { { name: "different node count", bl1: Blacklist{ - NodeCount: 4, + nodeCount: 4, + }, + bl2: Blacklist{ + nodeCount: 5, + }, + }, + { + name: "different max blacklisted orbits", + bl1: Blacklist{ + nodeCount: 4, + maxBlacklistedOrbits: 1, }, bl2: Blacklist{ - NodeCount: 5, + nodeCount: 4, + maxBlacklistedOrbits: 2, }, }, { name: "different suspected nodes count", bl1: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, }, bl2: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 1}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}, {NodeIndex: 2, SuspectingCount: 1}}, }, }, { name: "different suspected nodes content", bl1: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, }, bl2: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 1}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 1}}, }, }, { name: "different updates count", bl1: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, }, bl2: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}, {Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}, {Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, }, }, { name: "different updates content", bl1: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, }, bl2: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 2}}, }, }, { name: "equal blacklists", bl1: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, }, bl2: Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, + nodeCount: 4, + suspectedNodes: suspectedNodes{{NodeIndex: 3, SuspectingCount: 2}}, + updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}}, }, equals: true, }, @@ -864,3 +934,306 @@ func TestBlacklistEquals(t *testing.T) { }) } } + +func TestNewBlacklistMaxBlacklistedOrbits(t *testing.T) { + for _, testCase := range []struct { + name string + nodeCount uint16 + maxBlacklistedRounds uint16 + expectedOrbits uint16 + }{ + { + name: "rounds are converted to whole orbits", + nodeCount: 4, + maxBlacklistedRounds: 4620, + expectedOrbits: 1155, + }, + { + name: "rounds not divisible by node count are rounded down", + nodeCount: 7, + maxBlacklistedRounds: 20, + expectedOrbits: 2, + }, + { + name: "fewer rounds than nodes yields a single orbit", + nodeCount: 10, + maxBlacklistedRounds: 3, + expectedOrbits: 1, + }, + { + name: "zero rounds yields a single orbit", + nodeCount: 4, + maxBlacklistedRounds: 0, + expectedOrbits: 1, + }, + { + name: "zero node count does not divide by zero", + nodeCount: 0, + maxBlacklistedRounds: 9, + expectedOrbits: 9, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + bl := NewBlacklist(testCase.nodeCount, testCase.maxBlacklistedRounds) + require.Equal(t, testCase.nodeCount, bl.nodeCount) + require.Equal(t, testCase.expectedOrbits, bl.maxBlacklistedOrbits) + }) + } +} + +func TestBlacklistMaxBlacklistedOrbitsIsPreserved(t *testing.T) { + bl := NewBlacklist(4, 4620) + require.Equal(t, uint16(1155), bl.maxBlacklistedOrbits) + + applied := bl.Next(5, 4620, map[uint16]uint64{1: 5}, nil) + require.Equal(t, bl.maxBlacklistedOrbits, applied.maxBlacklistedOrbits, "Next should carry over maxBlacklistedOrbits when it is already at the target") + + cloned := applied.Clone() + require.Equal(t, bl.maxBlacklistedOrbits, cloned.maxBlacklistedOrbits, "Clone should carry over maxBlacklistedOrbits") + require.True(t, applied.Equals(&cloned)) + + var decoded Blacklist + require.NoError(t, decoded.FromBytes(applied.Bytes())) + require.Equal(t, bl.maxBlacklistedOrbits, decoded.maxBlacklistedOrbits, "serialization should preserve maxBlacklistedOrbits") + require.True(t, applied.Equals(&decoded)) +} + +func TestBlacklistedNodeExpiresAfterMaxBlacklistedOrbits(t *testing.T) { + const ( + nodeCount = 4 + maxBlacklistedOrbits = 2 + blacklistedNode = 1 + orbitSuspected = 3 + ) + + // With 4 nodes, node 1 is in orbit k during rounds 4(k-1)+1 ... 4k. + // Node 1 is blacklisted in orbit 3, and is allowed to remain blacklisted for 2 orbits (3 and 4). + // It should be removed from the blacklist once orbit 5 is reached. + blacklisted := SuspectedNode{NodeIndex: blacklistedNode, SuspectingCount: 2, OrbitSuspected: orbitSuspected} + + for _, testCase := range []struct { + name string + round uint64 + suspectedNode SuspectedNode + expectExpired bool + }{ + { + name: "same orbit it was suspected in", + round: 12, // orbit 3 + suspectedNode: blacklisted, + }, + { + name: "last orbit it is allowed to remain blacklisted in", + round: 16, // orbit 4 + suspectedNode: blacklisted, + }, + { + name: "first round of the orbit after the last allowed orbit", + round: 17, // orbit 5 + suspectedNode: blacklisted, + expectExpired: true, + }, + { + name: "long after the last allowed orbit", + round: 1000, + suspectedNode: blacklisted, + expectExpired: true, + }, + { + name: "partial redeeming votes do not prevent expiry", + round: 17, // orbit 5 + suspectedNode: SuspectedNode{NodeIndex: blacklistedNode, SuspectingCount: 2, OrbitSuspected: orbitSuspected, RedeemingCount: 1, OrbitToRedeem: 4}, + expectExpired: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + bl := Blacklist{ + nodeCount: nodeCount, + maxBlacklistedOrbits: maxBlacklistedOrbits, + suspectedNodes: suspectedNodes{testCase.suspectedNode}, + } + require.True(t, bl.IsNodeSuspected(blacklistedNode)) + + next := bl.Next(testCase.round, nodeCount*maxBlacklistedOrbits, nil, nil) + require.Equal(t, uint16(maxBlacklistedOrbits), next.maxBlacklistedOrbits) + + if testCase.expectExpired { + require.False(t, next.IsNodeSuspected(blacklistedNode)) + require.Empty(t, next.suspectedNodes) + } else { + require.True(t, next.IsNodeSuspected(blacklistedNode)) + require.Len(t, next.suspectedNodes, 1) + } + + // The blacklist of the previous round must not be modified. + require.True(t, bl.IsNodeSuspected(blacklistedNode)) + }) + } +} + +func TestBlacklistedNodeExpiryEndToEnd(t *testing.T) { + const ( + nodeCount = 4 + maxBlacklistedRounds = 8 // 2 orbits + blacklistedNode = 1 + ) + + bl := NewBlacklist(nodeCount, maxBlacklistedRounds) + require.Equal(t, uint16(2), bl.maxBlacklistedOrbits) + + // suspect makes the leader of the given round accuse the blacklisted node of timing out in that round. + suspect := func(round uint64) map[uint16]uint64 { + return map[uint16]uint64{blacklistedNode: round} + } + + // Round 2 and 3 are in orbit 1 for node 1. Two accusations blacklist it (f+1 = 2). + bl = bl.Next(2, maxBlacklistedRounds, suspect(2), nil) + require.False(t, bl.IsNodeSuspected(blacklistedNode)) + bl = bl.Next(3, maxBlacklistedRounds, suspect(3), nil) + require.True(t, bl.IsNodeSuspected(blacklistedNode)) + + // Round 4 is still orbit 1 for node 1, and orbit 2 spans rounds 5-8: still blacklisted. + for round := uint64(4); round <= 8; round++ { + bl = bl.Next(round, maxBlacklistedRounds, nil, nil) + require.True(t, bl.IsNodeSuspected(blacklistedNode), "round %d", round) + } + + // orbit 3 (round 9): expired. + bl = bl.Next(9, maxBlacklistedRounds, nil, nil) + require.False(t, bl.IsNodeSuspected(blacklistedNode)) + require.Empty(t, bl.suspectedNodes) + + // A redeem update for an expired node is rejected, as the node is no longer blacklisted. + redeem := []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: blacklistedNode}} + require.ErrorContains(t, bl.verifyBlacklistUpdates(redeem), errBlacklistNodeNotBlacklisted.Error()) + + // An expired node can be suspected again from scratch. + bl = bl.Next(10, maxBlacklistedRounds, suspect(10), nil) + require.False(t, bl.IsNodeSuspected(blacklistedNode)) + require.Equal(t, suspectedNodes{{NodeIndex: blacklistedNode, SuspectingCount: 1, OrbitSuspected: 3}}, bl.suspectedNodes) +} + +func TestBlacklistVerifyProposedBlacklistMaxBlacklistedOrbitsStep(t *testing.T) { + for _, testCase := range []struct { + name string + prev uint16 + proposed uint16 + }{ + {name: "unchanged", prev: 10, proposed: 10}, + {name: "increased by one", prev: 10, proposed: 11}, + {name: "decreased by one", prev: 10, proposed: 9}, + {name: "decreased to the minimum", prev: 2, proposed: 1}, + {name: "increased to the maximum", prev: math.MaxUint16 - 1, proposed: math.MaxUint16}, + {name: "increased from zero", prev: 0, proposed: 1}, + } { + t.Run(testCase.name, func(t *testing.T) { + prev := Blacklist{nodeCount: 4, maxBlacklistedOrbits: testCase.prev} + proposed := prev.Next(5, 0, nil, nil) + proposed.maxBlacklistedOrbits = testCase.proposed + require.NoError(t, prev.VerifyProposedBlacklist(proposed, 5)) + }) + } +} + +func TestBlacklistNextMaxBlacklistedOrbits(t *testing.T) { + for _, testCase := range []struct { + name string + current uint16 + target uint16 + expected uint16 + }{ + {name: "equal to target", current: 10, target: 10, expected: 10}, + {name: "below target", current: 10, target: 20, expected: 11}, + {name: "above target", current: 10, target: 5, expected: 9}, + {name: "never goes below one", current: 1, target: 0, expected: 1}, + {name: "never overflows", current: math.MaxUint16, target: math.MaxUint16, expected: math.MaxUint16}, + {name: "zero moves up to target", current: 0, target: 3, expected: 1}, + } { + t.Run(testCase.name, func(t *testing.T) { + // With a single node, an orbit is a single round, so the max blacklisted rounds are the target orbits. + // A target of zero rounds is treated as a single orbit. + bl := Blacklist{nodeCount: 1, maxBlacklistedOrbits: testCase.current} + next := bl.Next(5, testCase.target, nil, nil) + require.Equal(t, testCase.expected, next.maxBlacklistedOrbits) + }) + } +} + +func TestRescaleMaxBlacklistedOrbits(t *testing.T) { + for _, testCase := range []struct { + name string + orbits uint16 + prevNodeCount uint16 + nodeCount uint16 + expected uint16 + }{ + {name: "same node count", orbits: 1155, prevNodeCount: 4, nodeCount: 4, expected: 1155}, + {name: "node count grows", orbits: 1155, prevNodeCount: 4, nodeCount: 64, expected: 72}, + {name: "node count shrinks", orbits: 72, prevNodeCount: 64, nodeCount: 4, expected: 1152}, + {name: "rounds down", orbits: 10, prevNodeCount: 4, nodeCount: 7, expected: 5}, + {name: "never goes below one", orbits: 1, prevNodeCount: 4, nodeCount: 100, expected: 1}, + {name: "never overflows", orbits: math.MaxUint16, prevNodeCount: 100, nodeCount: 4, expected: math.MaxUint16}, + {name: "unknown previous node count", orbits: 10, prevNodeCount: 0, nodeCount: 4, expected: 10}, + {name: "zero node count", orbits: 10, prevNodeCount: 4, nodeCount: 0, expected: 10}, + } { + t.Run(testCase.name, func(t *testing.T) { + require.Equal(t, testCase.expected, rescaleMaxBlacklistedOrbits(testCase.orbits, testCase.prevNodeCount, testCase.nodeCount)) + }) + } +} + +func TestBlacklistMaxBlacklistedOrbitsRollingUpgrade(t *testing.T) { + // This test simulates a rolling upgrade where every node is reconfigured, one after another, + // from oldTarget to newTarget orbits. Each leader moves the value towards its own target, + // and every block must be accepted by all nodes regardless of their configuration. + const ( + nodeCount = 7 + oldTarget = 5 + newTarget = 20 + ) + + targets := make([]uint16, nodeCount) + for i := range targets { + targets[i] = oldTarget + } + + bl := Blacklist{nodeCount: nodeCount, maxBlacklistedOrbits: oldTarget} + round := uint64(0) + + buildAndVerify := func() { + leader := round % nodeCount + // The leader's target orbits correspond to that many orbits' worth of rounds. + next := bl.Next(round, targets[leader]*nodeCount, nil, nil) + require.NoError(t, bl.VerifyProposedBlacklist(next, round), "round %d", round) + bl = next + round++ + } + + // Upgrade the nodes one by one. While the upgraded nodes are a minority, the value can only + // be pushed above the old target by the upgraded nodes that lead consecutively, and is then pulled back. + // Once they are a majority, the value drifts towards the new target. + for upgraded := 1; upgraded < nodeCount; upgraded++ { + targets[upgraded-1] = newTarget + for range 3 * nodeCount { + buildAndVerify() + require.GreaterOrEqual(t, bl.maxBlacklistedOrbits, uint16(oldTarget), "round %d", round) + require.LessOrEqual(t, bl.maxBlacklistedOrbits, uint16(newTarget), "round %d", round) + if upgraded <= nodeCount-upgraded { + require.LessOrEqual(t, bl.maxBlacklistedOrbits, uint16(oldTarget+upgraded), "round %d", round) + } + } + } + + // Once all nodes are upgraded, the value moves by one orbit per block until it reaches the new target. + targets[nodeCount-1] = newTarget + for range newTarget - oldTarget { + buildAndVerify() + } + require.Equal(t, uint16(newTarget), bl.maxBlacklistedOrbits) + + // And it stays there. + for range 2 * nodeCount { + buildAndVerify() + require.Equal(t, uint16(newTarget), bl.maxBlacklistedOrbits) + } +} diff --git a/config.go b/config.go index 8467a700..d1574ec5 100644 --- a/config.go +++ b/config.go @@ -26,6 +26,10 @@ type ParameterConfig struct { // WALGCInterval is the number of indexed blocks between WAL garbage collections. // Defaults to DefaultWALGCInterval. WALGCInterval uint64 + // MaxBlacklistedRounds is the maximum number of rounds a node may remain blacklisted for. + // Changing it takes effect gradually, as each block may only move the value recorded in the chain by a single orbit. + // Defaults to DefaultMaxBlacklistedRounds + MaxBlacklistedRounds uint16 } // PlatformChain is an interface that abstracts the interaction with the P-chain. diff --git a/external_test.go b/external_test.go index 6af93437..1e9ae25d 100644 --- a/external_test.go +++ b/external_test.go @@ -10,10 +10,15 @@ import ( "github.com/ava-labs/simplex/common" metadata "github.com/ava-labs/simplex/msm" + "github.com/ava-labs/simplex/simplex" "github.com/stretchr/testify/require" ) func TestParseBlockSizeMatchesBytes(t *testing.T) { + blacklistWithUpdate := func() common.Blacklist { + blacklist := common.NewBlacklist(2, simplex.DefaultMaxBlacklistedRounds) + return blacklist.Next(1, simplex.DefaultMaxBlacklistedRounds, map[uint16]uint64{1: 1}, nil) + } // Case 1: Bytes() first, Size() second, size returns the cached length. pb := &ParsedBlock{ StateMachineBlock: metadata.StateMachineBlock{ @@ -25,11 +30,8 @@ func TestParseBlockSizeMatchesBytes(t *testing.T) { Epoch: 4, Seq: 2, }, - SimplexBlacklist: common.Blacklist{ - Updates: common.BlacklistUpdates{{NodeIndex: 1, Type: 1}}, - NodeCount: 2, - }, - PChainHeight: 6, + SimplexBlacklist: blacklistWithUpdate(), + PChainHeight: 6, }, InnerBlock: &testInnerBlock{ Height_: 7, @@ -53,11 +55,8 @@ func TestParseBlockSizeMatchesBytes(t *testing.T) { Epoch: 4, Seq: 2, }, - SimplexBlacklist: common.Blacklist{ - Updates: common.BlacklistUpdates{{NodeIndex: 1, Type: 1}}, - NodeCount: 2, - }, - PChainHeight: 6, + SimplexBlacklist: blacklistWithUpdate(), + PChainHeight: 6, }, InnerBlock: &testInnerBlock{ Height_: 9, @@ -85,11 +84,8 @@ func TestParseBlockSizeMatchesBytes(t *testing.T) { Epoch: 4, Seq: 2, }, - SimplexBlacklist: common.Blacklist{ - Updates: common.BlacklistUpdates{{NodeIndex: 1, Type: 1}}, - NodeCount: 2, - }, - PChainHeight: 6, + SimplexBlacklist: blacklistWithUpdate(), + PChainHeight: 6, }, InnerBlock: &testInnerBlock{ Height_: 11, diff --git a/instance.go b/instance.go index 197de4ee..ffaf1ca4 100644 --- a/instance.go +++ b/instance.go @@ -644,6 +644,7 @@ func (i *Instance) createEpochConfig(validators common.Nodes) (*epochConfig, err MaxProposalWait: i.Config.ParameterConfig.MaxNetworkDelay * 2, // 1 proposal + 1 vote MaxRebroadcastWait: i.Config.ParameterConfig.MaxNetworkDelay * 2, MaxRoundWindow: i.Config.ParameterConfig.MaxRoundWindow, + MaxBlacklistedRounds: i.Config.ParameterConfig.MaxBlacklistedRounds, ID: i.Config.ID, RandomSource: source, // Seed the random source from crypto/rand WAL: i.wal, diff --git a/instance_helpers_test.go b/instance_helpers_test.go index 1a2ca4a5..5573f1c2 100644 --- a/instance_helpers_test.go +++ b/instance_helpers_test.go @@ -72,6 +72,13 @@ var paramConfig = ParameterConfig{ WALGCInterval: walGCInterval, } +// paramsWithMaxBlacklistedRounds returns the shared paramConfig with the given max blacklisted rounds. +func paramsWithMaxBlacklistedRounds(rounds uint16) *ParameterConfig { + params := paramConfig + params.MaxBlacklistedRounds = rounds + return ¶ms +} + type testPlatformChain struct { genesisHeight uint64 // genesis height is the height of the pchain the genesis validator set lives @@ -614,7 +621,19 @@ func (n *node) walRecords() [][]byte { return records } +// restart restarts the node with the configuration its instance was started with. func (n *node) restart() *node { + params := n.inst.Config.ParameterConfig + return n.restartWithNodeConfig(nodeConfig{ + params: ¶ms, + lastNonSimplexBlock: n.inst.Config.LastNonSimplexInnerBlock, + }) +} + +// restartWithNodeConfig restarts the node like restart, but with the given config. +// The node always comes back up over its own storage and WAL, so the storage, wals and existingNode +// fields of the given config are ignored. +func (n *node) restartWithNodeConfig(cfg nodeConfig) *node { n.stop() records := n.walRecords() @@ -625,10 +644,10 @@ func (n *node) restart() *node { require.NoError(n.t, restoredWAL.Append(record)) } - newNode := n.net.addNodeWithConfig(n.id, nodeConfig{ - wals: []wal.DeletableWAL{restoredWAL}, storage: n.storage, existingNode: true, - }) - return newNode + cfg.wals = []wal.DeletableWAL{restoredWAL} + cfg.storage = n.storage + cfg.existingNode = true + return n.net.addNodeWithConfig(n.id, cfg) } // role reports whether the instance currently runs a validator epoch rather than a diff --git a/instance_test.go b/instance_test.go index f395ce12..647f35a8 100644 --- a/instance_test.go +++ b/instance_test.go @@ -1025,3 +1025,193 @@ func TestValidatorRemovedWhileOffline(t *testing.T) { isValidator, _ := node.role() require.False(t, isValidator) } + +// TestInstanceBlacklistConfigurationRollsOverAcrossEpochs builds a chain across epochs that alternate between two and three +// validators, whose configured max blacklisted rounds differ, and then inspects the chain to ensure that the max blacklisted +// orbits rolls over from one epoch to the next, and that each block moves it by at most a single orbit towards the +// configuration of the node that built it. Every epoch has at least two validators, so every block is verified by a +// validator other than the one that built it. +func TestInstanceBlacklistConfigurationRollsOverAcrossEpochs(t *testing.T) { + v1 := newNodeMapping(1) + v2 := newNodeMapping(2) + v3 := newNodeMapping(3) + + // The first ever blacklist starts from GenesisMaxBlacklistedRounds (2310 orbits of two validators), + // so the configurations are close to it, to let the chain reach them within a few blocks. + // v3 is configured differently, and is a validator only in the second epoch. + maxBlacklistedRoundsByNode := map[string]uint16{ + string(v1.NodeID[:]): common.GenesisMaxBlacklistedRounds - 8, // 2306 orbits of two validators, 1537 orbits of three validators + string(v2.NodeID[:]): common.GenesisMaxBlacklistedRounds - 8, // 2306 orbits of two validators, 1537 orbits of three validators + string(v3.NodeID[:]): common.GenesisMaxBlacklistedRounds + 60, // 1560 orbits of three validators + } + twoValidators := metadata.NodeBLSMappings{v1, v2} + threeValidators := metadata.NodeBLSMappings{v1, v2, v3} + + pChain := newTestPChain(twoValidators) + network := newNetwork(t, pChain) + addNode := func(vdr metadata.NodeBLSMapping) *node { + return network.addNodeWithConfig(vdr.NodeID[:], nodeConfig{params: paramsWithMaxBlacklistedRounds(maxBlacklistedRoundsByNode[string(vdr.NodeID[:])])}) + } + + // The first epoch is run by v1 and v2. + node1 := addNode(v1) + addNode(v2) + network.sync() + for range 4 { + network.acceptNewBlock() + } + + // v3 joins in the second epoch. + addNode(v3).sync() + pChain.setValidatorSetAt(10, threeValidators) + pChain.advanceHeight(10) + network.waitUntilSealingBlock(threeValidators.Nodes()) + for range 9 { + network.acceptNewBlock() + } + + // v3 leaves in the third epoch. + pChain.setValidatorSetAt(20, twoValidators) + pChain.advanceHeight(20) + network.waitUntilSealingBlock(twoValidators.Nodes()) + for range 8 { + network.acceptNewBlock() + } + + // Inspect the chain: every block moves the max blacklisted orbits of its parent, possibly from the previous epoch, + // by a single orbit towards the configuration of the node that built it. The first ever blacklist has v1's and v2's configuration. + // When the parent is from an epoch with a different number of validators, its max blacklisted orbits is first rescaled + // to the new number of validators. + validators := map[uint16]common.NodeIDs{2: twoValidators.NodeIDs(), 3: threeValidators.NodeIDs()} + orbits := uint16(common.GenesisMaxBlacklistedRounds / len(twoValidators)) + var epochSizes []uint16 + var prevBlacklist common.Blacklist + for seq := uint64(1); seq < network.seq; seq++ { + block := node1.storage.WaitForBlockCommit(seq) + blacklist := block.Blacklist() + if len(epochSizes) == 0 || epochSizes[len(epochSizes)-1] != blacklist.NodeCount() { + if len(epochSizes) > 0 { + fresh := prevBlacklist.Stateless(blacklist.NodeCount()) + orbits = fresh.MaxBlacklistedOrbits() + } + epochSizes = append(epochSizes, blacklist.NodeCount()) + } + + leader := simplex.LeaderForRound(validators[blacklist.NodeCount()], block.BlockHeader().Round) + target := maxBlacklistedRoundsByNode[string(leader)] / blacklist.NodeCount() + switch { + case orbits < target: + orbits++ + case orbits > target: + orbits-- + } + require.Equal(t, orbits, blacklist.MaxBlacklistedOrbits(), "block %d built by %s", seq, leader) + prevBlacklist = blacklist + } + + require.Equal(t, []uint16{2, 3, 2}, epochSizes) + // The last epoch is run by v1 and v2, so the chain ends up with their configuration. + require.Equal(t, maxBlacklistedRoundsByNode[string(v1.NodeID[:])]/uint16(len(twoValidators)), orbits) +} + +// TestInstanceMaxBlacklistedRoundsRollingRestart restarts the validators one after another with a higher max blacklisted rounds, +// and then again with a lower one, while the chain keeps growing. It then inspects the chain to ensure that each block moves +// the max blacklisted orbits by a single orbit towards the configuration of the node that built it at the time, +// and that the chain ends up with the configuration all nodes were eventually restarted with. +func TestInstanceMaxBlacklistedRoundsRollingRestart(t *testing.T) { + const ( + initialRounds = common.GenesisMaxBlacklistedRounds // 1155 orbits of 4 validators + increasedRounds = common.GenesisMaxBlacklistedRounds + 12 // 1158 orbits of 4 validators + decreasedRounds = common.GenesisMaxBlacklistedRounds - 8 // 1153 orbits of 4 validators + ) + + genesisSet := metadata.NodeBLSMappings{newNodeMapping(1), newNodeMapping(2), newNodeMapping(3), newNodeMapping(4)} + pChain := newTestPChain(genesisSet) + network := newNetwork(t, pChain) + + // maxBlacklistedRoundsByNode holds the max blacklisted rounds each node is currently configured with. + maxBlacklistedRoundsByNode := make(map[string]uint16) + nodes := make([]*node, len(genesisSet)) + for i, vdr := range genesisSet { + nodes[i] = network.addNodeWithConfig(vdr.NodeID[:], nodeConfig{params: paramsWithMaxBlacklistedRounds(initialRounds)}) + maxBlacklistedRoundsByNode[string(vdr.NodeID[:])] = initialRounds + } + network.sync() + + // targets holds the max blacklisted orbits each block's builder was configured with when it built the block. + // The first simplex block is built while the nodes sync, with the initial configuration. + nodeCount := uint16(len(genesisSet)) + targets := map[uint64]uint16{1: initialRounds / nodeCount} + acceptNewBlocks := func(count int) { + for range count { + block, _ := network.acceptNewBlock() + leader := simplex.LeaderForRound(genesisSet.NodeIDs(), block.BlockHeader().Round) + targets[block.BlockHeader().Seq] = maxBlacklistedRoundsByNode[string(leader)] / nodeCount + } + } + + acceptNewBlocks(4) + for _, maxBlacklistedRounds := range []uint16{increasedRounds, decreasedRounds} { + for i := range nodes { + nodes[i] = nodes[i].restartWithNodeConfig(nodeConfig{params: paramsWithMaxBlacklistedRounds(maxBlacklistedRounds)}) + maxBlacklistedRoundsByNode[string(nodes[i].id)] = maxBlacklistedRounds + acceptNewBlocks(4) + } + acceptNewBlocks(8) + } + + // Inspect the chain: every block moves the max blacklisted orbits of its parent by exactly a single orbit + // towards its target, or keeps it if it is already there. + // The first ever blacklist has no parent, it starts from GenesisMaxBlacklistedRounds. + prevMaxOrbits := common.GenesisMaxBlacklistedRounds / nodeCount + var highest uint16 + for seq := uint64(1); seq < network.seq; seq++ { + block := nodes[0].storage.WaitForBlockCommit(seq) + blacklist := block.Blacklist() + actual := blacklist.MaxBlacklistedOrbits() + + switch target := targets[seq]; { + case prevMaxOrbits < target: + require.Equal(t, prevMaxOrbits+1, actual, "block %d", seq) + case prevMaxOrbits > target: + require.Equal(t, prevMaxOrbits-1, actual, "block %d", seq) + default: + require.Equal(t, prevMaxOrbits, actual, "block %d", seq) + } + prevMaxOrbits = actual + highest = max(highest, actual) + } + + // The chain reached the increased configuration, and ended up with the decreased one. + require.Equal(t, increasedRounds/nodeCount, highest) + require.Equal(t, decreasedRounds/nodeCount, prevMaxOrbits) +} + +// TestInstanceMaxBlacklistedRoundsDifferAtGenesis runs a network whose validators are configured with different +// max blacklisted rounds from the very first block. The first blacklist must not depend on the configuration of its +// builder, otherwise validators configured differently would reject each other's blocks, and the network would never +// commit a block. It ensures that every block is committed by every validator. +func TestInstanceMaxBlacklistedRoundsDifferAtGenesis(t *testing.T) { + const ( + lowRounds = 8 // 2 orbits of 4 validators + highRounds = common.GenesisMaxBlacklistedRounds + 8 // 1157 orbits of 4 validators + ) + + genesisSet := metadata.NodeBLSMappings{newNodeMapping(1), newNodeMapping(2), newNodeMapping(3), newNodeMapping(4)} + configured := map[string]uint16{ + string(genesisSet[0].NodeID[:]): lowRounds, + string(genesisSet[1].NodeID[:]): lowRounds, + string(genesisSet[2].NodeID[:]): highRounds, + string(genesisSet[3].NodeID[:]): highRounds, + } + + pChain := newTestPChain(genesisSet) + network := newNetwork(t, pChain) + for _, vdr := range genesisSet { + network.addNodeWithConfig(vdr.NodeID[:], nodeConfig{params: paramsWithMaxBlacklistedRounds(configured[string(vdr.NodeID[:])])}) + } + network.sync() + for range 8 { + network.acceptNewBlock() + } +} diff --git a/msm/msm_test.go b/msm/msm_test.go index 538defc4..b15b5bfc 100644 --- a/msm/msm_test.go +++ b/msm/msm_test.go @@ -15,6 +15,7 @@ import ( "github.com/ava-labs/simplex/avalanchego" "github.com/ava-labs/simplex/common" + "github.com/ava-labs/simplex/simplex" "github.com/ava-labs/simplex/testutil" "github.com/stretchr/testify/require" ) @@ -435,8 +436,7 @@ func TestMSMNormalOp(t *testing.T) { md.Round++ md.Prev = lastBlock.Digest() - var blacklist common.Blacklist - blacklist.NodeCount = 4 + blacklist := common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds) blockTime := lastBlock.InnerBlock.Timestamp().Add(time.Second) @@ -1228,7 +1228,7 @@ func TestVerifyDoesNotInitializeApprovalStore(t *testing.T) { tc.blockBuilder.Block = &testutil.InnerBlock{TS: blockTime, BlockHeight: lastBlock.InnerBlock.Height()} // Build a transition block - block, err := sm.BuildBlock(context.Background(), md, common.Blacklist{NodeCount: 4}) + block, err := sm.BuildBlock(context.Background(), md, common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds)) require.NoError(t, err) require.Equal(t, newPChainHeight, block.Metadata.SimplexEpochInfo.NextPChainReferenceHeight) @@ -1242,7 +1242,7 @@ func TestVerifyDoesNotInitializeApprovalStore(t *testing.T) { next.Round++ next.Prev = block.Digest() tc.blockBuilder.Block = &testutil.InnerBlock{TS: blockTime, BlockHeight: block.InnerBlock.Height() + 1} - collecting, err := sm.BuildBlock(context.Background(), next, common.Blacklist{NodeCount: 4}) + collecting, err := sm.BuildBlock(context.Background(), next, common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds)) require.NoError(t, err) return avalanchego.BitmaskFromBytes(collecting.Metadata.SimplexEpochInfo.NextEpochApprovals.NodeIDs) } diff --git a/simplex/empty_block_builder_test.go b/simplex/empty_block_builder_test.go index b15e12c7..f317128a 100644 --- a/simplex/empty_block_builder_test.go +++ b/simplex/empty_block_builder_test.go @@ -45,7 +45,7 @@ func TestEmptyBlockBuilderReturnsRealBlockWhenBuilt(t *testing.T) { } bb.TriggerNewBlock() - block, ok := ebb.BuildBlock(context.Background(), md, common.NewBlacklist(5)) + block, ok := ebb.BuildBlock(context.Background(), md, common.NewBlacklist(5, simplex.DefaultMaxBlacklistedRounds)) require.True(t, ok) require.NotNil(t, block) require.Equal(t, md.Seq, block.BlockHeader().Seq) @@ -67,7 +67,7 @@ func TestEmptyBlockBuilderReturnsEmptyBlockWhenShouldBuildEmpty(t *testing.T) { ShouldBuildEmptyBlock: blocksUntilDone, } - block, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1)) + block, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1, simplex.DefaultMaxBlacklistedRounds)) require.True(t, ok) require.NotNil(t, block) } @@ -86,7 +86,7 @@ func TestEmptyBlockBuilderReturnsNoBlockOnCallerCancel(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() // caller cancels before the call. - block, ok := ebb.BuildBlock(ctx, common.ProtocolMetadata{}, common.NewBlacklist(1)) + block, ok := ebb.BuildBlock(ctx, common.ProtocolMetadata{}, common.NewBlacklist(1, simplex.DefaultMaxBlacklistedRounds)) require.False(t, ok) require.Nil(t, block) } @@ -116,7 +116,7 @@ func TestEmptyBlockBuilderDoesNotCancelWhenShouldBuildReturnsFalse(t *testing.T) bb.TriggerNewBlock() }() - block, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1)) + block, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1, simplex.DefaultMaxBlacklistedRounds)) require.True(t, ok) require.NotNil(t, block) } @@ -138,7 +138,7 @@ func TestEmptyBlockBuilderCleansUpAfterBuild(t *testing.T) { } bb.TriggerNewBlock() - _, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1)) + _, ok := ebb.BuildBlock(context.Background(), common.ProtocolMetadata{}, common.NewBlacklist(1, simplex.DefaultMaxBlacklistedRounds)) require.True(t, ok) select { diff --git a/simplex/epoch.go b/simplex/epoch.go index 36c2b3c0..8ea32c3c 100644 --- a/simplex/epoch.go +++ b/simplex/epoch.go @@ -28,6 +28,9 @@ var ( ) const ( + // DefaultMaxBlacklistedRounds is the default maximum number of blocks before a blacklisted node is redeemed. + // It equals the value the chain starts with, so nodes using the default do not move it. + DefaultMaxBlacklistedRounds = common.GenesisMaxBlacklistedRounds DefaultMaxRoundWindow = 10 DefaultProcessingBlocks = 500 DefaultMaxProposalWaitTime = 5 * time.Second @@ -67,6 +70,7 @@ func NewRound(block common.VerifiedBlock) *Round { } type EpochConfig struct { + MaxBlacklistedRounds uint16 MaxProposalWait time.Duration MaxRoundWindow uint64 MaxReplicationResponseSize int @@ -259,6 +263,9 @@ func (e *Epoch) init() error { } func (e *Epoch) maybeAssignDefaultConfig() error { + if e.MaxBlacklistedRounds == 0 { + e.MaxBlacklistedRounds = DefaultMaxBlacklistedRounds + } if e.MaxProposalWait == 0 { e.MaxProposalWait = DefaultMaxProposalWaitTime } @@ -1664,7 +1671,7 @@ func (e *Epoch) maybeMarkLeaderAsTimedOutForFutureBlacklisting(emptyNotarization } blacklist = e.lastBlock.VerifiedBlock.Blacklist() if e.lastBlock.Finalization.Finalization.Epoch < e.Epoch { - blacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + blacklist = blacklist.Stateless(uint16(len(e.validators))) } } round := emptyNotarization.Vote.Round @@ -2477,7 +2484,7 @@ func (e *Epoch) verifyProposalMetadataAndBlacklist(block common.Block) bool { // Else, either it's not the first block, or we haven't committed the first block, and it is the first block. // If it's the latter we have nothing else to do. // If it's the former, we need to find the parent of the block and ensure it is correct. - prevBlacklist := common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + prevBlacklist := common.NewBlacklist(uint16(len(e.validatorNodeIDs)), common.GenesisMaxBlacklistedRounds) if bh.Seq > 0 { prevBlock, _, found := e.locateBlock(bh.Seq-1, bh.Prev[:]) if !found { @@ -2488,18 +2495,14 @@ func (e *Epoch) verifyProposalMetadataAndBlacklist(block common.Block) bool { return false } - expectedSeq = bh.Seq - expectedPrevDigest = bh.Prev + expectedSeq = prevBlock.BlockHeader().Seq + 1 + expectedPrevDigest = prevBlock.BlockHeader().Digest prevBlacklist = prevBlock.Blacklist() // If the previous block belongs to an earlier epoch, the blacklist should be empty. - if prevBlock.BlockHeader().Epoch < e.Epoch { - prevBlacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) - } - - if prevBlacklist.IsEmpty() { - prevBlacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + if prevBlock.BlockHeader().Epoch < e.Epoch || prevBlacklist.IsEmpty() { + prevBlacklist = prevBlacklist.Stateless(uint16(len(e.validators))) } } @@ -2606,20 +2609,11 @@ func (e *Epoch) buildBlock() { } // Create the blacklist for the next round: - - // 1) Reset the updates from the previous round. - prevBlacklist.Updates = nil - // 2) Compute the updates for the new round, according to what we have observed. - e.Logger.Debug("Computing blacklist updates", - zap.String("timedOutRounds", fmt.Sprintf("%v", e.timedOutRounds)), - zap.String("redeemedRounds", fmt.Sprintf("%v", e.redeemedRounds))) - updates := prevBlacklist.ComputeBlacklistUpdates(metadata.Round, uint16(len(e.validatorNodeIDs)), e.timedOutRounds, e.redeemedRounds) - // 3) Apply the updates to the blacklist. - nextBlacklist := prevBlacklist.ApplyUpdates(updates, metadata.Round) + nextBlacklist := prevBlacklist.Next(metadata.Round, e.MaxBlacklistedRounds, e.timedOutRounds, e.redeemedRounds) e.Logger.Debug("Blacklist updated", zap.Uint64("round", metadata.Round), - zap.String("Update", common.BlacklistUpdatesAsString(updates)), + zap.Stringer("blacklist", &nextBlacklist), zap.Stringer("prev", &prevBlacklist), zap.Stringer("next", &nextBlacklist)) buildTheBlock := e.createBlockBuildingTask(metadata, nextBlacklist) @@ -2655,12 +2649,12 @@ func (e *Epoch) retrieveBlacklistForBlockBuilding(metadata common.ProtocolMetada // If the previous block belongs to a previous epoch, we need to reset the blacklist to be empty. if metadata.Epoch > prevBlock.BlockHeader().Epoch { - blacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + blacklist = blacklist.Stateless(uint16(len(e.validators))) } } if blacklist.IsEmpty() { - blacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + blacklist = blacklist.Stateless(uint16(len(e.validators))) } return blacklist, true @@ -2982,7 +2976,7 @@ func (e *Epoch) retrieveLastPersistedBlacklist() (common.Blacklist, bool) { blacklist = e.lastBlock.VerifiedBlock.Blacklist() if e.lastBlock.Finalization.Finalization.Epoch < e.Epoch { - blacklist = common.NewBlacklist(uint16(len(e.validatorNodeIDs))) + blacklist = blacklist.Stateless(uint16(len(e.validators))) } } return blacklist, true diff --git a/simplex/epoch_failover_test.go b/simplex/epoch_failover_test.go index 59cffa2d..30320322 100644 --- a/simplex/epoch_failover_test.go +++ b/simplex/epoch_failover_test.go @@ -984,19 +984,15 @@ func TestEpochBlacklist(t *testing.T) { block, _ := notarizeAndFinalizeRound(t, e, bb) require.Equal(t, nodes[0], LeaderForRound(nodes, block.BlockHeader().Round)) // our node will propose the empty block - // Make sure our node properly constructed the blacklist + // Make sure our node properly constructed the blacklist, accusing node 3 of timing out + timedOut := map[uint16]uint64{3: timedOutRound} actual := block.Blacklist() - expected := Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{{NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1}}, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}}, - } + expected := emptyBlacklist.Next(block.BlockHeader().Round, DefaultMaxBlacklistedRounds, timedOut, nil) require.True(t, expected.Equals(&actual)) - // Compute next blacklist - blacklist := NewBlacklist(4) - blacklist.Updates = []BlacklistUpdate{{Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}} - blacklist.SuspectedNodes = []SuspectedNode{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 1}} + // Compute next blacklist, in which node 2 accuses node 3 as well + blacklist := actual.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, timedOut, nil) + require.True(t, blacklist.IsNodeSuspected(3)) // Build the block and prime it to be proposed in the next round bb.BuildBlock(context.Background(), e.Metadata(), blacklist) @@ -1010,7 +1006,7 @@ func TestEpochBlacklist(t *testing.T) { // Do it again but preserve the blacklist until we get to the round of the last node. // This time, there are no updates to the blacklist, it just carries over. - blacklist.Updates = nil + blacklist = blacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, nil, nil) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) notarizeAndFinalizeRound(t, e, bb) @@ -1054,12 +1050,12 @@ func TestEpochBlacklist(t *testing.T) { wal.AssertNotarization(8) // Now node 2 proposes a block. - blacklist.Updates = nil + blacklist = blacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, nil, nil) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) notarizeAndFinalizeRound(t, e, bb) // Now node 3 proposes a block. - blacklist.Updates = nil + blacklist = blacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, nil, nil) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) notarizeAndFinalizeRound(t, e, bb) @@ -1085,19 +1081,9 @@ func TestEpochBlacklist(t *testing.T) { block = bb.GetBuiltBlock() - expected = Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{ - { - NodeIndex: 3, - SuspectingCount: 2, - OrbitSuspected: 1, - RedeemingCount: 1, - OrbitToRedeem: Orbit(12, 3, 4), - }, - }, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, - } + // Node 3 voted in round 8, which redeems it. + redeemed := map[uint16]uint64{3: 8} + expected = blacklist.Next(block.BlockHeader().Round, DefaultMaxBlacklistedRounds, nil, redeemed) actual = block.Blacklist() require.True(t, expected.Equals(&actual), "Node should vote to redeem the previously failed node") @@ -1105,20 +1091,17 @@ func TestEpochBlacklist(t *testing.T) { bb.SetBuiltBlock(block.(*testutil.TestBlock)) // Insert the block built by the block builder back into block builder so it will re-propose it notarizeAndFinalizeRound(t, e, bb) - // The next blacklist garbage collects node 3 from the blacklist. - - blacklist = Blacklist{ - NodeCount: 4, - Updates: []BlacklistUpdate{{Type: BlacklistOpType_NodeRedeemed, NodeIndex: 3}}, - } + // The next blacklist redeems node 3 with f+1 votes and removes it from the blacklist. + redeemed = map[uint16]uint64{3: e.Metadata().Round} + blacklist = actual.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, nil, redeemed) + require.False(t, blacklist.IsNodeSuspected(3)) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) notarizeAndFinalizeRound(t, e, bb) // Node 4 can now propose a block - blacklist = Blacklist{ - NodeCount: 4, - } + blacklist = blacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, nil, nil) + require.True(t, emptyBlacklist.Equals(&blacklist)) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) notarizeAndFinalizeRound(t, e, bb) @@ -1305,7 +1288,7 @@ func TestNodeIsNotSuspectedAfterSigningALaterNotarization(t *testing.T) { // notarizes the round, so the block stays in the rounds map without a notarization. storeProposalAndEmptyNotarize := func(round uint64) *testutil.TestBlock { leader := LeaderForRound(nodes, round) - built, ok := bb.BuildBlock(context.Background(), e.Metadata(), Blacklist{NodeCount: uint16(len(validators))}) + built, ok := bb.BuildBlock(context.Background(), e.Metadata(), NewBlacklist(uint16(len(validators)), e.MaxBlacklistedRounds)) require.True(t, ok) block := built.(*testutil.TestBlock) @@ -1368,11 +1351,13 @@ func TestNodeIsNotSuspectedAfterSigningALaterNotarization(t *testing.T) { } }, 5*time.Second, 10*time.Millisecond, "we never proposed a block for round 4") - for _, update := range proposal.Blacklist().Updates { - require.False(t, update.NodeIndex == suspectIndex && update.Type == BlacklistOpType_NodeSuspected, - "accused node %d of timing out in round 1, even though it signed the notarization for round 2", - suspectIndex) - } + // Only the leaders of rounds 2 and 3 are accused, and not nodes[suspectIndex] for timing out in round 1. + parentBlacklist := roundTwoBlock.Blacklist() + expected := parentBlacklist.Next(4, e.MaxBlacklistedRounds, map[uint16]uint64{2: 2, 3: 3}, nil) + actual := proposal.Blacklist() + require.True(t, expected.Equals(&actual), + "accused node %d of timing out in round 1, even though it signed the notarization for round 2: %s", + suspectIndex, actual.String()) } // TestEmptyVoteNotDuplicatedAfterFailedVerification asserts that a round contributes at most one diff --git a/simplex/epoch_multinode_test.go b/simplex/epoch_multinode_test.go index 7b37429e..937f7f15 100644 --- a/simplex/epoch_multinode_test.go +++ b/simplex/epoch_multinode_test.go @@ -4,6 +4,7 @@ package simplex_test import ( + "context" "fmt" "sync" "sync/atomic" @@ -182,18 +183,17 @@ func TestSimplexMultiNodeBlacklist(t *testing.T) { // Build two more blocks, which should blacklist the fourth node. // Ensure the fourth node is blacklisted by checking the blacklist on all nodes. + timedOut := map[uint16]uint64{3: 3} + emptyBlacklist := common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds) + accused := emptyBlacklist.Next(4, simplex.DefaultMaxBlacklistedRounds, timedOut, nil) + blacklisted := accused.Next(5, simplex.DefaultMaxBlacklistedRounds, timedOut, nil) // Build a block, ensure the blacklist contains the fourth node as a suspect. net.TriggerLeaderBlockBuilder(uint64(4)) for _, n := range net.Instances[:3] { block := n.Storage.WaitForBlockCommit(uint64(3)) blacklist := block.Blacklist() - expectedBlacklist := common.Blacklist{ - NodeCount: 4, - SuspectedNodes: common.SuspectedNodes{{NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: 1}}, - Updates: []common.BlacklistUpdate{{NodeIndex: 3, Type: common.BlacklistOpType_NodeSuspected}}, - } - require.True(t, expectedBlacklist.Equals(&blacklist)) + require.True(t, accused.Equals(&blacklist)) } // Reconnect the fourth node but don't let it un-blacklist itself prematurely. @@ -205,24 +205,14 @@ func TestSimplexMultiNodeBlacklist(t *testing.T) { for _, n := range net.Instances[:3] { block := n.Storage.WaitForBlockCommit(uint64(4)) blacklist := block.Blacklist() - expectedBlacklist := common.Blacklist{ - NodeCount: 4, - SuspectedNodes: common.SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 1}}, - Updates: []common.BlacklistUpdate{{NodeIndex: 3, Type: common.BlacklistOpType_NodeSuspected}}, - } - require.True(t, expectedBlacklist.Equals(&blacklist)) + require.True(t, blacklisted.Equals(&blacklist)) } // Wait for the node to replicate the missing blocks. net.Instances[3].TriggerNewBlock() block := net.Instances[3].Storage.WaitForBlockCommit(4) blacklist := block.Blacklist() - expectedBlacklist := common.Blacklist{ - NodeCount: 4, - SuspectedNodes: common.SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 1}}, - Updates: []common.BlacklistUpdate{{NodeIndex: 3, Type: common.BlacklistOpType_NodeSuspected}}, - } - require.True(t, expectedBlacklist.Equals(&blacklist)) + require.True(t, blacklisted.Equals(&blacklist)) // Make another block. net.Instances[2].TriggerNewBlock() @@ -410,3 +400,147 @@ func onlyAllowEmptyRoundMessages(msg *common.Message, _, _ common.NodeID) bool { } return false } + +// TestSimplexMultiNodeBlacklistExpires tests that a node that goes offline is blacklisted, and that it is +// removed from the blacklist by all nodes once it has been blacklisted for more than MaxBlacklistedOrbits, +// even though it is still offline and therefore never redeemed itself. +func TestSimplexMultiNodeBlacklistExpires(t *testing.T) { + nodes := []common.NodeID{{1}, {2}, {3}, {4}} + net := testutil.NewControlledNetwork(t, nodes) + + // A node may remain blacklisted for a single orbit. + const maxBlacklistedRounds = 4 + + // The chain starts from GenesisMaxBlacklistedRounds, and only moves towards the local configuration + // by a single orbit per block, so the nodes start from a finalized block of round 0 whose blacklist + // already has the configured max blacklisted orbits. + genesis, ok := testutil.NewTestBlockBuilder().BuildBlock(context.Background(), common.ProtocolMetadata{}, + common.NewBlacklist(uint16(len(nodes)), maxBlacklistedRounds)) + require.True(t, ok) + genesisFinalization, _ := testutil.NewFinalizationRecord(t, &testutil.TestSignatureAggregator{N: len(nodes)}, genesis, nodes) + + testEpochConfig := &testutil.TestNodeConfig{ + ReplicationEnabled: true, + MaxBlacklistedRounds: maxBlacklistedRounds, + InitialStorage: []common.VerifiedFinalizedBlock{{VerifiedBlock: genesis, Finalization: genesisFinalization}}, + } + for _, node := range nodes { + testutil.NewControlledSimplexNode(t, node, net, testEpochConfig) + } + + net.StartInstances() + defer net.StopInstances() + + const offlineNodeIndex = 3 + onlineNodes := net.Instances[:offlineNodeIndex] + + // buildBlock lets the leader of the given round build the block with the given sequence, + // waits for the online nodes to commit it, and returns its blacklist. + buildBlock := func(round, seq uint64) common.Blacklist { + net.TriggerLeaderBlockBuilder(round) + var blacklist common.Blacklist + for _, n := range onlineNodes { + block := n.Storage.WaitForBlockCommit(seq) + require.Equal(t, round, block.BlockHeader().Round) + blacklist = block.Blacklist() + } + return blacklist + } + + // timeoutOnOfflineLeader lets the online nodes time out on the offline leader of the given round. + timeoutOnOfflineLeader := func(round uint64) { + require.Equal(t, nodes[offlineNodeIndex], simplex.LeaderForRound(nodes, round)) + for _, n := range onlineNodes { + testutil.WaitToEnterRound(t, n.E, round) + n.BlockShouldBeBuilt() + } + for _, n := range onlineNodes { + testutil.WaitForBlockProposerTimeout(t, n.E, &n.E.StartTime, round) + } + } + + // skipBlacklistedLeader asserts that the online nodes do not wait for the blacklisted leader + // of the given round to propose a block. Time is not advanced, so the round can only be + // empty notarized if the nodes immediately vote for an empty block. + skipBlacklistedLeader := func(round uint64) { + require.Equal(t, nodes[offlineNodeIndex], simplex.LeaderForRound(nodes, round)) + for _, n := range onlineNodes { + testutil.WaitToEnterRound(t, n.E, round) + n.BlockShouldBeBuilt() + } + for _, n := range onlineNodes { + require.Eventually(t, func() bool { + return n.WAL.ContainsEmptyVote(round) || n.WAL.ContainsEmptyNotarization(round) + }, time.Minute, 10*time.Millisecond, "node %s waited for the blacklisted leader of round %d", n.E.ID, round) + } + } + + // Rounds 1-2: ordinary blocks proposed by nodes 1 and 2, on top of the finalized block of round 0. + for seq := uint64(1); seq < 3; seq++ { + buildBlock(seq, seq) + } + + // The last node goes offlineNodeIndex for the rest of the test. + net.Disconnect(nodes[offlineNodeIndex]) + + // Round 3: the online nodes time out on the offlineNodeIndex node. + timeoutOnOfflineLeader(3) + + // The blacklists the online nodes are expected to build, as the offline node timed out in round 3. + emptyBlacklist := common.NewBlacklist(uint16(len(nodes)), testEpochConfig.MaxBlacklistedRounds) + timedOut := map[uint16]uint64{offlineNodeIndex: 3} + accused := emptyBlacklist.Next(4, testEpochConfig.MaxBlacklistedRounds, timedOut, nil) + blacklisted := accused.Next(5, testEpochConfig.MaxBlacklistedRounds, timedOut, nil) + + // Round 4: node 0 accuses the offline node of timing out in its orbit 1. + blacklist := buildBlock(4, 3) + require.Equal(t, uint16(1), blacklist.MaxBlacklistedOrbits()) + require.False(t, blacklist.IsNodeSuspected(offlineNodeIndex)) + require.True(t, accused.Equals(&blacklist), "expected %s, got %s", accused.String(), blacklist.String()) + + // Round 5: node 1 accuses the offline node as well, which is then blacklisted with f+1 accusations in its orbit 1. + blacklist = buildBlock(5, 4) + require.True(t, blacklist.IsNodeSuspected(offlineNodeIndex)) + require.True(t, blacklisted.Equals(&blacklist), "expected %s, got %s", blacklisted.String(), blacklist.String()) + + // Round 6: node 2 carries over the blacklist. + blacklist = buildBlock(6, 5) + require.True(t, blacklist.IsNodeSuspected(offlineNodeIndex)) + + // Round 7: the offline node's turn, in its orbit 2. It is blacklisted, so it is skipped. + // It has now been blacklisted for a single orbit, but the blacklist is only garbage collected + // when a block is built, so it is still skipped in this round. + skipBlacklistedLeader(7) + + // Round 8: node 0 builds a block, which garbage collects the offline node, although it never redeemed itself. + blacklist = buildBlock(8, 6) + require.False(t, blacklist.IsNodeSuspected(offlineNodeIndex)) + require.True(t, emptyBlacklist.Equals(&blacklist), "expected an empty blacklist, got %s", blacklist.String()) + + // Rounds 9 and 10: nodes 1 and 2 carry over the empty blacklist. + for round, seq := uint64(9), uint64(7); round <= 10; round, seq = round+1, seq+1 { + blacklist = buildBlock(round, seq) + require.True(t, emptyBlacklist.Equals(&blacklist), "round %d: expected an empty blacklist, got %s", round, blacklist.String()) + } + + // Round 11: the offline node's turn. It is no longer blacklisted, so the online nodes wait for its block + // rather than skip it, and only vote for an empty block once they time out on it. + for _, n := range onlineNodes { + testutil.WaitToEnterRound(t, n.E, 11) + n.BlockShouldBeBuilt() + } + + // Since we don't advance the time, the online nodes should not vote for an empty block just yet. + // This ensures that the offline node is no longer blacklisted, and that the online nodes are waiting for it to propose a block. + require.Never(t, func() bool { + for _, n := range onlineNodes { + if n.WAL.ContainsEmptyVote(11) { + return true + } + } + return false + }, 500*time.Millisecond, 10*time.Millisecond, "the online nodes skipped the offlineNodeIndex node although it is no longer blacklisted") + + // Now we can timeout on the offline node and finish the test. + timeoutOnOfflineLeader(11) +} diff --git a/simplex/epoch_sealed_test.go b/simplex/epoch_sealed_test.go index a348dbb1..0ef937b0 100644 --- a/simplex/epoch_sealed_test.go +++ b/simplex/epoch_sealed_test.go @@ -244,7 +244,7 @@ func setupSealingBlockTest(t *testing.T, epochLeaderRound uint64, markSealing bo epochNode := LeaderForRound(nodes, epochLeaderRound) quorum := Quorum(len(nodes)) - blacklist := Blacklist{NodeCount: uint16(len(nodes)), SuspectedNodes: SuspectedNodes{}, Updates: []BlacklistUpdate{}} + blacklist := NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds) bb := testutil.NewTestBlockBuilder() comm := &recordingComm{ diff --git a/simplex/epoch_test.go b/simplex/epoch_test.go index 7caec1e3..7c3ce811 100644 --- a/simplex/epoch_test.go +++ b/simplex/epoch_test.go @@ -25,13 +25,15 @@ import ( ) var ( - emptyBlacklist = Blacklist{ - NodeCount: 4, - SuspectedNodes: SuspectedNodes{}, - Updates: []BlacklistUpdate{}, - } + emptyBlacklist = newEmptyBlacklist() ) +// newEmptyBlacklist returns the blacklist that a leader of a network of four nodes proposes when it has nothing to report. +func newEmptyBlacklist() Blacklist { + blacklist := NewBlacklist(4, DefaultMaxBlacklistedRounds) + return blacklist.Next(0, DefaultMaxBlacklistedRounds, nil, nil) +} + // TestFinalizeSameSequence tests that a node only verifies a block after it receives all the empty notarizations the block depends on. // This means the node can(and should) send a finalize vote for the same sequence if and only if a valid empty notarization has been verified. // @@ -82,9 +84,7 @@ func TestFinalizeSameSequence(t *testing.T) { Seq: 1, // set next seq to 1 not 2 Prev: initialBlock.VerifiedBlock.BlockHeader().Digest, } - vb, ok := bb.BuildBlock(context.Background(), md, Blacklist{ - NodeCount: uint16(len(e.Comm.Validators())), - }) + vb, ok := bb.BuildBlock(context.Background(), md, NewBlacklist(uint16(len(e.Comm.Validators())), e.MaxBlacklistedRounds)) require.True(t, ok) block := vb.(*testutil.TestBlock) @@ -225,9 +225,7 @@ func testFinalizeSameSequenceGap(t *testing.T, nodes []NodeID, numEmptyNotarizat Seq: seqToDoubleFinalize, Prev: finalizeVoteSeqs[seqToDoubleFinalize-1].Finalization.Digest, } - vb, ok := bb.BuildBlock(context.Background(), md, Blacklist{ - NodeCount: uint16(len(e.Comm.Validators())), - }) + vb, ok := bb.BuildBlock(context.Background(), md, NewBlacklist(uint16(len(e.Comm.Validators())), e.MaxBlacklistedRounds)) require.True(t, ok) block := vb.(*testutil.TestBlock) @@ -1069,13 +1067,14 @@ func TestEpochResizesBlacklistOnEpochChange(t *testing.T) { // The epoch number is the sequence of the last indexed sealing block, so both nodes // below start in epoch 1 while their last indexed block belongs to epoch 0. - epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(1)) - sealingBlock := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 1, Seq: 1, Prev: epoch0Block.Digest}, NewBlacklist(1)) + epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(1, DefaultMaxBlacklistedRounds)) + sealingBlock := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 1, Seq: 1, Prev: epoch0Block.Digest}, NewBlacklist(1, DefaultMaxBlacklistedRounds)) sealingBlock.SealingInfo = &SealingBlockInfo{ ValidatorSet: NodeIDs(nodes).EqualWeightedNodes(), PrevSealingBlockHash: epoch0Block.Digest, } - require.Equal(t, uint16(1), sealingBlock.Blacklist().NodeCount, + sealingBlacklist := sealingBlock.Blacklist() + require.Equal(t, uint16(1), sealingBlacklist.NodeCount(), "blacklist must contain exactly one node") sigAggregator := &testutil.TestSignatureAggregator{N: len(nodes)} @@ -1096,7 +1095,8 @@ func TestEpochResizesBlacklistOnEpochChange(t *testing.T) { // parent (1), otherwise its blacklist is malformed and the block cannot be notarized. bb.BlockShouldBeBuilt <- struct{}{} block := bb.GetBuiltBlock() - require.Equal(t, uint16(2), block.Blacklist().NodeCount, + blockBlacklist := block.Blacklist() + require.Equal(t, uint16(2), blockBlacklist.NodeCount(), "blacklist must be resized to the new epoch's validator count") e.Stop() @@ -1146,15 +1146,13 @@ func TestEpochBlacklistCapBypassedByPendingSuspects(t *testing.T) { notarizeAndFinalizeRound(t, e, bb) block, _ := notarizeAndFinalizeRound(t, e, bb) - accuseNodes0And1 := []BlacklistUpdate{ - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 0}, - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 1}, - } + // Pretend nodes 0 and 1 timed out in round 2, so that the leaders accuse them. + accuseNodes0And1 := map[uint16]uint64{0: 2, 1: 2} // Round 2: node 2 proposes a block accusing nodes 0 and 1. // Nothing is blacklisted yet, so both are added as pending with a single accusation each. prevBlacklist := block.Blacklist() - blacklist := prevBlacklist.ApplyUpdates(accuseNodes0And1, e.Metadata().Round) + blacklist := prevBlacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, accuseNodes0And1, nil) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) block, _ = notarizeAndFinalizeRound(t, e, bb) require.Equal(t, nodes[2], LeaderForRound(nodes, block.BlockHeader().Round)) @@ -1162,12 +1160,12 @@ func TestEpochBlacklistCapBypassedByPendingSuspects(t *testing.T) { actual := block.Blacklist() require.False(t, actual.IsNodeSuspected(0)) require.False(t, actual.IsNodeSuspected(1)) - require.Len(t, actual.SuspectedNodes, 2) + require.True(t, blacklist.Equals(&actual)) // Round 3: node 3 accuses nodes 0 and 1 again, still within the same orbit. // Each pending entry is incremented to f+1 without re-checking the cap. prevBlacklist = block.Blacklist() - blacklist = prevBlacklist.ApplyUpdates(accuseNodes0And1, e.Metadata().Round) + blacklist = prevBlacklist.Next(e.Metadata().Round, DefaultMaxBlacklistedRounds, accuseNodes0And1, nil) bb.BuildBlock(context.Background(), e.Metadata(), blacklist) block, _ = notarizeAndFinalizeRound(t, e, bb) require.Equal(t, nodes[3], LeaderForRound(nodes, block.BlockHeader().Round)) @@ -2314,7 +2312,7 @@ func TestEpochReplicatedNotarizationVerifiedAfterFinalizationIsNotFinalized(t *t // To ensure block1 has finished verifying, we send our node block3. Because the verification scheduler only verifies // one block at a time, we know that if block3 is verified, block1's verification task must have completed(it was enqueued before block3). blacklist2 := block2.Blacklist() - _, built = bb.BuildBlock(ctx, e.Metadata(), blacklist2.ApplyUpdates(nil, 3)) + _, built = bb.BuildBlock(ctx, e.Metadata(), blacklist2.Next(3, DefaultMaxBlacklistedRounds, nil, nil)) require.True(t, built) block3 := bb.GetBuiltBlock() vote3, err := testutil.NewTestVote(block3, nodes[3]) @@ -2561,9 +2559,7 @@ func advanceRound(t *testing.T, e *Epoch, bb *testutil.TestBlockBuilder, notariz // only create blocks if we are not the node running the epoch isEpochNode := leader.Equals(e.ID) if !isEpochNode { - _, ok := bb.BuildBlock(context.Background(), md, Blacklist{ - NodeCount: uint16(len(e.Comm.Validators())), - }) + _, ok := bb.BuildBlock(context.Background(), md, NewBlacklist(uint16(len(e.Comm.Validators())), e.MaxBlacklistedRounds)) require.True(t, ok) } @@ -3011,7 +3007,7 @@ func notarizeRoundNotFinalized(t *testing.T, e *Epoch, nodes []NodeID, round uin md := e.Metadata() require.Equal(t, round, md.Round) - block := testutil.NewTestBlock(md, NewBlacklist(uint16(len(nodes)))) + block := testutil.NewTestBlock(md, NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds)) vote, err := testutil.NewTestVote(block, leader) require.NoError(t, err) @@ -3038,7 +3034,7 @@ func notarizeRoundNotFinalized(t *testing.T, e *Epoch, nodes []NodeID, round uin // The genuine quorum round for the same block is then accepted. func TestEpochRejectsReplicatedQuorumRoundWithMismatchedHeader(t *testing.T) { nodes := []NodeID{{1}, {2}, {3}, {4}} - blacklist := Blacklist{NodeCount: uint16(len(nodes)), SuspectedNodes: SuspectedNodes{}, Updates: []BlacklistUpdate{}} + blacklist := NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds) mutations := []struct { name string @@ -3446,7 +3442,7 @@ func TestFutureProposalDispatchedOnceAfterReentrantCommit(t *testing.T) { // The epoch node leads only round 9, so it never proposes any of the blocks used here. epochNode := LeaderForRound(nodes, 9) quorum := Quorum(len(nodes)) - blacklist := Blacklist{NodeCount: uint16(len(nodes)), SuspectedNodes: SuspectedNodes{}, Updates: []BlacklistUpdate{}} + blacklist := NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds) bb := testutil.NewTestBlockBuilder() comm := &recordingComm{ @@ -3716,10 +3712,14 @@ func TestEpochFollowerIgnoresStaleBlacklistOnEpochChange(t *testing.T) { // The sealing block is at round 1, so the new epoch starts at round 2, led by nodes[2]. // The previous epoch blacklists that very node (f=1, so two accusations suffice). - staleBlacklist := NewBlacklist(uint16(len(nodes))) - staleBlacklist.SuspectedNodes = SuspectedNodes{{NodeIndex: 2, SuspectingCount: 2, OrbitSuspected: 1}} - - epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(uint16(len(nodes)))) + staleBlacklist := NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds) + // Two leaders of orbit 1 accuse nodes[2] of timing out in round 2. + staleTimeout := map[uint16]uint64{2: 2} + staleBlacklist = staleBlacklist.Next(2, DefaultMaxBlacklistedRounds, staleTimeout, nil) + staleBlacklist = staleBlacklist.Next(3, DefaultMaxBlacklistedRounds, staleTimeout, nil) + require.True(t, staleBlacklist.IsNodeSuspected(2)) + + epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds)) sealingBlock := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 1, Seq: 1, Prev: epoch0Block.Digest}, staleBlacklist) sealingBlock.SealingInfo = &SealingBlockInfo{ ValidatorSet: NodeIDs(nodes).EqualWeightedNodes(), @@ -3751,7 +3751,7 @@ func TestEpochFollowerIgnoresStaleBlacklistOnEpochChange(t *testing.T) { e.AdvanceTime(e.StartTime.Add(conf.MaxProposalWait / 2)) // The leader proposes the first block of the new epoch, with an empty blacklist. - block := testutil.NewTestBlock(md, NewBlacklist(uint16(len(nodes)))) + block := testutil.NewTestBlock(md, NewBlacklist(uint16(len(nodes)), DefaultMaxBlacklistedRounds)) vote, err := testutil.NewTestVote(block, leader) require.NoError(t, err) require.NoError(t, e.HandleMessage(&Message{ @@ -3780,10 +3780,14 @@ func TestEpochLeaderRecordsTimeoutDespiteStaleBlacklistOnEpochChange(t *testing. // The sealing block is at round 1, so the new epoch starts at round 2, led by nodes[2]. // The previous epoch blacklists that very node. - staleBlacklist := NewBlacklist(nodeCount) - staleBlacklist.SuspectedNodes = SuspectedNodes{{NodeIndex: 2, SuspectingCount: 2, OrbitSuspected: 1}} - - epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(nodeCount)) + staleBlacklist := NewBlacklist(nodeCount, DefaultMaxBlacklistedRounds) + // Two leaders of orbit 1 accuse nodes[2] of timing out in round 2. + staleTimeout := map[uint16]uint64{2: 2} + staleBlacklist = staleBlacklist.Next(2, DefaultMaxBlacklistedRounds, staleTimeout, nil) + staleBlacklist = staleBlacklist.Next(3, DefaultMaxBlacklistedRounds, staleTimeout, nil) + require.True(t, staleBlacklist.IsNodeSuspected(2)) + + epoch0Block := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 0, Seq: 0}, NewBlacklist(nodeCount, DefaultMaxBlacklistedRounds)) sealingBlock := testutil.NewTestBlock(ProtocolMetadata{Epoch: 0, Round: 1, Seq: 1, Prev: epoch0Block.Digest}, staleBlacklist) sealingBlock.SealingInfo = &SealingBlockInfo{ ValidatorSet: NodeIDs(nodes).EqualWeightedNodes(), @@ -3825,17 +3829,9 @@ func TestEpochLeaderRecordsTimeoutDespiteStaleBlacklistOnEpochChange(t *testing. round := block.BlockHeader().Round require.Equal(t, nodes[0], LeaderForRound(nodes, round)) - expected := Blacklist{ - NodeCount: nodeCount, - SuspectedNodes: SuspectedNodes{ - {NodeIndex: 2, SuspectingCount: 1, OrbitSuspected: Orbit(round, 2, nodeCount)}, - {NodeIndex: 3, SuspectingCount: 1, OrbitSuspected: Orbit(round, 3, nodeCount)}, - }, - Updates: []BlacklistUpdate{ - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 2}, - {Type: BlacklistOpType_NodeSuspected, NodeIndex: 3}, - }, - } + // The new epoch starts from a fresh blacklist, and our node accuses the leaders of rounds 2 and 3. + freshBlacklist := NewBlacklist(nodeCount, DefaultMaxBlacklistedRounds) + expected := freshBlacklist.Next(round, DefaultMaxBlacklistedRounds, map[uint16]uint64{2: 2, 3: 3}, nil) actual := block.Blacklist() require.True(t, expected.Equals(&actual), "the timeout of nodes[2] was not recorded: expected %s, got %s", expected.String(), actual.String()) @@ -4299,3 +4295,66 @@ func TestReplicatedNotarizationForEmptyNotarizedRound(t *testing.T) { require.True(t, wal.ContainsNotarization(0)) require.Equal(t, uint64(1), e.Metadata().Round) } + +// TestEpochRejectsProposalWithWrongSeq asserts that a proposal is rejected when +// its Seq is not one after the parent. +func TestEpochRejectsProposalWithWrongSeq(t *testing.T) { + for _, tc := range []struct { + name string + seqDelta uint64 + accepted bool + }{ + {name: "seq one past the parent is accepted", seqDelta: 1, accepted: true}, + {name: "seq two past the parent is rejected", seqDelta: 2, accepted: false}, + {name: "seq equal to the parent is rejected", seqDelta: 0, accepted: false}, + } { + t.Run(tc.name, func(t *testing.T) { + bb := testutil.NewTestBlockBuilder() + nodes := []NodeID{{1}, {2}, {3}, {4}} + conf, wal, _ := testutil.DefaultTestNodeEpochConfig(t, nodes[3], testutil.NewNoopComm(nodes), bb) + + e, err := NewEpoch(conf) + require.NoError(t, err) + t.Cleanup(e.Stop) + require.NoError(t, e.Start()) + + // Notarize rounds 0 and 1 without finalizing them, so that the parent of the next proposal + // is a block at a non-zero sequence that is still in memory, not in storage. + advanceRoundFromNotarization(t, e, bb) + parent, _ := advanceRoundFromNotarization(t, e, bb) + require.Equal(t, uint64(2), e.Metadata().Round) + require.Equal(t, uint64(1), parent.BlockHeader().Seq) + // The WAL holds the blocks of rounds 0 and 1 and their notarizations. + wal.AssertWALSize(4) + + // Propose a block for round 2 that extends the parent, but claims a sequence of our choosing. + md := e.Metadata() + require.Equal(t, parent.BlockHeader().Digest, md.Prev) + md.Seq = parent.BlockHeader().Seq + tc.seqDelta + block := testutil.NewTestBlock(md, NewBlacklist(uint16(len(nodes)), e.MaxBlacklistedRounds)) + + leader := LeaderForRound(nodes, md.Round) + vote, err := testutil.NewTestVote(block, leader) + require.NoError(t, err) + require.NoError(t, e.HandleMessage(&Message{ + BlockMessage: &BlockMessage{ + Vote: *vote, + Block: block, + }, + }, leader)) + + if tc.accepted { + // The block is stored once it has been verified. + wal.AssertBlockProposal(md.Round) + return + } + + // The proposal is dropped, so nothing is ever written for it. + require.Never(t, func() bool { + records, err := wal.ReadAll() + return err != nil || len(records) != 4 + }, 500*time.Millisecond, 10*time.Millisecond) + require.Equal(t, uint64(2), e.Metadata().Round) + }) + } +} diff --git a/simplex/recovery_test.go b/simplex/recovery_test.go index 5dfb609a..9a29419f 100644 --- a/simplex/recovery_test.go +++ b/simplex/recovery_test.go @@ -354,7 +354,6 @@ func TestWalWritesFinalization(t *testing.T) { // send and notarize a second block require.Equal(t, uint64(1), e.Metadata().Round) md := e.Metadata() - md.Seq++ md.Prev = firstBlock.BlockHeader().Digest _, ok := bb.BuildBlock(context.Background(), md, emptyBlacklist) require.True(t, ok) diff --git a/testutil/node.go b/testutil/node.go index 92516387..e02a97d9 100644 --- a/testutil/node.go +++ b/testutil/node.go @@ -213,6 +213,10 @@ func UpdateEpochConfig(epochConfig *simplex.EpochConfig, testConfig *TestNodeCon epochConfig.MaxRoundWindow = testConfig.MaxRoundWindow } + if testConfig.MaxBlacklistedRounds != 0 { + epochConfig.MaxBlacklistedRounds = testConfig.MaxBlacklistedRounds + } + if testConfig.Logger != nil { epochConfig.Logger = testConfig.Logger } @@ -242,6 +246,7 @@ type TestNodeConfig struct { SigAggregatorCreator common.SignatureAggregatorCreator ReplicationEnabled bool BlockBuilder *TestControlledBlockBuilder + MaxBlacklistedRounds uint16 // Long Running Tests MaxRoundWindow uint64 diff --git a/testutil/random_network/mempool_test.go b/testutil/random_network/mempool_test.go index c56f6af5..b8df4dea 100644 --- a/testutil/random_network/mempool_test.go +++ b/testutil/random_network/mempool_test.go @@ -8,15 +8,12 @@ import ( "testing" "github.com/ava-labs/simplex/common" + "github.com/ava-labs/simplex/simplex" "github.com/ava-labs/simplex/testutil" "github.com/stretchr/testify/require" ) -var emptyBlacklist = common.Blacklist{ - NodeCount: 4, - SuspectedNodes: common.SuspectedNodes{}, - Updates: []common.BlacklistUpdate{}, -} +var emptyBlacklist = common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds) func TestMempoolVerifiesTx(t *testing.T) { logger := testutil.MakeLogger(t, 1) diff --git a/testutil/util.go b/testutil/util.go index 0a99a88f..970e032c 100644 --- a/testutil/util.go +++ b/testutil/util.go @@ -21,17 +21,18 @@ func DefaultTestNodeEpochConfig(t *testing.T, nodeID common.NodeID, comm common. storage := NewInMemStorage() wal := NewTestWAL(t) conf := simplex.EpochConfig{ - MaxRoundWindow: simplex.DefaultMaxRoundWindow, - MaxProposalWait: simplex.DefaultMaxProposalWaitTime, - MaxRebroadcastWait: simplex.DefaultEmptyVoteRebroadcastTimeout, - Comm: comm, - Logger: l, - ID: nodeID, - Signer: &TestSigner{}, - WAL: wal, - Verifier: &TestVerifier{}, - Storage: storage, - BlockBuilder: bb, + MaxBlacklistedRounds: simplex.DefaultMaxBlacklistedRounds, + MaxRoundWindow: simplex.DefaultMaxRoundWindow, + MaxProposalWait: simplex.DefaultMaxProposalWaitTime, + MaxRebroadcastWait: simplex.DefaultEmptyVoteRebroadcastTimeout, + Comm: comm, + Logger: l, + ID: nodeID, + Signer: &TestSigner{}, + WAL: wal, + Verifier: &TestVerifier{}, + Storage: storage, + BlockBuilder: bb, SignatureAggregatorCreator: func(weights []common.Node) common.SignatureAggregator { return &TestSignatureAggregator{N: len(weights)} },