Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant code #3288

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

ghostant-1017
Copy link
Contributor

@ghostant-1017 ghostant-1017 commented Jun 4, 2024

@apruden2008
Copy link
Contributor

Is this connected to an issue? And if not, can we quantify the impact?

@ghostant-1017
Copy link
Contributor Author

Is this connected to an issue? And if not, can we quantify the impact?

It's not connected to any issues, it's just redundant code, doesn't make any effects.

@ghostant-1017 ghostant-1017 changed the title Remove useless code Remove redundant code Jun 13, 2024
@@ -822,10 +822,6 @@ impl<N: Network> Primary<N> {
}
// Retrieve the committee lookback for the round.
let committee_lookback = self_.ledger.get_committee_lookback_for_round(proposal.round())?;
// Retrieve the address of the validator.
let Some(signer) = self_.gateway.resolver().get_address(peer_ip) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this code is redundant. The previous check above:

        if self.gateway.resolver().get_address(peer_ip).map_or(true, |address| address != signer) {
            // Proceed to disconnect the validator.
            self.gateway.disconnect(peer_ip);
            bail!("Malicious peer - batch signature is from a different validator ({signer})");
        }

Returns true if there was no signer associated with a connected peer ip. Removing this check means that we would now allow validators to process the signatures for non-connected validators.

In theory, could alter the above check, but I believe the double-lookup is cheap enough to not warrant any changes.

@raychu86 raychu86 requested a review from ljedrz July 3, 2024 00:54
Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 points to consider:

  • the lookup at L787 already returns early for disconnected validators, though the message would indicate that the signer was different
  • that particular lookup is in fact quite cheap, but avoidable

My suggestion would be to adjust this change so that we:

  1. perform the address lookup and bind it to a variable or - if it fails - return early stating that the validator is disconnected
  2. check if the address is the signer and if that fails, return early stating that the signer is different

That way we would avoid the double lookup, and make the logs more clear.

There is one caveat: the validator technically could disconnect between L787 and L826, which would alter the current behavior with such a change (or with the one currently proposed in the PR); this may or may not be acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants