Updated ETHDKG with new flow and contract bindings #66

ricardopinto · 2022-02-16T16:39:36Z

Scope

What is changing with this PR?

This PR introduces improvements to the ETHDKG flow enabling it to execute faster on good conditions, as well as leverage the new smart contracts' logic. It also introduces improvements in state management, retry logics, scheduling of tasks, and a great deal of testing. This new ETHDKG flow is event driven and allows for the process to complete quicker than before, with less phases to go through, and achieves greater code coverage.

Why?

Why are we doing this?

We're doing this to simplify the existing flow, speed up the process and integrate with the latest and greatest ETHDKG smart contracts.

Follow up stories

If any, what are the follow-up tasks required other than merging this PR? Have they been arranged?

…dators registrater

…aries

- Fixed ShouldRetry logic on tasks - ETHDKG can now be restarted - Improved RetrieveParticipants to be faster and more efficient

…pulling data from the chain - Improved scheduling management in regards to ShouldRetry logic that now relies on start and end blocks set when the task is created - Fixed bug where evicting a validator would cause access out of array boundaries due to poor state management - Cleanups

- Fixed base genesis file to support london fork - Improved code reusability on registration (NeedsRegistration)

- Fixed registration check

…ncluding a task, to avoid race conditions on ethereum forks

- Improved WaitConfirmations() method to use eth.GetFinalityDelay()

…ution

… to test it properly - Improved testing structure for better state management and keeping DRY tests - Tested bad distributed shares

…issing key shares and mpk submission

Cr0wn-Gh0ul · 2022-02-16T21:26:33Z

blockchain/dkg/utilities.go

+			done = true
+		}
+
+		time.Sleep(5 * time.Second)


will be addressed and fixed in the following ticket: https://madhive.atlassian.net/browse/MP-299

blockchain/dkg/utilities.go

Cr0wn-Gh0ul · 2022-02-16T21:57:59Z

blockchain/monitor/event_setup.go

@@ -13,43 +13,72 @@ import (
 func SetupEventMap(em *objects.EventMap, cdb *db.Database, adminHandler interfaces.AdminHandler, depositHandler interfaces.DepositHandler) error {


Do not hardcode the event hashes as this will result in silent failure when an event is updated.

parse from bindings

Cr0wn-Gh0ul · 2022-02-16T22:03:08Z

blockchain/monitor/monevents/validators.go

 	if err != nil {
 		return err
 	}

 	epoch := uint32(event.Epoch.Int64())

-	index := uint8(event.Index.Uint64()) - 1
+	index := uint32(event.Index.Uint64()) // - 1


clean up how this index is used below

Cr0wn-Gh0ul · 2022-02-16T22:03:58Z

blockchain/monitor/monevents/validators.go

-	if err != nil {
-		return err
-	}
+	// err = checkValidatorSet(state, epoch, logger, adminHandler)


remove, not used here

Cr0wn-Gh0ul · 2022-02-16T22:05:33Z

blockchain/monitor/monitor.go

+				logger.WithField("EventID", eventID).
+					WithField("info", info.Name).
+					Info("event received")
+			} /*  else {


Cr0wn-Gh0ul · 2022-02-16T22:10:44Z

blockchain/objects/dkg_state.go

+}
+
+// GetSortedParticipants returns the participant list sorted by Index field
+func (state *DkgState) GetSortedParticipants() ParticipantList {


need to make sure this has state.lock/unlock where used, see previous state locking comment.

same for functions below

https://madhive.atlassian.net/browse/MP-313

Cr0wn-Gh0ul · 2022-02-16T22:17:38Z

blockchain/objects/generic_marshaller.go

@@ -66,7 +67,7 @@ func (registry *TypeRegistry) WrapInstance(t interface{}) (*InstanceWrapper, err

 	name, present := registry.lookupName(tipe)
 	if !present {
-		return nil, ErrUnknownType
+		return nil, fmt.Errorf("unable to wrapInstance: %v", tipe)


evaluate if panic would be better here

Cr0wn-Gh0ul · 2022-02-16T22:20:09Z

blockchain/objects/scheduler_test.go

+	t.Log("error:", err)
+	assert.NotNil(t, err)
+
+	// _, err = s.Schedule(6, 14, task)


Cr0wn-Gh0ul · 2022-02-16T22:21:08Z

blockchain/objects/state.go


 // These are the valid phases of ETHDKG
 const (
-	Registration EthDKGPhase = iota
+	RegistrationOpen EthDKGPhase = iota


force default value is invalid

! investigate, if necessary preserve.

can we get through the bindings?

bindings don't give us this information yet ethereum/EIPs#47

Cr0wn-Gh0ul · 2022-02-16T22:24:02Z

blockchain/transaction_queue.go

@@ -146,6 +146,12 @@ func (b *Behind) collectReceipts() {
 					b.logger.Debugf("receipt not found: %v", txn.Hex())
 					return
 				}
+
+				//This only could happen using a SimulatedBackend during the tests


Cr0wn-Gh0ul · 2022-02-16T22:25:35Z

cmd/deploy/deploy.go

-	}
-	eu := &blockchain.Updater{Updater: ethdkgUpdater, TxnOpts: txnOpts, Logger: logger}
-	eth.Queue().QueueGroupTransaction(ctx, MIGRATION_GRP, eu.Add("migrate(uint256,uint32,uint32,uint256[4],address[],uint256[4][])", migrateEthDKGAddr))
+	// // Wire EthDKG migration contract


Cr0wn-Gh0ul · 2022-02-16T22:27:56Z

cmd/utils/utils.go

@@ -252,8 +264,13 @@ func register(logger *logrus.Entry, eth interfaces.Ethereum, cmd *cobra.Command,

 	// More ethereum setup
 	acct := eth.GetDefaultAccount()
+
+	if acct.Address.String() == "0x546F99F244b7B58B855330AE0E2BC1b30b41302F" {


no magic values / hardcoded addresses

Cr0wn-Gh0ul · 2022-02-16T22:28:39Z

cmd/utils/utils.go

-		logger.Errorf("StakingToken.Approve() failed")
+	var retry bool = true
+	for retry {
+		txn, err := c.StakingToken().Approve(txnOpts, c.ValidatorsAddress(), big.NewInt(1_000_000))


no magic values, resolve from contracts

this is going to be deleted soon because we'll be using the StakeNFT contract for this logic

Cr0wn-Gh0ul · 2022-02-16T22:29:06Z

cmd/utils/utils.go

-		logger.Errorf("Staking.LockStake() failed")
+	retry = true
+	for retry {
+		txn, err := c.Staking().LockStake(txnOpts, big.NewInt(1_000_000))


same as above comment

this is going to be deleted soon because we'll be using the StakeNFT contract for this logic

Cr0wn-Gh0ul · 2022-02-16T22:30:48Z

cmd/utils/utils.go

-	}
-	if rcpt != nil && rcpt.Status != 1 {
-		logger.Errorf("StakingToken.Approve() failed")
+	var retry bool = true


bound these for-loops

Cr0wn-Gh0ul · 2022-02-16T22:31:36Z

cmd/utils/utils.go

@@ -485,6 +516,82 @@ func deposittokens(logger *logrus.Entry, eth interfaces.Ethereum, cmd *cobra.Com
 	return 0
 }

+func testtx(logger *logrus.Entry, eth interfaces.Ethereum, cmd *cobra.Command, args []string) int {


this is scary

Cr0wn-Gh0ul · 2022-02-16T22:32:22Z

cmd/utils/utils.go

@@ -581,17 +688,10 @@ func ethdkg(logger *logrus.Entry, eth interfaces.Ethereum, cmd *cobra.Command, a

 	for _, log := range logs {
 		if log.Topics[0].Hex() == "0x9c6f8368fe7e77e8cb9438744581403bcb3f53298e517f04c1b8475487402e97" {


remove magic values

Cr0wn-Gh0ul · 2022-02-16T22:37:30Z

blockchain/dkg/dkgtasks/task_retry.go

-	// If we aren't registered correctly retry won't work -- Not true for registration
-	if status == NoRegistration || status == BadRegistration {
-		logger.Warnf("registration status: %v", status)
+	if currentBlock >= expectedLastBlock || currentBlock < expectedFirstBlock {


oddity around expectedFirstBlock

investigate

Cr0wn-Gh0ul

See and evaluate comments, make tickets as necessary. Fix only required comments for merge.

…passing)

sonarqubecloud · 2022-02-22T15:44:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

vtleonardo and others added 30 commits January 12, 2022 11:51

Make the binary compile with the new ETHDKG bindings

8fb2f43

Make the code compile

45a39f8

Updated event IDs and started refactor for the new ETHDKG

498f32e

Validators will now proceed to ShareDistribution phase after all vali…

1ef6722

…dators registrater

ETHDKG will now run the happy path to completion

5ff0f53

Fixed marshall bugs and scheduling to account for exclusive end bound…

8c164b3

…aries

- Cleaned ETHDKG state

d4e3f85

- Fixed ShouldRetry logic on tasks - ETHDKG can now be restarted - Improved RetrieveParticipants to be faster and more efficient

Validators can now dispute ETHDKG missing registration

11c8814

Stabler ethdkg with better should retry logic (still wip)

a3e4cfa

- Implemented 10% fee/tip increase on registration retry

7f2423e

- Fixed base genesis file to support london fork - Improved code reusability on registration (NeedsRegistration)

- Fixed nonce assignment when transactions fail.

634dccb

- Fixed registration check

Stabler version with replacement and "nonce too low" fix

eaee085

update doTask() and ShouldRetry() of each dkgtasks

06ff640

Fixed test setup files

497d940

testing register dkgtask

04028cd

Create OnboardingDocumentation

aede085

add tests for dispute_missing_registration_task

2dbd06c

Implemented mechanism to wait for a number of confirmations before co…

33e7d72

…ncluding a task, to avoid race conditions on ethereum forks

- Added eth.GetFinalityDelay() method to Ethereum interface

ac1b6a5

- Improved WaitConfirmations() method to use eth.GetFinalityDelay()

test share_distribution_task

efd704a

Added more tests to missing share distributions

ce16d3a

test ShouldRetry share_distribution and dispute_missing_share_distrib…

b6122eb

…ution

Added tests for bad distributed shares

94beda9

updating tests using processors

6c1fd3b

- Decoupled ethdkg state management from monitorState code to be able…

60bdbbd

… to test it properly - Improved testing structure for better state management and keeping DRY tests - Tested bad distributed shares

Tested key share submission

e758291

test dispute_share_distribution_task

4a5536a

update participant phase in processors and validators, test dispute m…

203ffc6

…issing key shares and mpk submission

test mkp_submission_task

caf235e

Cr0wn-Gh0ul reviewed Feb 16, 2022

View reviewed changes

blockchain/dkg/utilities.go Show resolved Hide resolved

Cr0wn-Gh0ul reviewed Feb 16, 2022

View reviewed changes

Cr0wn-Gh0ul suggested changes Feb 16, 2022

View reviewed changes

ricardopinto and others added 7 commits February 17, 2022 18:26

Fixed PR review notes

8e089a2

Fixed more PR review notes

97c6755

Fixing PR review notes

e382741

More fixes related to PR review notes

022bf1c

Fixed tests noted in the PR review (all tests inside blockchain/ are …

1cb2b12

…passing)

adding flag for transaction_queue test run

97361ec

Merge branch 'candidate' into new-ethdkg

3cb700c

Cr0wn-Gh0ul approved these changes Feb 22, 2022

View reviewed changes

Cr0wn-Gh0ul merged commit 32e7ce6 into MadBase:candidate Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated ETHDKG with new flow and contract bindings #66

Updated ETHDKG with new flow and contract bindings #66

ricardopinto commented Feb 16, 2022 •

edited

Loading

Cr0wn-Gh0ul Feb 16, 2022

kosegor Feb 17, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

ricardopinto Feb 17, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

ricardopinto Feb 17, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

ricardopinto Feb 17, 2022

Cr0wn-Gh0ul Feb 16, 2022

ricardopinto Feb 17, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul Feb 16, 2022

Cr0wn-Gh0ul left a comment

sonarqubecloud bot commented Feb 22, 2022

		@@ -13,43 +13,72 @@ import (
		func SetupEventMap(em objects.EventMap, cdb db.Database, adminHandler interfaces.AdminHandler, depositHandler interfaces.DepositHandler) error {

		@@ -581,17 +688,10 @@ func ethdkg(logger logrus.Entry, eth interfaces.Ethereum, cmd cobra.Command, a

		for _, log := range logs {
		if log.Topics[0].Hex() == "0x9c6f8368fe7e77e8cb9438744581403bcb3f53298e517f04c1b8475487402e97" {

Updated ETHDKG with new flow and contract bindings #66

Updated ETHDKG with new flow and contract bindings #66

Conversation

ricardopinto commented Feb 16, 2022 • edited Loading

Scope

Why?

Follow up stories

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cr0wn-Gh0ul left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 22, 2022

ricardopinto commented Feb 16, 2022 •

edited

Loading