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

Majority without gaps #1544

Closed
wants to merge 4 commits into from
Closed

Majority without gaps #1544

wants to merge 4 commits into from

Conversation

tbekas
Copy link
Contributor

@tbekas tbekas commented May 17, 2021

Resolves #1497

  • Majority should not have gaps
  • Majority should always overlap with the previous majority
  • Missing proposals are getting looked up only if the majority stalls for 4 consecutive rounds or more
  • Lookup range of missing proposals changed to [+2, +8] (relative to latest majority height)
  • Replacing MajorityInfo with CuckooFilter
  • Adding endpoint POST /snapshot/proposal/_query


def persistPeerProposal(peer: Id, proposal: Signed[SnapshotProposal]): F[Unit] =
def removePeerProposalsBelowHeight(height: Long): F[Unit] =
Copy link
Contributor

@TheMMaciek TheMMaciek May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should just remove proposals based on our calculated majority. In case someone else gets stuck because of a missing proposal, we may not have that proposal anymore at the time when the node will be asking for it. I would take into account the filter and check if all other nodes have that proposal present/majority picked at certain height and only then remove it.

Copy link
Contributor Author

@tbekas tbekas May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We store at least 40 heights of proposals, a node that is 30 heights below or more would start redownloading itself already before we would remove any proposals he may need. These parameters could change, but implementing this would introduce just more complexity and with the current config, that complexity would never be used. Also, another case is what if someone doesn't have proposals at all, should we keep all of them indefinitely? If so memory leak, so another case to handle.

Copy link
Contributor

@TheMMaciek TheMMaciek May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point. If you think it's too much hassle and no gain then there is no point in implementing that indeed. Especially if the logic right now is sealed e.i. all the cases are handled and we won't have nodes continuing ahead indefinitely - in consequence the redownloads will work as intended.

@tbekas tbekas force-pushed the majority-without-gaps branch 3 times, most recently from c2beb36 to e6de9a3 Compare May 24, 2021 15:55
TheMMaciek
TheMMaciek previously approved these changes May 26, 2021
marcinwadon
marcinwadon previously approved these changes May 26, 2021
@tbekas
Copy link
Contributor Author

tbekas commented Jun 28, 2021

Already merged to dev

@tbekas tbekas closed this Jun 28, 2021
@tbekas tbekas deleted the majority-without-gaps branch June 28, 2021 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Majority state chooser should stop at a gap
4 participants