Refactoring of the chain packages #3062

lukasz-zimnoch · 2022-07-06T11:40:47Z

Depends on: #3034

This pull request refactors the chain package structure to match the expectations of v2. I recommend going commit by commit to grasp the changes in an easy way.

Planned packages structure for v2

We aim for the following structure of packages:

pkg
    beacon
        beacon.go <-- main package file exposing `beacon.Initialize` function
        chain.go  <-- declares `beacon.Chain` interface that sets expectations for the required subset of on-chain functions
    tbtc
        tbtc.go   <-- main package file exposing `tbtc.Initialize` function
        chain.go  <-- declares `tbtc.Chain` interface that sets expectations for the required subset of on-chain functions
    chain
        ethereum
            beacon/gen  <-- beacon contract bindings
            tbtc/gen    <-- tbtc contract bindings
            ethereum.go <-- defines `ethereum.Chain` which is a base non-app-specific chain exposing generic features
            beacon.go   <-- defines `ethereum.BeaconChain` which embeds `ethereum.Chain` and implements `beacon.Chain`
            tbtc.go     <-- defines `ethereum.TbtcChain` which embeds `ethereum.Chain` and implements `tbtc.Chain`
            ...         <-- other files like stake_monitor.go, key.go, etc.
        block_counter.go <-- declares chain-agnostic `chain.BlockCounter` interface
        signing.go       <-- declares chain-agnostic `chain.Singing` interface
        stake_monitor.go <-- declares chain-agnostic `chain.StakeMonitor` interface
        ...              <-- other chain-agnostic cross-application interfaces and types

This structure allows each application package (pkg/beacon and pkg/tbtc) to set and control their own expectations regarding the chain. The pkg/chain contains chain implementations that must implement application-specific chain interfaces. This way the chain implementation is decoupled from the application domain code and does not affect it. Chain-agnostic interfaces declared directly in the pkg/chain package represents cross-application chain concepts that are shared by all applications.

Steps towards the desired structure done in this PR

This PR makes the following steps to reach the desired structure:

It renames the legacy pkg/chain/ethereum package to pkg/chain/ethereum_v1 and adjusts all the references
Adds the stub of the new Ethereum implementation in pkg/chain/ethereum that consists of the base ethereum.Chain chain handle placed in ethereum.go and app-specific chain handles ethereum.BeaconChain and ethereum.TbtcChain placed in beacon.go and tbtc.go respectively.
Removes the utility chain functionality to reduce the complexity of chain code
Removes the chain.Handle interface that was living in pkg/chain in favor of small specialized interfaces scattered through separate files like stake_monitor.go, block_counter.go and so on.
Removes the chain.Staker thus the entire concept of staker as it no longer relevant for v2
Moves the Ethereum key code from the legacy to the new package as it remains unchanged
Adds the generic features implementations to the new pkg/chain/ethereum package. Those are the block counter, the stake monitor and the signer.

Next steps that need to be done

To completely transition towards the new structure we still need to:

Move v2 contract bindings under pkg/chain/ethereum
Simplify the pkg/beacon structure to have chain.go at the root
Extend pkg/chain/ethereum to contain references to v2 contract bindings and all the required features
Gradually replace usages of the legacy pkg/chain/ethereum_v1 package in favor of pkg/chain/ethereum.

`pkg/chain/ethereum_v1` We need to make a transition towards the new chain package structure required for V2. Here is the first step.

This is no longer relevant for v2

pdyraga

This is really good start. I would also consider moving:

pkg/chain/random-beacon
pkg/chain/tbtc-v2
pkg/chain/threshold-network
pkg/chain/ecdsa

under ethereum. But maybe in a separate PR to unblock the other work?

pdyraga · 2022-07-06T12:11:27Z

cmd/relay.go

@@ -1,104 +0,0 @@
-package cmd


Let's add an issue to the last milestone to implement the relay command with a reference to this PR where we temporarily remove it.

pkg/chain/ethereum/tbtc.go

This interface was an artificial bag for more specialized chain interfaces. The client code rarely needs all of them at once. In order to make things simpler, we decided to remove the `chain.Handle` interface and let the client code depend on specific chain interfaces.

# Conflicts: # pkg/chain/ethereum/ethereum.go

The v2 client should operate on the concept of the operator and staking provider. That said, the concept of staker is no longer needed. We take an opportunity and cleanup it here.

pkg/chain/signing.go

pdyraga · 2022-07-06T14:36:53Z

pkg/chain/ethereum/stake_monitor.go


-func (esm *ethereumStakeMonitor) HasMinimumStake(
+// TODO: Real implementation with v2 contracts.
+func (sm *stakeMonitor) HasMinimumStake(


@tomaszslabon Your #3060 will need this function. The proposed approach is: the firewall package should define an interface it is expecting, or import chain.StakeMonitor. I think the latter is fine for the time being. Per #3010, we no longer want to check HasMinimumStake but HasStakeDelegation (we should rename this function). And the logic checking staking provider <> operator mapping and the TokenStaking roles should be checked inside HasStakeDelegated.

I think we should check both RandomBeacon and WalletRegistry for staking provider <> operator mapping. We want to have one network of nodes for both applications.

pdyraga · 2022-07-06T14:40:52Z

pkg/chain/ethereum/block_counter.go

+
+// BlockCounter creates a BlockCounter that uses the block number in Ethereum.
+func (c *Chain) BlockCounter() (chain.BlockCounter, error) {
+	return c.blockCounter, nil


What is the idea behind this function? THe block counter comes from keep-common, right? Maybe this function should be a part of ethereum.go? 🤔

We could put in ethereum.go but wanted to keep it in the same way as others generic features and align with how interfaces are declared in pkg/chain. Also, if we want to merge keep-common one day, the block counter will live here.

# Conflicts: # pkg/chain/chain.go

pdyraga · 2022-07-07T07:05:07Z

pkg/chain/ethereum/ethereum.go

+func NewChain(
+	ctx context.Context,
+	config *ethereum.Config,
+	client ethutil.EthereumClient,


This should probably be *ethclient.Client, from github.com/ethereum/go-ethereum/ethclient. We need (in the next PR, I am working on it) obtain ChainID with a call to chainID, err := client.ChainID(context.Background()) and this is not possible with ethutil.EthereumClient.

I did this change in #3067 at commit 012e362. Started building on a top of this PR.

pdyraga · 2022-07-07T07:52:19Z

pkg/chain/ethereum/beacon.go

+	config *ethereum.Config,
+	client ethutil.EthereumClient,
+) (*BeaconChain, error) {
+	chain, err := NewChain(ctx, config, client)


The problem with creating a new instance of the base chain in New* function of the application chain is that we will end up with multiple instances of the base chain: one for Random Beacon and one for TBTC. If the base chain holds the transaction mutex - and this feels like the right place - it means we have two mutexes.

To avoid it, I would consider keeping the base chain as a singleton and creating application chains using public functions of the base chain.

I did this change in a3ba22a in the followup PR of #3067.

Please also see fa75502 that deals with Connect function.

Current `golint` CI job raised a lot of errors in packages that will be removed soon. In the same time `golint` does not allow for ignoring those errors and became deprecated. We decided to remove it in favor of `gofmt` to maintain formatting rules. We will add a new linting tool soon.

pdyraga

Should we modify our pre-commit hooks ?

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

.github/workflows/client.yml

-      - name: Lint Go
-        uses: keep-network/golint-action@v1.0.2
+      - name: Go format
+        uses: Jerome1337/gofmt-action@v1.0.4


Non-blocking but we should fork in case this repository gets deleted one day.

hooks

lukasz-zimnoch · 2022-07-07T11:09:47Z

Should we modify our pre-commit hooks ?

Yeah! See 059ec4f

Cleanup of the `pkg/beacon` package This pull request is the next step of the package refactoring started in #3062. It simplifies the structure of the `pkg/beacon` and makes some general cleanup. Specific changes are described below. Please review commit by commit to easily understand all changes. ### Remove the intermediary `relay` package So far, all the beacon code have lived under `pkg/beacon/relay/*`. The intermediary `relay` package turned out to be redundant and just made the package structure more complicated. This PR removes the `relay` package and places everything directly in the root `pkg/beacon` package. This makes sense because the core logic now lives there. ### Embed the `pkg/beacon/relay.go` file into the `pkg/beacon/node.go` file. There is a `Node` structure that encapsulates the behavior of the beacon node. So far, their code was scattered through two files `relay.go` and `node.go`. The rules behind that split were unclear so we decided to merge them together into a single `node.go` file that defines the `Node` type along with all their methods. ### Make the `Node` type package-private The aforementioned `Node` type is used internally by the `pkg/beacon` and should never be instantiated manually outside of the `pkg/beacon` package. External clients should use `beacon.Initialize` function. That said, we decided to make the `Node` type package-private. ### Adjustments around beacon chain interface So far, the beacon chain interface was living in the `pkg/beacon/relay/chain` package and was divided into two files `chain.go` and `result.go`. Since the `relay` package was removed, the new path is `pkg/beacon/chain` which is more straightforward. To adhere to this change, all code references that used `relaychain.Interface` are now using `beaconchain.Interface`. Also, the `result.go` file content was merged into the `chain.go` file, and all the nomenclature was adjusted to say about the "beacon chain interface" instead of the "relay chain". The interface was also aligned to be compatible with the new `pkg/chain/ethereum` package that we plan to use for v2. Ideally, the beacon chain interface should live directly in the `pkg/beacon` and should be named `beacon.Chain`. Unfortunately, that causes import cycles with the `dkg` and `entry` packages since `pkg/beacon` triggers the logic from there. Making that work would require a bigger refactoring that is described in the last section of this PR (see **Ideas for further refactoring**) ### Remove deprecated `StakerAddress` and replace it with `chain.Address` Since v2 doesn't use the concept of `staker`, the `StakerAddress` type was deprecated recently. Here we remove it and replace all occurrences with the new `chain.Address` type introduced recently. The `chain.Address` type represents a chain-agnostic address and is a good fit to replace the obsolete `StakerAddress`. ### Ideas for further refactoring As mentioned above, the best place for the beacon chain interface is the root `pkg/beacon`. However, that approach brings some import cycles. If we transform the `pkg/beacon/chain.Interface` to be `pkg/beacon.Chain`, the `pkg/beacon/dkg` and `pkg/beacon/entry` packages still need to import it. At the same time, the `node` type living in the `pkg/beacon` triggers the `pkg/beacon/dkg` and `pkg/beacon/entry` logic directly. To break the cycle, we need to make the root `pkg/beacon` independent of their sub-packages that hold the implementation of the DKG and entry signing logic. In order to do so, we would need to: - Move the `dkg.ThresholdSigner` to the `pkg/beacon` package and rename it to `beacon.Signer`. This type seems to belong to the core beacon logic and should not be tied to the specific DKG implementation provided by the `pkg/beacon/dkg` package - Decouple `pkg/beacon` from `pkg/beacon/dkg` and `pkg/beacon/entry` using the `beacon.DkgExecutor` and `beacon.RelayEntryExecutor` interfaces: ``` package beacon // Former `beaconchain.Interface` moved from `pkg/beacon/chain` type Chain interface { ... } // Former `dkg.ThresholdSigner` moved from `pkg/beacon/dkg` type Signer struct { ... } type DkgExecutor interface { Execute(chain Chain, channel net.BroadcastChannel, ...) (*Signer, error) } type RelayEntryExecutor interface { Execute(chain Chain, channel net.BroadcastChannel, signer *Signer, ...) error } ``` - Make `beacon.Intialize` accepting `beacon.DkgExecutor` and `beacon.RelayEntryExecutor` interfaces and use them to trigger DKG and relay entry from within `beacon.node`. Those interfaces would be implemented by `pkg/beacon/dkg` and `pkg/beacon/entry` respectively. Those implementation would be injected in the `cmd/start.go` file. Those changes will introduce a proper direction of dependencies in the entire `pkg/beacon` package and will allow to easily test the `pkg/beacon` code by providing DKG and entry stubs.

lukasz-zimnoch added 3 commits July 6, 2022 13:07

Rename pkg/chain/ethereum to

cded786

`pkg/chain/ethereum_v1` We need to make a transition towards the new chain package structure required for V2. Here is the first step.

Add base ethereum.Chain implementation

05e46b2

Application-specific Ethereum chain handles

5ad3fab

lukasz-zimnoch added the 📟 client label Jul 6, 2022

lukasz-zimnoch added this to the v2.0.0-m1 milestone Jul 6, 2022

lukasz-zimnoch self-assigned this Jul 6, 2022

lukasz-zimnoch requested a review from pdyraga July 6, 2022 11:41

Remove the utility chain interface

2dd5922

This is no longer relevant for v2

lukasz-zimnoch force-pushed the pkg-chain-refactoring branch from 5bac6d2 to 2dd5922 Compare July 6, 2022 12:01

pdyraga reviewed Jul 6, 2022

View reviewed changes

lukasz-zimnoch added 5 commits July 6, 2022 14:34

Merge branch 'dkg-loop' into pkg-chain-refactoring

918227a

# Conflicts: # pkg/chain/ethereum/ethereum.go

Post-merge fix

5dccd0d

Remove the chain.Staker interface

74dc2e5

The v2 client should operate on the concept of the operator and staking provider. That said, the concept of staker is no longer needed. We take an opportunity and cleanup it here.

Move the chain key code

7cb6c84

Base automatically changed from dkg-loop to main July 6, 2022 13:13

lukasz-zimnoch added 4 commits July 6, 2022 15:26

Add stake monitor to the new pkg/chain/ethereum

6b95ac5

Add block counter to the new pkg/chain/ethereum

b13ca72

Add signer to the new pkg/chain/ethereum

ac1fee2

TODO about balance monitoring

dc1ceb6

pdyraga reviewed Jul 6, 2022

View reviewed changes

lukasz-zimnoch added 3 commits July 6, 2022 16:42

s/TBTCChain/TbtcChain

c29d0e5

Merge branch 'main' into pkg-chain-refactoring

c64ff79

# Conflicts: # pkg/chain/chain.go

Add chain-agnostic address type

5be40e4

lukasz-zimnoch marked this pull request as ready for review July 6, 2022 15:18

pdyraga reviewed Jul 7, 2022

View reviewed changes

pdyraga mentioned this pull request Jul 7, 2022

RandomBeacon contract for the Beacon Ethereum chain - sortition functions #3067

Merged

pdyraga reviewed Jul 7, 2022

View reviewed changes

lukasz-zimnoch added 2 commits July 7, 2022 12:08

Fix unit tests

c7336f4

Make gofmt happy

1a5a970

pdyraga reviewed Jul 7, 2022

View reviewed changes

Replace golint with gofmt in the pre-commit

059ec4f

hooks

pdyraga approved these changes Jul 7, 2022

View reviewed changes

pdyraga merged commit 2a1b355 into main Jul 7, 2022

pdyraga deleted the pkg-chain-refactoring branch July 7, 2022 11:53

lukasz-zimnoch mentioned this pull request Jul 8, 2022

Cleanup of the pkg/beacon package #3074

Merged

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

pdyraga unassigned lukasz-zimnoch Sep 30, 2022

Refactoring of the chain packages #3062

Refactoring of the chain packages #3062

Uh oh!

Conversation

lukasz-zimnoch commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Planned packages structure for v2

Steps towards the desired structure done in this PR

Next steps that need to be done

Uh oh!

pdyraga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdyraga Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdyraga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasz-zimnoch commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lukasz-zimnoch commented Jul 6, 2022 •

edited

Loading

pdyraga Jul 7, 2022 •

edited

Loading