Skip to content

fix: Adjust invariant checks when selecting participants#680

Merged
netrome merged 7 commits intonear:mainfrom
hot-dao:kuksag/random-participants-choice-fix
Jul 23, 2025
Merged

fix: Adjust invariant checks when selecting participants#680
netrome merged 7 commits intonear:mainfrom
hot-dao:kuksag/random-participants-choice-fix

Conversation

@kuksag
Copy link
Copy Markdown
Collaborator

@kuksag kuksag commented Jul 20, 2025

Context

The function:

pub fn select_random_active_participants_including_me(
        &self,
        total: usize,
        peers_to_consider: &[ParticipantId],
    ) -> anyhow::Result<Vec<ParticipantId>>

Should return total number of online participants amongst peers_to_consider, including the caller participant.

This function has two usages:

  1. In generation of triples
  2. In generation eddsa signatures

Bug

This function behaves incorrectly, when the size of peers_to_consider is insufficient, to make up for total participants.
Extrapolated example: if we pass empty peers_to_consider. In that case [me] will be returned, because me is getting appended regardless. The error should be returned instead.

IteratorRandom::choose_multiple(_, amount) silently returns less than amount elements, if there are not enough.

Solution

Make invariant check on return value

Effect

This has no practical effect at the moment, because all_participants_from_config is being passed into select_random_active_participants_including_me

@kuksag kuksag requested a review from a user July 20, 2025 21:57
@kuksag kuksag changed the title fix: Adjust invariant checks when selecting participant fix: Adjust invariant checks when selecting participants Jul 20, 2025
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

That's a very good catch. Thanks for fixing it.

Could you also add a unit test, so we have some regression test for this bug? It's not a hard requirement from my side if it's tedious to test.

@kuksag kuksag force-pushed the kuksag/random-participants-choice-fix branch from 4ea77bc to 648c171 Compare July 21, 2025 11:09
Comment thread node/src/network.rs Outdated
Copy link
Copy Markdown
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Some suggestions, otherwise lgtm.

Comment thread node/src/network.rs Outdated
Comment thread node/src/network.rs Outdated
@kuksag kuksag enabled auto-merge July 21, 2025 14:31
@kuksag kuksag requested a review from a user July 21, 2025 14:32
@kuksag kuksag added this pull request to the merge queue Jul 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 22, 2025
@netrome
Copy link
Copy Markdown
Collaborator

netrome commented Jul 23, 2025

I assume this failed due to main being broken for a brief time. Taking the liberty to add it to the merge queue again.

@netrome netrome added this pull request to the merge queue Jul 23, 2025
Merged via the queue into near:main with commit 191dba3 Jul 23, 2025
4 checks passed
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.

2 participants