remove Deadlock from lib/grandpa/message_tracker.go

[kishansagathiya]

remove common mapLock from map tracker and have two separate locks for commit tracker and vote tracker

Changes

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Fixes #2841

Primary Reviewer

@EclesioMeloJunior

[codecov]

Codecov Report

Merging #2923 (a26ccf5) into development (ac2af46) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #2923      +/-   ##
===============================================
+ Coverage        63.29%   63.36%   +0.07%     
===============================================
  Files              218      218              
  Lines            27533    27542       +9     
===============================================
+ Hits             17426    17453      +27     
+ Misses            8495     8480      -15     
+ Partials          1612     1609       -3     

[qdm12]

When do you get such deadlock? A bit more context would be nice 😉

EDIT: never mind I'm an idiot, I just read the issue linked 👍

[EclesioMeloJunior]

There are no tests related to the commits in the tracker so I would like to suggest adding tests to it

[qdm12]

There are no tests related to the commits in the tracker so I would like to suggest adding tests to it

What do you mean? There is lib/grandpa/commits_tracker_test.go right?

There are no tests for the callers of the commits tracker, although I have a local WIP branch with these (waiting for a grandpa refactor and/or something like #2575)

[EclesioMeloJunior]

What do you mean? There is lib/grandpa/commits_tracker_test.go right?

There are no tests for the callers of the commits tracker, although I have a local WIP branch with these (waiting for a grandpa refactor and/or something like #2575)

The message tracker handles vote messages and commit messages, but in the message_tracker_test.go there is no tests related to adding a commit to the tracker and calling the (t *tracker) handleTick method, the test TestMessageTracker_handleTick only tests vote messages, so I would suggest to add one more case to tests the commit messages as well

[qdm12]

The message tracker handles vote messages and commit messages, but in the message_tracker_test.go there is no tests related to adding a commit to the tracker and calling the (t *tracker) handleTick method, the test TestMessageTracker_handleTick only tests vote messages, so I would suggest to add one more case to tests the commit messages as well

Yeah.... That's what I was trying to do a few months ago, but it's kinda of a pain in the ass to write good enough unit tests due to the impl details of the subtrackers (and the unexported fields of the linked lists)... so we would need interfaces... ideally internal subpackages for the 2 trackers and dep inject them to the parent tracker... but then we need #2575 ... anyway probably overthinking it lol...

We can for sure make an integration test without too much testing depth.

[EclesioMeloJunior]

LGTM!

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀