Skip to content

fix: wait should timeout instead of checking at beginning of protocol#485

Merged
ChaoticTempest merged 2 commits intodevelopfrom
phuong/fix/protocol-timeout-wait
Aug 21, 2025
Merged

fix: wait should timeout instead of checking at beginning of protocol#485
ChaoticTempest merged 2 commits intodevelopfrom
phuong/fix/protocol-timeout-wait

Conversation

@ChaoticTempest
Copy link
Copy Markdown
Contributor

This is most likely what was causing our introduction issue. We should timeout not at the beginning of the protocol loop, but when trying to receive messages. This is because when awaiting for a message, it will stall till the next message, meaning we don't check the timeout until we get a new message if ever. This means that there will be a set of owned ongoing generators that will never wake up given that they receive no messages, which means our introduced "counter" never gets decremented since there will be live generators that never expire that contribute to this count.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a timeout handling issue in the chain-signatures protocol generators that was causing ongoing generators to never expire, leading to resource leaks. The fix moves timeout checking from the main protocol loop to the message receiving function, ensuring that generators properly timeout when waiting for messages.

  • Moved timeout logic from main loop to message receiving for proper timeout handling
  • Added dedicated recv() methods with timeout for triple, signature, and presignature generators
  • Removed a TODO comment about timeout handling for votes in posit.rs

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
chain-signatures/node/src/protocol/triple.rs Added timeout-aware recv() method and removed loop-based timeout check
chain-signatures/node/src/protocol/signature.rs Added timeout-aware recv() method and removed timeout() function
chain-signatures/node/src/protocol/presignature.rs Added timeout-aware recv() method and removed loop-based timeout check
chain-signatures/node/src/protocol/posit.rs Removed outdated TODO comment about timeout handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Copy Markdown
Contributor

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Make sense,
some day we will need to have some kind of abstraction on top of all the protocols. Lot's of things are repetitive.

@ChaoticTempest ChaoticTempest merged commit 5775a93 into develop Aug 21, 2025
3 checks passed
@ChaoticTempest ChaoticTempest deleted the phuong/fix/protocol-timeout-wait branch August 21, 2025 15:40
jakmeier added a commit to jakmeier/mpc that referenced this pull request Aug 28, 2025
Adds a test that would fail without the fix in sig-net#485
jakmeier added a commit to jakmeier/mpc that referenced this pull request Aug 28, 2025
jakmeier added a commit to jakmeier/mpc that referenced this pull request Aug 28, 2025
jakmeier added a commit that referenced this pull request Aug 29, 2025
* add regression test for presignature timeout

Adds a test that would fail without the fix in #485

* make clippy happy

* increase default triple timeout in test setup
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