Feat/signers resend timed out messages #4737
Conversation
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
… amount of time Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
| @@ -570,6 +597,7 @@ mod tests { | |||
| 3000, | |||
| None, | |||
| tx_fee_ustx, | |||
| None, | |||
There was a problem hiding this comment.
Nit: can you add a test here to ensure that the duration config field is set correctly when deserializing from a string?
| "{self}: Saving outbound {:?} message(s) to SignerDB.", | ||
| outbound_messages.len() | ||
| ); | ||
| if let Err(e) = self.signer_db.insert_outbound_messages( |
There was a problem hiding this comment.
If I'm understanding this correctly, couldn't this overwrite existing messages that we would've wanted to re-send after a timeout?
For example:
- Signer sends outbound A at X
- Signer sends outbound B at Y
- At
Y + timeout, state hasn't changed since A, but we only send messages B
If I'm understanding that correctly, then maybe a better implementation would be to insert these messages with the existing outbound messages, but only if the signer's state hasn't changed
| @@ -409,6 +409,39 @@ impl Signer { | |||
| self.coordinator_selector.last_message_time = Some(Instant::now()); | |||
| } | |||
|
|
|||
| /// Resend outbound messages if no state change has occurred within the configured timeout | |||
| pub fn resend_outbound_messages(&mut self, response_wait_timeout: Duration) { | |||
| // We do not need to resend outbound messages if we are in the Idle state. | |||
There was a problem hiding this comment.
Would it be the case then that self.signer_db.outbound_messages_lookup(self.reward_cycle) would return None if the state were Idle?
| @@ -1605,6 +1656,37 @@ impl Signer { | |||
| } | |||
| } | |||
|
|
|||
| /// Convert the signer state to a string | |||
| pub fn signer_state_to_string(state: &SignerState) -> String { | |||
There was a problem hiding this comment.
Nit: can these methods here instead be part of a Display impl for SignerState and CoordinatorState?
| /// The messages sent to listening parties | ||
| pub outbound_messages: Vec<Packet>, | ||
| /// the WSTS Coordinator state at the time of sending the outbound messages | ||
| pub coordinator_state: String, |
There was a problem hiding this comment.
Does this have to be a String? Why can't it be CoordinatorState? Same below for signer_state
| const CREATE_OUTBOUND_MESSAGES_TABLE: &str = " | ||
| CREATE TABLE IF NOT EXISTS outbound_messages ( | ||
| insertion_time TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| reward_cycle INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
I'm not sure how this is going to work if you have multiple messages with the same reward cycle? You'll get a UNIQUE constraint failed error from rusqlite if you try to insert multiple messages for the same reward cycle. Is it the case that you'll only ever insert at most one message per reward cycle?
There was a problem hiding this comment.
In this table, outbound_messages is a slice of strings all concatenated into one blob. So outbound_messages is something like msg_a,msg_b,msg_c.
Closes #4502