Fix extractCommitteeMember

[fed-franz]

QUESTIONS on extractCommitteeMember
Originally posted by @fed-franz in #1491 (comment)

Original code:

	for i := 0; ; i++ {
		// If a provisioner is missing, we use the provisioner at position 0
		if m, e = p.MemberAt(i); e != nil {
			m, e = p.MemberAt(0)

			// If provisioner 0 is also missing, panic
			if e != nil {
				// FIXME: shall this panic?
				log.Panic(e)
			}

			i = 0
		}

		stake, err := p.GetStake(m.PublicKeyBLS)
		if err != nil {
			// If we get an error from GetStake, it means we either got a public key of a
			// provisioner who is no longer in the set, or we got a malformed public key.
			// We can't repair our committee on the fly, so we have to panic.
			log.Panic(fmt.Errorf("pk: %s err: %v", util.StringifyBytes(m.PublicKeyBLS), err))
		}

		// If the current stake is higher than the score, return the current provisioner's BLS key
		if stake >= score {
			return m.PublicKeyBLS
		}

		score -= stake
	}
}

This line:
if m, e = p.MemberAt(i); e != nil {
handles the case in which a Member is missing.
Q: When does/can this occur?

This line:
m, e = p.MemberAt(0)
handles a missing Member by setting the current member 'm' to Member 0.
Q: What's the logic behind this?

This line:
if err != nil {
seems similar to the case in which a member is missing.
Q: When does this occur? What's the difference between the two cases?

[fed-franz]

@herr-seppia @autholykos

We discussed this in Amsterdam.
If I recall correctly, the first check is actually not necessary.
Not sure about the last one.

Can you confirm that?

[herr-seppia]

The first check shall not be necessary.
Indeed I cannot see any reason why a provisioner should not be found.
It smells to me like a workaround for some tests when the rusk integration was not fully done.

So, back to your question

1. No reason why a provisioner should not be found
2. Then, no need to fallback to the first member
3. No way to panic if the provisioners are empty.

About the last point, maybe it worth to add a check into the stake contract in order to not perform unstake for the last provisioner

[fed-franz]

@herr-seppia
What do you think about this?:

stake, err := p.GetStake(m.PublicKeyBLS)
	if err != nil {
		// If we get an error from GetStake, it means we either got a public key of a
		// provisioner who is no longer in the set, or we got a malformed public key.
		// We can't repair our committee on the fly, so we have to panic.
		log.Panic(fmt.Errorf("pk: %s err: %v", util.StringifyBytes(m.PublicKeyBLS), err))
	}

Is this necessary?
I think the provisioner set should not change while extracting a member. Isn't that the case?

[fed-franz]

@herr-seppia

I think we misinterpreted the code.
The missing provisioner check is actually meant to loop over the provisioner set when the last index is reached.
Since the loop only increases i with no break condition, when i equals p.size() + 1, it sets the provisioner to provisioner 0 and the index i to 0.

So the first check is actually necessary for the loop to work.

The second check (if e != nil) I still believe is not necessary.

[herr-seppia]

I think we misinterpreted the code.

Yes, you're right.

In that case, I suggest to change the comments with

- // extractCommitteeMember walks through the provisioners set, while deducting each stake
+ // extractCommitteeMember loops the provisioners set, while deducting each stake

and

- // If a provisioner is missing, we use the provisioner at position 0
+ // If a provisioner is missing it means that we've reached the end of the set. 
+ // Then we use the provisioner at position 0 and continue to loop