feat(erc20signersvc): add erc20 reward signer svc #1346

Yaiba · 2025-02-05T00:28:12Z

This pr adds erc20 reward signer service to kwild's sub service.

Most important change is how erc20 ext process epoch. Now we have three types of epoch:

confirmed epoch, epoch that has been confirmed on EVM
finalized epoch, epoch that has been finalized, wait to be confirmed
collecting epoch, epoch that current collect all new rewards

Except the first epoch, at any given time, there will only be two active epochs: finalized epoch and collecting epoch.
After an finalized epoch is confirmed, when Kwil propose a new epoch:

the current collecting epoch will be finalized
a new collecting epoch will be created

Yaiba · 2025-02-05T22:26:40Z

config/config.go

+	GenesisState  string                       `toml:"genesis_state" comment:"path to the genesis state file, relative to the root directory"`
+	Migrations    MigrationConfig              `toml:"migrations" comment:"zero downtime migration configuration"`
+	Checkpoint    Checkpoint                   `toml:"checkpoint" comment:"checkpoint info for the leader to sync to before proposing a new block"`
+	Erc20RWSigner Erc20RewardSignerConfig      `toml:"erc20_reward_signer" comment:"ERC20 reward signer service configuration"`


I'm not happy with this name, looking for suggestions

Lets start calling it "erc20 bridge".

So ERC20BridgeSignerConfig

sounds good, the name is more accurate

Yaiba · 2025-02-05T22:31:36Z

config/config.go

+			PrivateKeys: nil,
+			Targets:     nil,
+			// the reasonable value is the block time
+			SyncEvery: types.Duration(1 * time.Minute),


Technically we don't need this configuration, but it maybe not what user wants.
Since rn the block time is determined by two consensus parameters(proposeTimeout and emptyBlockTimeout), not easy for the signerSvc to generate a good value based on that two parameters for this configuration. So I'll leave to the user.

We should have this use end block hooks, not polling.

Yaiba · 2025-02-06T17:39:25Z

config/config.go

+type Erc20RewardSignerConfig struct {
+	Enable      bool           `toml:"enable" comment:"enable the ERC20 reward signer service"`
+	Targets     []string       `json:"targets" comment:"target reward ext alias for the ERC20 reward"`
+	PrivateKeys []string       `json:"private_keys" comment:"private key for the ERC20 reward target"`


here the 'PrivateKeys' is an array, so different 'signer' could be configured for different target

Yaiba · 2025-02-19T04:04:39Z

app/node/start.go

-		parts := strings.Split(args[i], ".")
-		if len(parts) != 3 {
+		// there are at least 3 parts.
+		parts := strings.SplitN(args[i], ".", 3)


the 3rd part can have values like URL

Yaiba · 2025-02-19T04:20:34Z

node/services/erc20signersvc/eth.go

+	"github.com/ethereum/go-ethereum/accounts/abi/bind"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/ethclient"
+	"github.com/samber/lo"


Initially I used this libaray for functional operations, but rn this is the only place it's used

Zero deps with it's release tag. 👍

Yaiba · 2025-02-19T04:51:44Z

others worth mention:

Add erc20_reward_signer config section to kwild's config, support multiple ext instance
signer service retries if RPC service is not ready or the configured 'target' erc20 instance is not created
erc20 ext stores temporary vote records on epoch, once an epoch is confirmed, those record will be deleted
use multicall3 if possible, to reduce the rpc roundtrip, and get values from same block

node/exts/erc20reward/meta_sql.go

brennanjl · 2025-02-19T15:23:05Z

app/node/start.go

-		parts := strings.Split(args[i], ".")
-		if len(parts) != 3 {
+		// there are at least 3 parts.
+		parts := strings.SplitN(args[i], ".", 3)


brennanjl · 2025-02-19T15:55:37Z

config/config.go

+			PrivateKeys: nil,
+			Targets:     nil,
+			// the reasonable value is the block time
+			SyncEvery: types.Duration(1 * time.Minute),


We should have this use end block hooks, not polling.

brennanjl · 2025-02-19T15:57:28Z

config/config.go

@@ -311,6 +311,13 @@ func DefaultConfig() *Config {
 			Height: 0,
 			Hash:   types.Hash{},
 		},
+		Erc20RWSigner: Erc20RewardSignerConfig{
+			Enable:      false,
+			PrivateKeys: nil,


Private keys should point to file paths. not the private key itself.

hmm, so all keys in one file?

No, the user can specify many files paths just like how they specify many keys.

brennanjl · 2025-02-19T15:59:06Z

config/config.go

+}
+
+func (cfg Erc20RewardSignerConfig) Validate() error {
+	if (len(cfg.PrivateKeys) != len(cfg.Targets)) && (len(cfg.EthRpcs) != len(cfg.Targets)) {


We should have this tap into the eth rpcs that the user already has configured. Will follow up on this in-person, but users would essentially be doubly configuring RPCs here

It seems like we should have a structure like:

type ERC20BridgeSignerConfig struct { PrivateKeyPath string `toml:"private_key_path" comment:"private key file path for the ERC20 reward target"` Chain string `toml:"chain" comment:"chain name for the ERC20 reward target"` }

which would be stored in the top-level node config as:

type Config struct { // ... ERC20Bridge map[string]ERC20BridgeSignerConfig `toml:"erc20_bridge" comment:"ERC20 bridge signer service configuration"`

yeah thought abut that, I think we should do it. Will look into it

node/exts/erc20reward/meta_schema.sql

brennanjl · 2025-02-19T16:05:16Z

node/exts/erc20reward/meta_sql.go

 	return app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `
 	{kwil_erc20_meta}UPDATE reward_instances
-	SET balance = balance - $amount;
+	SET balance = balance - $amount
+    WHERE id = $instance_id;


brennanjl · 2025-02-19T16:06:00Z

node/exts/erc20reward/meta_sql.go

+}
+
+// previousEpochConfirmed return whether previous exists and confirmed.
+func previousEpochConfirmed(ctx context.Context, app *common.App, instanceID *types.UUID, endBlock int64) (bool, bool, error) {


nit: can you name the returns so that it is clear what each one means? just because there are two bools

brennanjl · 2025-02-19T16:07:47Z

node/exts/erc20reward/meta_sql.go

+	}
+
+	return confirmed, nil
+}


You should probably add an error here where it errors out if the epoch doesn't exist. I'd do this by simply adding a counter than increments on each callback function passed to the engine. If the counter is 0, then the epoch with that ID does not exist.

This seems more correct than simply returning false

yeah make sense

brennanjl · 2025-02-19T16:13:04Z

node/exts/erc20reward/meta_extension.go

+								return err
+							}
+
+							confirmed, err := epochConfirmed(ctx.TxContext.Ctx, app, epochID)


Seems like there is a bug here.

Epochs have 3 stages:

collecting

finalized

confirmed

Only an epoch in the "finalized" phase can be signed. But this check only ensures that "confirmed" epochs cannot be signed; it does not protect against signers voting for a "collecting" epoch.

yeah you're right

brennanjl · 2025-02-19T16:15:38Z

node/exts/erc20reward/meta_extension.go

@@ -68,6 +71,8 @@ var (
 	// so that we know how to decode them
 	logTypeTransfer       = []byte("e20trsnfr")
 	logTypeConfirmedEpoch = []byte("cnfepch")
+
+	mtLRUCache = lru.NewMap[[32]byte, []byte](rewardMerkleTreeLRUSize) // tree root => tree body


brennanjl · 2025-02-19T16:26:55Z

node/services/erc20signersvc/eth.go

@@ -0,0 +1,221 @@
+package signersvc


seems sort've weird to have a services package, since the erc20 signer service is in no way actually a service of any kind.

We probably should put both this package and the extension package into an erc20-bridge package

This will allow us to have one common place to store abis, manage a node's configuration of RPCs (which are used both by validators and signers), etc.

I like that, will move

brennanjl · 2025-02-19T16:29:22Z

node/services/erc20signersvc/kwil.go

+	procedure := "info"
+	input := []any{}
+
+	res, err := k.clt.Call(ctx, k.namespace, procedure, input)


You essentially have this node calling itself, but that seems like a weird and unnecessary hack?

Why don't we just properly make it a new module that uses the engine?

yeah it becomes weird now the signerSvc could a reference to the engine, gonna change this

This is a little bit related to the polling, in that we kind want to avoid using RPC service. We've agreed that for polling we don't want to react on every block, then I don't see much difference between using end_of_block and just use a ticker.

After a bit experiment, IMO, RPC might be the preferred(and easier) way to communicate with erc20 ext, reasons are:

although we can use engine to call ext VIEW methods, but we cannot call methods(i.e., vote_epoch) that change the state(because it's not deterministic); thus we need to use two sets of API handling to communicate with extension

when signeSvc call vote_epoch, there is no easy way to call the RPC service method. You need to construct the payload manually, well, if this is the case Idk why we don't just use client SDK to do this.

brennanjl · 2025-02-19T16:40:26Z

node/services/erc20signersvc/kwil.go

+	VoteSignatures [][]byte
+}
+
+type FinalizedReward struct {


It seems weird to doubly declare these. We should really consolidate the signer and extension logic.

previously I define the return types in the extension, and use them in the method return, later the API could reuse that. Is this what you mean?

node/exts/erc20reward/named_extension.go

Yaiba · 2025-02-20T04:01:01Z

config/config.go

@@ -335,6 +341,7 @@ type Config struct {
 	GenesisState string                       `toml:"genesis_state" comment:"path to the genesis state file, relative to the root directory"`
 	Migrations   MigrationConfig              `toml:"migrations" comment:"zero downtime migration configuration"`
 	Checkpoint   Checkpoint                   `toml:"checkpoint" comment:"checkpoint info for the leader to sync to before proposing a new block"`
+	Erc20Bridge  ERC20BridgeConfig            `toml:"erc20_bridge" comment:"ERC20 bridge configuration"`


This config is used in two places:

evm-sync extension

erc20-bridge extension

There are few places like oracle still use the general extension config, which I'm not sure if we should move them under erc20_bridge cofiguration.

brennanjl · 2025-02-20T22:49:56Z

app/node/build.go

+		failBuild(err, "Failed to load erc20 bridge signer state file")
+	}
+
+	rpcUrl := "http://" + d.cfg.RPC.ListenAddress


Will this always work? Are ways that a user could set their listen address where it may not?

If so, an alternative would be to give the signer access to the local mempool

now you mentioned it, do we expected to configure a public ip in ListenAddress?

brennanjl · 2025-02-20T22:51:03Z

app/node/build.go

@@ -504,6 +508,33 @@ func buildConsensusEngine(_ context.Context, d *coreDependencies, db *pg.DB,
 	return ce
 }

+func buildErc20RWignerMgr(d *coreDependencies) *signersvc.ServiceMgr {


Seems like a typo: buildErc20RWignerMgr -> buildErc20RSignerMgr

brennanjl · 2025-02-20T23:25:21Z

node/exts/erc20-bridge/erc20/meta_extension.go

+								return fmt.Errorf("signature is not 65 bytes")
+							}
+
+							from, err := ethAddressFromHex(ctx.TxContext.Caller)


Shouldn't we check the signature here?

I commented at line 1063, basically we don't have enough info to verify

I created #1399 for this

I commented at line 1063

The issue seems to be that I cannot read.

Yaiba · 2025-02-21T18:06:05Z

node/exts/erc20-bridge/signersvc/kwil.go

-		err = types.ScanTo(v, &er.ID, &er.StartHeight, &er.StartTimestamp, &er.EndHeight,
-			&er.RewardRoot, &er.RewardAmount, &er.EndBlockHash, &er.Confirmed, &er.Voters, &er.VoteNonces, &er.VoteSignatures)
+// execute mimics client.Client.Execute, without client options.
+func (k *signerClient) execute(ctx context.Context, namespace string, txSigner auth.Signer, action string, tuples [][]any) (types.Hash, error) {


still a bit hacky, if it's worth it we should implement a internal TxSvcClient. For now this is all I needed for signer service

Yaiba · 2025-02-21T18:11:19Z

node/exts/erc20-bridge/signersvc/kwil.go

 }

-var _ erc20ExtAPI = (*erc20rwExtApi)(nil)
+type signerClient struct {


Another way to implement signerClient is to call the usersvc.Service. This approach will have unnecessary wrap/unwrap the request/response types, should be easier to implement

jchappelow · 2025-02-26T20:45:22Z

Sorry, we removed types.ResultBroadcastTx recently since it was modeled after a very odd cometbft type that conflated tx execution code with a broadcast error code (even before the tx is executed). The returns are now a types.Hash and a *types.TxResult. Changes were made in #1405

Yaiba · 2025-02-26T20:46:46Z

Sorry, we removed types.ResultBroadcastTx recently since it was modeled after a very odd cometbft type that conflated tx execution code with a broadcast error code (even before the tx is executed). The returns are now a types.Hash and a *types.TxResult. Changes were made in #1405

thanks for the reminder, I'll make the change accordingly

Yaiba commented Feb 5, 2025

View reviewed changes

Yaiba force-pushed the feature/erc20-signersvc branch 3 times, most recently from 87794d1 to 73b23a4 Compare February 6, 2025 17:31

Yaiba commented Feb 6, 2025

View reviewed changes

Yaiba force-pushed the feature/erc20-signersvc branch 3 times, most recently from 56742f2 to 1e66c34 Compare February 19, 2025 03:38

Yaiba commented Feb 19, 2025

View reviewed changes

Yaiba changed the title ~~[WIP] feat(erc20signersvc): add erc20 reward signer svc~~ feat(erc20signersvc): add erc20 reward signer svc Feb 19, 2025

Yaiba force-pushed the feature/erc20-signersvc branch from 1e66c34 to a34fbae Compare February 19, 2025 04:42

Yaiba marked this pull request as ready for review February 19, 2025 04:43

Yaiba requested a review from brennanjl February 19, 2025 15:16

Yaiba commented Feb 19, 2025

View reviewed changes

node/exts/erc20reward/meta_sql.go Outdated Show resolved Hide resolved

brennanjl reviewed Feb 19, 2025

View reviewed changes

node/exts/erc20reward/named_extension.go Outdated Show resolved Hide resolved

Yaiba force-pushed the feature/erc20-signersvc branch 2 times, most recently from c4240fa to fba0ec0 Compare February 20, 2025 00:07

Yaiba commented Feb 20, 2025

View reviewed changes

brennanjl reviewed Feb 20, 2025

View reviewed changes

Yaiba commented Feb 21, 2025

View reviewed changes

feat(erc20bridge): add erc20 bridge signer svc

94604e5

Yaiba added 3 commits February 26, 2025 15:39

use internal ext client

53cd35e

add bridge method to withdraw balance

97f7861

adjust removing ResultBroadcastTx

efae332

Yaiba force-pushed the feature/erc20-signersvc branch from c5d58a6 to efae332 Compare February 26, 2025 21:39

fix lint

e6b1b49

charithabandi approved these changes Feb 26, 2025

View reviewed changes

Yaiba merged commit 3f95602 into kwilteam:main Feb 26, 2025
2 checks passed

feat(erc20signersvc): add erc20 reward signer svc #1346

feat(erc20signersvc): add erc20 reward signer svc #1346

Conversation

Yaiba commented Feb 5, 2025 • edited Loading

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

Yaiba Feb 6, 2025 • edited Loading

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

Yaiba commented Feb 19, 2025

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

Yaiba Feb 19, 2025 • edited Loading

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

jchappelow commented Feb 26, 2025

Yaiba commented Feb 26, 2025

Yaiba commented Feb 5, 2025 •

edited

Loading

Yaiba Feb 6, 2025 •

edited

Loading

Yaiba Feb 19, 2025 •

edited

Loading