Register in Random Beacon's Sortition Pool #3028

nkuba · 2022-06-17T12:01:05Z

Closes: #3006
Depends on: #3016

In this PR we register an operator in the Random Beacon's Sortition Pool.

We introduced a sortition package that defines a function for registration in the sortition pool. The package defines an interface for sortition pool-related functions that should be implemented by the contract handle (in this case RandomBeacon, but later we will have to do the same for WalletRegistry).

We export Ethereum Chain handle to use in other packages as we are aiming for packages structure refactoring and we expect to use the common chain handle in multiple modules. We also renamed the struct to `Chain` as it's defined in `ethereum` pacakge and will be used externally as `ethereum.Chain`, which is better than `ethereum.EthereumChain`.

We need getters for chain handle properites to be able to use them in other packages raleted to chain integration.

Implemented chain handle for Random Beacon's Sortition Pool contract.

The interface should be used by the beacon and ecdsa handles to run sortition pool related functions.

Implement functions for sortition pool in the random beacon's handle.

If we need to fetch the config for V2 we need to implement this function again.

We need to update the code for V2

The code is based on keep-ecdsa register.go code, with some improvements. We want the operator to join the sortition pool if it's eligible and the operator is registered by a staking provider.

Start sortition pool registration once a client starts.

This is a common package that will be used by beacon and ecdsa modules.

When pool is locked operators cannot join it.

pdyraga · 2022-07-04T12:52:53Z

cmd/start.go

+	// 			"is correct and it has KEEP tokens delegated and the operator " +
+	// 			"contract has been authorized to operate on the stake",
+	// 	)
+	// }


I think we should remove it. It is hard to say what we should wait for here - stake, authorization, or maybe staking provider<>operator mapping?

pdyraga · 2022-07-04T12:56:33Z

cmd/start.go

 		chainProvider,
 		netProvider,
 		persistence,
+		beaconChainHandle,


Instead of passing it as a separate parameter, I would replace ThresholdRelay() in chain.Handle with this interface. In case it is too complicated to replace ThresholdRelay() now, I would introduce it next to ThresholdRelay() and deprecated ThresholdRelay() use.

pdyraga · 2022-07-04T13:07:03Z

pkg/chain/ethereum/_LEGACY_V1

@@ -1 +1 @@
-This directory contains legacy code from V1. It should be removed.
+This directory contains legacy code from V1. It should be moved to fit the new directories structure.


I would clear up this directory and remove everything that is not needed for v2, assuming there is a way to make the compilation pass.

pdyraga · 2022-07-04T13:09:33Z

pkg/chain/ethereum/connect.go

-	ec := &ethereumChain{
-		config:           config,
+	c := &Chain{


Two questions:

Why not keep it as ethereumChain and have ethereumBeaconChain and ethereumEcdsaChain? I think it's easier to navigate through implementation files when it's implicitly denoted this is Ethereum implementation.

Why do we export it?

It was exported as it's used under random beacon's ethereum handle:
https://github.com/keep-network/keep-core/blob/fcc72bc88d90c714564caec45d60fe0e2b43b984/pkg/chain/random-beacon/ethereum.go#L23-L24

I'm open to other approaches.

This change was made in the initial implementation approach I've taken for this PR to reuse common Ethereum chain implementation in the random beacon and ecdsa packages, along with keeping it functional for the legacy code.

@nkuba

This change is greatly inspired by the work of @nkuba in #3028 but it is just a subset of the original work with some changes introduced. First, `sortition.Chain` handle interface is introduced. In opposite to v1 approach, where we had a global chain handle, the new proposed approach is to keep a chain handle per module. This should make the chain code less complicated and allow introducing custom behavior to functions, if needed be the logic of the given module. Second, `chain.Address` type based on `string` is introduced. We need a chain-agnostic implementation of a chain address to use in interfaces. So far, it's been an array of bytes, as presented in `Signing` interface, but `string` seems easier to use in most cases. Last but not least, a local implementation of `sortition.Chain` handle is introduced. Also, a set of simple test utils for assertions is added. Most of the time, such simple assertions are all we need, and having them extracted to a single place lets us avoid the assertion error message formatting boilerplate in test files. Long-term, if needed, we may use some more sophisticated assertion library.

lukasz-zimnoch · 2022-07-05T10:24:41Z

I'm a bit lost with the chain package structure we aim for. Especially, I don't understand the point of declaring upfront interfaces in the same package as the implementation of that interface. That seems to match more the Java-style than the Go-style interfaces. There are also some issues with that approach, for example:

pkg/chain/random-beacon/chain.go: There is a Handle interface declared in the beacon package. The client code will use it as beacon.Handle. That suggests this is a handle to the beacon instance, not the chain subset used by the random beacon. Looks like there can be more naming-related problems like that once we add more functions and interfaces.
Such chain interfaces with a default Ethereum implementation living nearby will be likely polluted with chain-specific types soon. Also, if we wanted to add another chain, we would be forced to add the new chain implementation in the same package. That will make the package messy
The implementation of each chain will be scattered through the module-specific packages. For example, if we want to implement Ethereum, we need to add ethereum.go in pkg/chain/random-beacon, pkg/chain/ecdsa and so on. This doesn't look like a bad idea at the moment but, at the same time, we need to create an additional package for common features and import them from there. That makes the package structure more complicated. There is also a risk that we will need to do more juggling in order to avoid code duplication in the future.
Module packages containing the domain logic like pkg/beacon and future pkg/ecdsa will not control their chain interfaces. That makes them dependent on external packages that define those chain interfaces. Also, those external packages contain a default chain implementation that can strongly affect the interface. The direction of dependency seems to be wrong. The intuition says that the domain logic should declare its own chain interface and the external implementation should depend on it.

That said, I'm wondering whether we shouldn't go with something like:

pkg/beacon declares its own chain.go where it defines the chain interface needed by that package. Actually, it already has the pkg/beacon/relay/chain/chain.go file with the relay chain interface. We can flatten that a little bit and remove the relay sub-package.
pkg/ecdsa will declare its own chain.go as well.
We will have a single pkg/chain/ethereum package containing the entire Ethereum chain implementation split into several module-specific files like: beacon.go, ecdsa.go and containing the common features like stake_monitor.go, signing.go and so on. This is also the place where contract bindings will live.
We will remove pkg/chain/chain.go. It looks like this is an artificial grouping of the BlockCounter, StakeMonitor and Signing interfaces. We can push those interfaces to separate files in pkg/chain package or move them to pkg/beacon and pkg/ecdsa altogether.

What are your thoughts guys?

pdyraga · 2022-07-05T11:06:04Z

I think your proposal makes sense but I also see some advantages of keeping chain implementations per module. I think we need to do some more experimentation and then discuss one more time before we decide to go this way or another.

A couple of quick thoughts:

Yes, keeping ethereum.go next to chain.go inside a module feels weird at first. On the other hand, if we treat the module like a chain-agnostic building block providing some functionality, it could make sense. Say, we have sortition module with sortition.go, chain.go, ethereum.go and algorand.go. The code outside of sortition.go does not care about most of sortition's chain.go functions because they are needed only in this context. Also, if there is something sortition-specific that must be included in ethereum.go implementation of one function, so that ethereum.go is not just a thin layer or read-writes, having it inside the interested module makes sense.

In my opinion, the problem with ethereum/ethereum.go is that it's a bag of functions without a context of how they are used and sometimes the context is needed to understand some specific logic inside of ethereum.go. For instance, why some call is retried and some other is not?

On the other hand, I think that instantiating one handle and then passing it everywhere is easier than instantiating multiple per-module handles that can even depend on each other. Also, using a set of common functions should be easier with a single handle, as you say.

Regardless of the decision here, I think keeping local.go per module makes sense. Usually, there is some logic in local.go needed by the tests of the given module that does not make sense without the context. For example, why do we return an error from JoinSortitionPool call if eligible stake is 0 or if the operator <> staking provider mapping is not set. This is very specific to sortition.

pdyraga · 2022-07-08T11:26:48Z

Closing in favor of #3056, #3067, #3070, #3077

nkuba added the 📟 client label Jun 17, 2022

nkuba added this to the v2.0.0-m1 milestone Jun 17, 2022

nkuba linked an issue Jun 17, 2022 that may be closed by this pull request

Register in Random Beacon sortition pool #3006

Closed

nkuba force-pushed the sortition-pool-register branch from 0182690 to 688277b Compare June 23, 2022 10:35

nkuba requested review from dimpar, lukasz-zimnoch and pdyraga and removed request for dimpar, lukasz-zimnoch and pdyraga June 23, 2022 12:31

nkuba force-pushed the sortition-pool-register branch from fb2ae37 to 05c030f Compare June 27, 2022 20:32

nkuba added 11 commits June 29, 2022 08:51

Expose properties of ethereum chain handle

d982bc9

We need getters for chain handle properites to be able to use them in other packages raleted to chain integration.

Update legacy comment

04f8647

Chain handle for Random Beacon's Sortition Pool

dd64f26

Implemented chain handle for Random Beacon's Sortition Pool contract.

Fix block counter assignment

69fcec0

Define chain handle interface for sortition pools

5663c57

The interface should be used by the beacon and ecdsa handles to run sortition pool related functions.

Implement sortition pool's functions for beacon

5942634

Implement functions for sortition pool in the random beacon's handle.

Disable legacy V1 chain config code

499eeb3

If we need to fetch the config for V2 we need to implement this function again.

Disable legacy stake monitoring

ac63879

We need to update the code for V2

Register operator in the sortition pool

ca98c08

The code is based on keep-ecdsa register.go code, with some improvements. We want the operator to join the sortition pool if it's eligible and the operator is registered by a staking provider.

Register sortition pool registration for client start

cf474f0

Start sortition pool registration once a client starts.

nkuba force-pushed the sortition-pool-register branch from 05c030f to cf474f0 Compare June 29, 2022 06:51

Move sortition code to pkg/sortition

d31e60a

This is a common package that will be used by beacon and ecdsa modules.

nkuba force-pushed the sortition-pool-register branch from 902ab51 to d31e60a Compare June 29, 2022 07:26

Fix typo

cdb3d50

nkuba added 14 commits June 29, 2022 10:18

Add todo to check if sortition pool is unlocked

11a11da

Unit test waitUntilRegistered function

a54b292

Reorder functions

1c3fed3

Register in a separate goroutine

7b9f648

Add unit tests for joinSortitionPoolWhenEligible

4b35103

Add unit tests for waitUntilJoined

6ff3779

Rename chain handle argument

a2850e3

Check if pool is locked when joining

242a8f1

When pool is locked operators cannot join it.

Improve log messages to include retry delay

476b87e

Include locked pool case in test

4997b6e

Print logs in tests

d81b89e

Add unit tests for RegisterAndMonitorStatus

0894f40

Local implementation of isPoolLocked

6c0ec08

Add more time before test verification

f21af30

nkuba marked this pull request as ready for review June 29, 2022 13:37

Base automatically changed from go-bindings-for-v2-contracts to main June 29, 2022 13:38

pdyraga reviewed Jul 4, 2022

View reviewed changes

pdyraga mentioned this pull request Jul 5, 2022

Interface and local implementation for sortition chain #3056

Merged

pdyraga closed this Jul 8, 2022

pdyraga deleted the sortition-pool-register branch July 8, 2022 11:26

pdyraga assigned nkuba Jul 8, 2022

pdyraga removed the request for review from dimpar July 15, 2022 12:09

pdyraga modified the milestones: v2.0.0-m0, v2.0.0-m4 Sep 30, 2022

pdyraga unassigned nkuba Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register in Random Beacon's Sortition Pool #3028

Register in Random Beacon's Sortition Pool #3028

Uh oh!

nkuba commented Jun 17, 2022 •

edited

Loading

Uh oh!

pdyraga Jul 4, 2022

Uh oh!

pdyraga Jul 4, 2022

Uh oh!

pdyraga Jul 4, 2022

Uh oh!

pdyraga Jul 4, 2022

Uh oh!

nkuba Jul 4, 2022

Uh oh!

nkuba Jul 4, 2022

Uh oh!

lukasz-zimnoch commented Jul 5, 2022

Uh oh!

pdyraga commented Jul 5, 2022

Uh oh!

pdyraga commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -1 +1 @@
		This directory contains legacy code from V1. It should be removed.
		This directory contains legacy code from V1. It should be moved to fit the new directories structure.

Register in Random Beacon's Sortition Pool #3028

Register in Random Beacon's Sortition Pool #3028

Uh oh!

Conversation

nkuba commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdyraga Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

pdyraga Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

nkuba Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

nkuba Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

lukasz-zimnoch commented Jul 5, 2022

Uh oh!

pdyraga commented Jul 5, 2022

Uh oh!

pdyraga commented Jul 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nkuba commented Jun 17, 2022 •

edited

Loading