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

add sleep after new block to the request_decider #1246

Merged
merged 10 commits into from
Jan 27, 2025

Conversation

Jiloc
Copy link
Collaborator

@Jiloc Jiloc commented Jan 20, 2025

Description

Part of : #1237

Changes

  • originally part of fix: missing signers' votes #1243 , this change add a new requests_processing_delay to the request_decider. This will give additional time to the bitcoin nodes to get in sync before receiving the vote decisions from the signers.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@Jiloc Jiloc self-assigned this Jan 20, 2025
@Jiloc Jiloc requested review from djordon, cylewitruk and matteojug and removed request for djordon and cylewitruk January 20, 2025 17:49
Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good.

Let's wait to get more eyes on this before merging, in case there is some edge case that we're blanking on that would make this a bad idea.

@djordon djordon added this to the sBTC: Key rotation milestone Jan 21, 2025
Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

Thinking out loud: assuming everyone gets the bitcoin block at exactly the same time, with the sleep as is here we may risk having the coordinator starting the round before the decisions manage to get to it (like, everyone broadcast its own decision, then the coordinator starts but it only has its own decision).
I think this is slightly worse than before, but it could be equivalent.

@Jiloc
Copy link
Collaborator Author

Jiloc commented Jan 22, 2025

How about we separate the two timeouts: one in the RequestDecider after a new block, and another in the TxCoordinator after receiving the signal from the RequestDecider? We could set them both to 15 seconds.

While this isn't an ideal or final solution, I believe it's a reasonable compromise. It should still allow us to receive the decisions and blocks in time most of the time.

#1243 will remain the final solution. And if we implement a filter to only resend messages that didn't receive all replies, we could significantly reduce the number of repeated messages.

@Jiloc Jiloc changed the title move sleep after new block to the request_decider add sleep after new block to the request_decider Jan 22, 2025
@Jiloc Jiloc requested a review from djordon January 22, 2025 17:00
signer/src/config/error.rs Outdated Show resolved Hide resolved
signer/src/config/mod.rs Outdated Show resolved Hide resolved
signer/src/config/mod.rs Outdated Show resolved Hide resolved
signer/src/config/mod.rs Show resolved Hide resolved
Jiloc and others added 3 commits January 23, 2025 11:55
@Jiloc Jiloc added the breaking-local Breaking changes for the local signer (i.e. config, db) label Jan 23, 2025
@Jiloc Jiloc merged commit e9129f8 into main Jan 27, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-local Breaking changes for the local signer (i.e. config, db)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants