server: Add suspending sessions that did not pass the pHash verification

[leszko]

What does this pull request do? Explain your changes. (required)
Suspend untrusted session when the following conditions are correct:

• The untrusted session returned incorrect p-hash
• Another untrusted session returned correct p-hash

It would be nice to add some automated tests for this change, however, it's hard to do it with the current design. I think we should tackle it separately while refactoring broadcast.go.

Specific updates (required)

• Suspend the untrusted session that returned incorrect p-hash

  How did you test each of these updates (required)

1. Create a fake orchestrator that returns incorrect p-hash
2. Run the setup with 1 trusted orchestrator, 2 correct untrusted orchestrators, and 1 incorrect untrusted orchestrator
3. Check that from time to time the incorrect untrusted orchestrator is suspended for one session selection

Does this pull request close any open issues?

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[darkdarkdragon]

LGTM

[jailuthra]

LGTM. Made an unrelated sidecomment

[jailuthra]

Not related to this exact issue or very high priority, so maybe we should open a new issue/PR for this:

Maybe we should reset our verifiedSession to nil if the hash it sends doesn't match?

Suggested change

	} else {
	} else {
	if (bsm.verifiedSession == untrustedResult.Session) {
	bsm.verifiedSession = nil
	}

[leszko]

I see one issue with adding this condition. If we add it, then a false-negative will cause bsm.verifiedSession = nil and in a result, we'll start looking for a new session. That can in turn cause bouncing between sessions if we encounter many false-negative scenarios.

We decided that we'll suspend sessions only if it failed p-hash verification but at the same time the other untrusted sessions passed the verification. We could do the same and maybe set verifiedSession to nil when we suspend the session, but technically it does not matter, because this condition takes into consideration both bsm.verifiedSession = nil and excluding suspended sessions.

[jailuthra]

Good point about false negatives causing swaps, and yeah we'd anyway suspend a verified session and look for a new one if the other untrusted session passes. LGTM feel free to merge 👍

[yondonfu]

Just a few nits, but looks good!

[leszko]

Thanks for the comments @yondonfu . I addressed them. PTAL.

[yondonfu]

LGTM! Let's just cleanup the last few commits either via squashing and/or a rebase + push prior to merging.

[leszko]

LGTM! Let's just cleanup the last few commits either via squashing and/or a rebase + push prior to merging.

Yeah, sure I'll clean it up