Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New accounts cannot be added even if there is a quorum of trustee approvals #355

Closed
uabjabborov opened this issue May 11, 2022 · 2 comments
Assignees
Labels
bug Something isn't working size: S
Projects

Comments

@uabjabborov
Copy link
Collaborator

Problem:
We have a situation in Testnet 2.0 when an addition of a NodeAdmin account was stuck even if a quorum of trustee approvals has been received (7 approvals from 10 trustees).

Possible reason:
This issue seems to be related to the revocation of trustee accounts while there are pending accounts to be approved.

Example:

  1. There are 10 trustees in the network
  2. Account "A" is proposed to be added (requires at least 7 approvals. 2/3 of trustees)
  3. Account "A" receives 6 approvals (one more approval is required)
  4. Meanwhile 1 trustee account has been revoked and there are 9 trustees in the network
  5. Account "A" is still in pending approvals, even if it has 6 approvals which are enough after the revocation of a trustee
  6. Account "A" receives one more approval and now it has 7 approvals. The account has not been added even if it has more than the required number of approvals (6 required)
  7. The DCL logic checks whether an account has enough approvals in the following way:
    // check if pending account has enough approvals
    if len(pendAcc.Approvals) == k.AccountApprovalsCount(ctx)
    
    which is a problem because it checks whether the number of approvals is exactly equal to the required number.
    k.AccountApprovalsCount(ctx) calculates the required number of approvals based on the number of trustees at the moment of execution. In our particular case k.AccountApprovalsCount(ctx) returns 6, but pendAcc.Approvals is already 7. So this account will never be added unless the number of trustees increases.

Possible solutions:

  1. Change the logic for checking whether an account has required approvals:
    // check if pending account has enough approvals
    if len(pendAcc.Approvals) >= k.AccountApprovalsCount(ctx)
    
    so that whenever the number of approvals exceeds the required number an account is added.
  2. Whenever a trustee account is revoked, all pending accounts should be checked whether they have enough approvals. If so, accounts are added automatically.
@uabjabborov uabjabborov added bug Something isn't working size: M labels May 11, 2022
@uabjabborov uabjabborov added this to Backlog in DCL via automation May 11, 2022
@uabjabborov uabjabborov moved this from Backlog to To do in DCL May 11, 2022
@ashcherbakov ashcherbakov added this to the v1.0: Main Net Launch milestone May 16, 2022
@ashcherbakov
Copy link
Contributor

ashcherbakov commented May 16, 2022

I think both items need to be implemented, but Item 2 doesn't look like a must have for 1.0, and can be considered as an additional feature.
Let's do Item 1 only (change == to >=) in the scope of this task, and create a separate task for Item 2, see #358.

@abdulla-ashurov abdulla-ashurov moved this from To do to In progress in DCL May 19, 2022
@ashcherbakov ashcherbakov moved this from In progress to In Review in DCL May 20, 2022
@abdulla-ashurov
Copy link
Collaborator

Plan of attack:

  • New accounts cannot be added even if there is a quorum of trustee approvals, which mention above
  • Add minimum integration tests for checking a new implementation

uabjabborov pushed a commit that referenced this issue May 23, 2022
Issue355 -  New accounts cannot be added even if there is a quorum of trustee approvals #355
DCL automation moved this from In Review to Done May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size: S
Projects
No open projects
DCL
  
Done
Development

No branches or pull requests

3 participants