Patch concurrent update error #458
Conversation
| @@ -63,6 +63,17 @@ impl FinalityWithNull { | |||
| /// Clear the cache and set the committed finality to the provided value | |||
| pub fn reset(&self, finality: IPCParentFinality) -> Stm<()> { | |||
| self.cached_data.write(SequentialKeyCache::sequential())?; | |||
|
|
|||
| if let Some(p) = self.last_committed_finality.read_clone()? { | |||
There was a problem hiding this comment.
I think .read()? should suffice here.
| @@ -71,6 +82,17 @@ impl FinalityWithNull { | |||
| height: BlockHeight, | |||
| maybe_payload: Option<ParentViewPayload>, | |||
| ) -> StmResult<(), Error> { | |||
| if let Some(p) = self.last_committed_finality.read_clone()? { | |||
There was a problem hiding this comment.
| if let Some(p) = self.last_committed_finality.read_clone()? { | |
| if let Some(p) = self.last_committed_finality.read()? { |
Here too, the value isn't returned, so you can access it through the Arc
|
I would like to add that this would be a good candidate for state machine testing (fuzzing) or even just quickcheck. You can generate an idealised block tree and then simulate multiple cursors on it, one representing your own parent, perhaps doing even reorgs, and the other representing quorums coming from your peers, either in or outside the range that you have in the cache. If you can drive the sync outside the timers, then you can have events such as:
And then it would bring out these random errors that you seem to be discovering live, such as the quorum cursor being further ahead than the parent cursor. You could assert whatever you like based on your overall view of the world, such as you don't propose a block which is earlier than the last finalized, you don't have entries in your cache which are older, etc. |
|
@aakoshh The state machine testing is pretty cool, I will give it a try. |
This fixes a concurrent update error. It is possible that a validator is catching up and received a finality from peer that is higher than the value stored in cache during polling. In this case, there is no need to update cache. Adding a test case for it.