[yacovm]

This commit adds a maximum amount of rounds that a node can be blacklisted.
After the number of rounds passes, the node is automatically redeemed.

The blacklist stores the maximum number of rounds as a maximum orbit (uint16).
If a node has been blacklisted more than this maximum orbit ago, it is redeemed.

The maximum orbit / number of rounds can be changed via consensus by a local config change.

When a node proposes a new block, it is able to increase or decrease the maximum orbit by 1 according to the distance from its local config.

Nodes reject blocks that their maximum orbit changes for more than 1.

[samliok]

im not a fan of having some of these fields be public, it makes it hard for the caller to figure out which fields to set. Of course they can read the comments, but I think we can easily abstract the blacklist into a few constructor functions & make the call site much cleaner.

[samliok]

u might not like this, but I think there is a very worthwhile refactor here.

This mainly stems from my thinking of why is everything public in the blacklist.go file? It makes figuring out what the API of blacklist a bit complicated when writing testing and production code.

Examples

1. NodeCount is used once in production code. This can easily be private, and we can just create a constructor that takes nodeCount as a param.
2. MaxBlacklistedOrbits is set once in buildBlock, and then used only ever in epoch.freshBlacklist. When I'm initially looking at MaxBlacklistedOrbits its hard to know when I need to set it/reset it.
3. Updates and SuspectedNodes seems like implementation details of Blacklist, SuspectedNodes is not set in production code & Updates is set only once in production code. Again though, why should the epoch need to know how to set the blacklist? This part of the epoch code seems confusing

// 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.

Maybe the blacklist would benefit from a public Next function something like

// Next returns the blacklist of a block proposed in the given round on top of this blacklist.
// It accuses and redeems nodes according to the given rounds in which they timed out and were redeemed,
// and moves maxBlacklistedOrbits by a single orbit towards maxBlacklistedRounds.
func (bl *Blacklist) Next(round uint64, timedOut, redeemed map[uint16]uint64, maxBlacklistedRounds uint16) Blacklist {
	updates := bl.computeBlacklistUpdates(round, timedOut, redeemed)
	next := bl.applyUpdates(updates, round)
	next.maxBlacklistedOrbits = bl.nextMaxBlacklistedOrbits(maxBlacklistedOrbitsForRounds(bl.nodeCount, maxBlacklistedRounds))
	return next
}

This way the epoch doesn't need to call internal details of the blacklist, and building a block becomes

	e.Logger.Debug("Computing blacklist updates",
		zap.String("timedOutRounds", fmt.Sprintf("%v", e.timedOutRounds)),
		zap.String("redeemedRounds", fmt.Sprintf("%v", e.redeemedRounds)))
	nextBlacklist := prevBlacklist.Next(metadata.Round, e.timedOutRounds, e.redeemedRounds, e.MaxBlacklistedRounds)

instead of

	// 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)
	// 4) Move the max blacklisted orbits by a single orbit towards our locally configured value.
	targetOrbits := common.MaxBlacklistedOrbitsForRounds(uint16(len(e.validatorNodeIDs)), e.MaxBlacklistedRounds)
	nextBlacklist.MaxBlacklistedOrbits = prevBlacklist.NextMaxBlacklistedOrbits(targetOrbits)

[samliok]

Another benefit is the fact that our tests actually become simpler to understand. Since we don't have access to the internals, we actually need to set our blacklist naturally.

Take for example TestSimplexMultiNodeBlacklist

We have

expectedBlacklist := common.Blacklist{
	NodeCount:            4,
	MaxBlacklistedOrbits: simplex.DefaultMaxBlacklistedRounds / 4,
	SuspectedNodes:       common.SuspectedNodes{{NodeIndex: 3, SuspectingCount: 2, OrbitSuspected: 1}},
	Updates:              []common.BlacklistUpdate{{NodeIndex: 3, Type: common.BlacklistOpType_NodeSuspected}},
}

when i'm writing an epoch test, why do i need to know how to properly construct the blacklist? something like this is much more intuitive

// Rounds 4 and 5 accuse the fourth node of timing out in round 3.
timedOut := map[uint16]uint64{3: 3}
emptyBlacklist := common.NewBlacklist(4, simplex.DefaultMaxBlacklistedRounds)
accused := emptyBlacklist.Next(4, timedOut, nil, simplex.DefaultMaxBlacklistedRounds)
blacklisted := accused.Next(5, timedOut, nil, simplex.DefaultMaxBlacklistedRounds)

Another example is in TestEpochBlacklist, where we are manually setting the blacklists and then its updates field to nil.

These is kinda hard to understand at first glance, as the internals of the blacklist are necessary to understand. As opposed to changes on the right, you can just assume the blacklist works properly and pass the right params.

[yacovm]

ok I tend to agree, I'll see what I can do

[samliok]

another function that is weird to have exposed. This should just be internal to the blacklist

[samliok]

I don't see any benefit of having this be in terms of orbits, wouldn't it make sense to just have this value in terms of rounds? This way we can remove the rescaling logic associated with converting the orbits to rounds anyways.

[yacovm]

If we turn it into rounds, we will need to put the re-scaling logic here:

		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
		}

And it makes it more complex. Also - since the field is uint16, it allows us to pack more rounds into it when it's an orbit.

[samliok]

i made a branch with some of the refactoring changes I was talking above, I think it's worth discussing. It basically removes the need for the epoch to understand how to setup a blacklist and creates a more strict public API. Would love to hear thoughts about it

[samliok]

im still looking, leaving these for now

[samliok]

why did we add this config value? we don't seem to be using it anywhere

[yacovm]

we are using it: nodes[i] = network.addNodeWithConfig(vdr.NodeID[:], nodeConfig{maxBlacklistedRounds: initialRounds})

[samliok]

hmm, i dont see where this is

[yacovm]

ok I removed it: 7bfdeef

[samliok]

is this supposed to be the config value for the first ever MaxBlacklistRounds?

[yacovm]

correct

[samliok]

where do we verify the blacklist is set to GenesisMaxBlacklistedRounds for the first block?

[yacovm]

It's by construction. in verifyProposalMetadataAndBlacklist we first create prevBlacklist := common.NewBlacklist(uint16(len(e.validatorNodeIDs)), common.GenesisMaxBlacklistedRounds) and never take the branch of if bh.Seq > 0 {

[yacovm]

I think it can be +-1 of GenesisMaxBlacklistedRounds though

[samliok]

hmm, if we are building of a snowman chain what will the sequence be? Will simplex always start at seq 0, or will it just start at the height of the last snowman block?

[yacovm]

Good question.

So even if we take the branch where bh.Seq > 0 then:

		// If the previous block belongs to an earlier epoch, the blacklist should be empty.
		if prevBlock.BlockHeader().Epoch < e.Epoch || prevBlacklist.IsEmpty() {
			prevBlacklist = prevBlacklist.Stateless(uint16(len(e.validators)))
		}

and:

func (bl *Blacklist) Stateless(nodeCount uint16) Blacklist {
	if bl.maxBlacklistedOrbits == 0 {
		return NewBlacklist(nodeCount, GenesisMaxBlacklistedRounds)
	}

[yacovm]

So, since a pre-simplex block doesn't have a blacklist, it's empty, so we will initialize it to a default value.

i meant to just press comment not approve