Notify batch notifier for actual sequence number because it could be blocking it #5868
Conversation
crates/sui-core/src/authority.rs
Outdated
| if seq < ticket_seq { | ||
| debug!("Notifying during retry, current low watermark {:?}, ticket_seq {:?}, seq {:?}", self.batch_notifier.low_watermark(), ticket_seq, seq); | ||
| self.batch_notifier.notify(seq); |
There was a problem hiding this comment.
the notifier_ticket.notify(); in fn commit_certificate does not work, is it because it only notifies the ticket_seq but not the old one (seq) ?
Is it better to move the notify-old-seq logic to the end of fn commit_certificate where notifier_ticket.notify(); happens?
There was a problem hiding this comment.
@longbowlu lmk if you strongly feel about moving this check in commit_certificate but i like the fact that all tricky batch notifier checks are in one place here
crates/sui-core/src/authority.rs
Outdated
| @@ -851,10 +851,19 @@ impl AuthorityState { | |||
| } else { | |||
| error!(?digest, "commit_certificate failed: {}", err); | |||
There was a problem hiding this comment.
can we log seq here
error!(?digest, seq?=ticket_seq, "commit_certificate failed: {}", err);
same thing for the above debug!
crates/sui-core/src/authority.rs
Outdated
| debug!( | ||
| "Failed to notify ticket with sequence number: {}", | ||
| ticket_seq | ||
| ); |
There was a problem hiding this comment.
| debug!( | |
| "Failed to notify ticket with sequence number: {}", | |
| ticket_seq | |
| ); | |
| debug!( | |
| seq=?ticket_seq, | |
| "Ticket not notified due to commit failure", | |
| ); |
There was a problem hiding this comment.
The probability is probably low, but you could have the following scenario:
- The first run failed, so we have a ticket that's not released
- The second run failed again, and we have another ticket that's not released.
- The third time succeeded, but it will only unlock the first one, not the second.
sadhansood
force-pushed
the
sadhan/fix_notifier
branch
2 times, most recently
from
3036428 to
1d8f870
Compare
Good point! I think I handled your comments @lxfind, lmk what you think |
crates/sui-core/src/authority.rs
Outdated
| "Ticket not notified due to commit failure", | ||
| ); | ||
| // Check if we were able to sequence the tx at all | ||
| if let Some(tx_seq) = self.db().get_tx_sequence(*certificate.digest()).await? { |
There was a problem hiding this comment.
I am probably being picky, but this line could return Err as well..
There was a problem hiding this comment.
I am ok with this going in though, as the chance should be really really really low. We need to start somewhere and this is a big improvement already
There was a problem hiding this comment.
Yeah I agree, i thought of it too and wanted to add a few retries there to handle db read failing. Let me do it in a follow up PR, and we can let this go for now
sadhansood
force-pushed
the
sadhan/fix_notifier
branch
from
1d8f870 to
28887bc
Compare
Authority notifier gets stuck if the previous attempt of commiting a certificate sequenced the transaction but failed to commit. In this case, we should notify both previous and old sequence.
Also, if we failed to lock a sequence number and failed to commit, we should notify the ticket as well