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

Add polybft consensus support to edge #1051

Conversation

epikichi
Copy link
Contributor

@epikichi epikichi commented Dec 14, 2022

Description

  • Adding PolyBFT Consensus option to Docker compose
  • Adding --docker modes to the scripts/cluster script
  • Adding README to docker and scripts folder
  • Changed log-level to debug for all containers

How to use

  • scripts/cluster ibft --docker - run docker environment with ibft consensus
  • scripts/cluster polybft --docker run docker environment with polybft consensus
  • scripts/cluster (ibft or polybft) --docker destroy - destroy docker environment (volumes will also be deleted)
  • scripts/cluster (ibft or polybft) --docker stop - stop docker environment

@epikichi epikichi changed the base branch from develop to feature/v3-parity December 14, 2022 14:45
@codecov
Copy link

codecov bot commented Dec 15, 2022

Codecov Report

❗ No coverage uploaded for pull request base (feature/v3-parity@cafd3ac). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head e6e116d differs from pull request most recent head d048e64. Consider uploading reports for the commit d048e64 to get more accurate results

@@                 Coverage Diff                  @@
##             feature/v3-parity    #1051   +/-   ##
====================================================
  Coverage                     ?   54.50%           
====================================================
  Files                        ?      172           
  Lines                        ?    22871           
  Branches                     ?        0           
====================================================
  Hits                         ?    12466           
  Misses                       ?     9427           
  Partials                     ?      978           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ZeljkoBenovic ZeljkoBenovic marked this pull request as ready for review December 15, 2022 23:51
@ZeljkoBenovic ZeljkoBenovic self-assigned this Dec 15, 2022
@ZeljkoBenovic ZeljkoBenovic added the feature New update to Polygon Edge label Dec 15, 2022
Makefile Outdated
Comment on lines 65 to 71
EDGE_CONSENSUS=ibft docker-compose -f ./docker/local/docker-compose.yml stop

.PHONY: destroy-local
destroy-local:
EDGE_CONSENSUS=ibft docker-compose -f ./docker/local/docker-compose.yml down -v

.PHONY: run-local-polybft
run-local-polybft:
$(MAKE) compile-core-contracts
EDGE_CONSENSUS=polybft docker-compose -f ./docker/local/docker-compose.yml up -d --build

.PHONY: stop-local-polybft
stop-local-polybft:
EDGE_CONSENSUS=polybft docker-compose -f ./docker/local/docker-compose.yml stop

.PHONY: destroy-local-polybft
destroy-local-polybft:
EDGE_CONSENSUS=polybft docker-compose -f ./docker/local/docker-compose.yml down -v

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO Makefile is not the place to manage the lifecycle of local env, it's not building any aspect of the binary, this belongs to a docker script in scripts folder

Copy link
Contributor

Choose a reason for hiding this comment

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

This local env var determines which consensus mechanism is to be used. It acts as a kind of a switch statement.
The docker-compose file passes it to the Dockerfile as a second argument, which is used by the entrypoint script to create genesis for a specific consensus.
It must be set before the init container spins up, as it needs to know which consensus is being used.

The alternative to this approach is to have separate files for each consensus.
We would than need to have dedicated docker-compose, Dockerfile and entrypoint script for each consensus.

IMHO, this is much simpler approach, as we're using the same files and the users can easily make consensus selection just by setting EDGE_CONSENSUS env var.

But if you disagree with this, we can easily split this logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vcastellm updated readme file with the instruction on how to run with make or directly with docker-compose.
Kept the same logic, as we're using existing docker files, but just switching the consensus with EDGE_CONSENSUS env var. This keeps the scripts simple and lean.
Also added in some customization options for the chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal is to remove all the run-local stuff out from Makefile to a docker script in scripts, the fact that it's driven by env variables it's not related to using Makefile, it can be also improved by adding an option to the cluster script for running under docker, scripts/cluster polybft --docker should be the UI

Copy link
Contributor

Choose a reason for hiding this comment

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

@vcastellm I see now that we already have dedicated scripts folder. Edited the cluster script to accommodate ibft and polybft docker environments.
Let me know if this works for you and the rest of the team.

Will change the PR description, but there are README.md files in both docker and scripts folder with some explanation how to use the script.

docker/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Tnx for effort. Leaving some comments and doubts to consider.

consensus/polybft/polybft.go Outdated Show resolved Hide resolved
docker/local/polygon-edge.sh Show resolved Hide resolved
command/genesis/utils.go Outdated Show resolved Hide resolved
command/genesis/utils.go Outdated Show resolved Hide resolved
command/genesis/utils.go Outdated Show resolved Hide resolved
command/genesis/utils.go Outdated Show resolved Hide resolved
@ZeljkoBenovic ZeljkoBenovic force-pushed the epikichi/edge-988-add-polybft-consensus-support-to-edge branch from df4e5da to e6e116d Compare December 22, 2022 10:19
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

I don't want to block this for just the scripts thing, LGTM if we agree as a team that Makefile is the place

@vcastellm vcastellm dismissed their stale review December 23, 2022 16:05

I don't want to block this for just the scripts thing, LGTM if we agree as a team that Makefile is the place

…' of github.com:0xPolygon/polygon-edge into epikichi/edge-988-add-polybft-consensus-support-to-edge
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeljkoBenovic ZeljkoBenovic merged commit ad329a9 into feature/v3-parity Dec 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 26, 2022
@Stefan-Ethernal Stefan-Ethernal deleted the epikichi/edge-988-add-polybft-consensus-support-to-edge branch December 26, 2022 13:59
@epikichi epikichi changed the title Epikichi/edge 988 add polybft consensus support to edge Add polybft consensus support to edge Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New update to Polygon Edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants