Fix: Ignore community descriptions from ourselves if we are the control node #5155
Conversation
Jenkins BuildsClick to see older builds (12)
|
I think this is the issue, not the lock. Why would we save the shard when a new member is added? This seems like an oversight |
I reread the description and realized I didn't quite put it right. |
Ah ok yeah that makes more sense. I doubt it's the root cause of the issue though.
Yeah sadly it's more probable. Have you asked Iuri if he can receive messages on the control node? |
MishkaRogachev
force-pushed
the
fix/community-membership-request-pending-stuck
branch
from
2c68cbf
to
49736df
Compare
MishkaRogachev
changed the title
protocol/communities/manager.go
Outdated
| community, err := m.GetByID(id) | ||
| if err != nil && err != ErrOrgNotFound { | ||
| return nil, err | ||
| } | ||
|
|
||
| // We should not process the description from ourselves if we are the control node | ||
| if community != nil && community.IsControlNode() && common.IsPubKeyEqual(community.ControlNode(), signer) { |
There was a problem hiding this comment.
@osmaczko i think a clock check is extra here
MishkaRogachev
force-pushed
the
fix/community-membership-request-pending-stuck
branch
2 times, most recently
from
16aa2ca
to
a9f454d
Compare
|
@osmaczko @mprakhov many tests fail because they check both the response on call and a signal from their change of community description. If it is confirmed that the fix helps, I can fix these tests, but I have a little doubt whether it will break the app. Are there places in the desktop code where instead of a response from the rpc call we expect a signal with a changed community description from ourselves? |
protocol/communities/manager.go
Outdated
| community, err := m.GetByID(id) | ||
| if err != nil && err != ErrOrgNotFound { | ||
| return nil, err | ||
| } | ||
|
|
||
| // We should not process the description from ourselves if we are the control node | ||
| if community != nil && community.IsControlNode() && common.IsPubKeyEqual(community.ControlNode(), signer) { |
There was a problem hiding this comment.
| if community != nil && community.IsControlNode() && common.IsPubKeyEqual(community.ControlNode(), signer) { | |
| if community != nil && community.IsControlNode() { |
Should be enough.
There was a problem hiding this comment.
Not sure if the token master or another administrator updates the description? I think in this case the description should be handled by the management node, so checking signer of received message make sense for me
There was a problem hiding this comment.
There can be only one control node (one device) at given time.
| @@ -3596,16 +3611,10 @@ func (m *Manager) GetCommunityShard(communityID types.HexBytes) (*shard.Shard, e | |||
| } | |||
|
|
|||
| func (m *Manager) SaveCommunityShard(communityID types.HexBytes, shard *shard.Shard, clock uint64) error { | |||
| m.communityLock.Lock(communityID) | |||
There was a problem hiding this comment.
Should it really be removed?
There was a problem hiding this comment.
it is possible not to remove locs here, but add a similar check to the HandleCommunityPublicShardInfo and HandleCommunityShardKey, not sure if it is cleaner
| return m.persistence.SaveCommunityShard(communityID, shard, clock) | ||
| } | ||
|
|
||
| func (m *Manager) DeleteCommunityShard(communityID types.HexBytes) error { | ||
| m.communityLock.Lock(communityID) |
There was a problem hiding this comment.
Should it really be removed?
I wouldn't be surprised. If that's the case, then it is wrong. Although, it wouldn't be an issue for control node, because we do signal changes done to our own community on publish: status-go/protocol/messenger_communities.go Lines 371 to 378 in c0e922f The problem may exist for token-masters and admins though. |
MishkaRogachev
force-pushed
the
fix/community-membership-request-pending-stuck
branch
from
a9f454d
to
7c5bcf8
Compare
| // We should not process the community description on the control node | ||
| if community != nil && community.IsControlNode() { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
stupid question: what if control node restore his account by a seed phrase? Won't we loose CommunityDescriptions which were not added to the backup?
There was a problem hiding this comment.
Good question! We won't merge this PR, we need to take a more comprehensive approach for the next milestone.
For status-im/status-desktop#14700
My assumption: controlNode receives and processes its own
protobuf.CommunityDescriptionwhich can lead to a hang of the receiver thread. It might explain why one community's prolonged reevaluation would cause another community to freeze up.