Feat: use cmd to setup data committee #3

lesterli · 2023-09-06T10:34:16Z

Summary

SNA-301, add a new CLI command to register DAC node
SNA-302, add a new command make run-validium in the makefile

Test Plan

Update the docker image, docker pull snapchain/cdk-validium-node
Run make run-validium

* Hotfix v0.1.4 to main (0xPolygonHermez#2250) * fix concurrent web socket writes * fix eth_syncing * fix custom trace internal tx call error handling and update prover * add test to custom tracer depth issue; fix internal call error and gas used * fix custom tracer for internal tx with error and no more steps after it * remove debug code * Make max grpc message size configurable (0xPolygonHermez#2179) * make max grpc message size configurable * fix state tests * fix tests * fix tests * get SequencerNodeURI from SC if empty and not IsTrustedSequencer * Optimize trace (0xPolygonHermez#2183) * optimize trace * fix memory reading * update docker image * update prover image * fix converter * fix memory * fix step memory * fix structlogs * fix structlogs * fix structlogs * fix structlogs * fix structlogs * fix structlogs * fix structlogs * fix structlogs * update prover image * fix struclogs * fix memory size * fix memory size * fix memory size * refactor memory resize * refactor memory resize * move log for the best fitting tx (0xPolygonHermez#2192) * fix load zkCounters from pool * remove unnecessary log.info * add custom tracer support to CREATES opcode without depth increase (0xPolygonHermez#2213) * logs * fix getting stateroot from previous batch (GetWIPBatch) * logs * Fix GetWipBatch when previous last batch is a forced batch * fix forcedBatch trusted state * Revert "fix getting stateroot from previous batch (GetWIPBatch)" This reverts commit 860f0e7. * force GHA * add pool limits (0xPolygonHermez#2189) * Hotfix/batch l2 data (0xPolygonHermez#2223) * Fix BatchL2Data * Force GHA * remove failed txs from the pool limit check (0xPolygonHermez#2233) * debug trace by batch number via external rpc requests (0xPolygonHermez#2235) * fix trace batch remote requests in parallel limitation (0xPolygonHermez#2244) * Added RPC.TraceBatchUseHTTPS config parameter * fix executor version --------- Co-authored-by: tclemos <[email protected]> Co-authored-by: tclemos <[email protected]> Co-authored-by: Toni Ramírez <[email protected]> Co-authored-by: agnusmor <[email protected]> Co-authored-by: agnusmor <[email protected]> Co-authored-by: Thiago Coimbra Lemos <[email protected]> * fix test * fix test --------- Co-authored-by: tclemos <[email protected]> Co-authored-by: tclemos <[email protected]> Co-authored-by: Toni Ramírez <[email protected]> Co-authored-by: agnusmor <[email protected]> Co-authored-by: agnusmor <[email protected]> Co-authored-by: Thiago Coimbra Lemos <[email protected]>

* effective GasPrice refactor * bugs fixes and finalizer tests fixes * fix typo * fix calculate effective gasprice percentage * fix test gas price

…#2258) * effective gas price returned by the rpc in the receipt * linter

…#2260) * bugfix: fixing l2blocks timestamp for the fist batch Signed-off-by: Nikolay Nedkov <[email protected]> * fix finalizer unit test --------- Signed-off-by: Nikolay Nedkov <[email protected]>

…ssword from etherman.Config that are not in use

…ermez#2200-add-documentation-for-node-config-file-2

* fix fea2scalar and gas used * suggestion * fix fea2scalar * suggestion

* fix pending tx when duplicate nonce * set pool.transaction.failed_reason to NULL when updating an existing tx * add more log details when adding tx to AddrQueue * fix query to add tx to the pool. Fix lint errors * change failed_reason for tx discarded due duplicate nonce

…ermez#2273) * Return a tx from the pool only if it is * fix TestGetTransactionByHash --------- Co-authored-by: agnusmor <[email protected]>

…xPolygonHermez#2200-add-documentation-for-node-config-file-2 Feature/0xPolygonHermez#2200 generate json-schema + docs for node config file and network_custom

…t forkId is under 5. Signed-off-by: Nikolay Nedkov <[email protected]>

…add-check-for-fork-id-to-skip-effectivePercentage improve: adding check to skip appending effectivePercentage if current forkId is under 5.

…onHermez#2278)

…om config param. Signed-off-by: Nikolay Nedkov <[email protected]>

…stop-sequencer-on-batch-num feat: adding functionality to stop sequencer on specific batch num from config param.

Signed-off-by: Nikolay Nedkov <[email protected]>

… log conversion (0xPolygonHermez#2280) * fix and check order * linter

…d-print-for-X-Real-IP patch: adding print for X-Real-IP in JSON-RPC

Rename to cdk

Updated README

bap2pecs

nice work! let's discuss whether we want to support multiple DAC member registration at the same time or not

bap2pecs · 2023-09-07T03:39:52Z

cmd/setupcommittee.go

+		return err
+	}
+	if !ctx.Bool(config.FlagYes) {
+		fmt.Println("*WARNING* Are you sure you want to setup committee? [y/N]: ")


I think we should also print out some key info here (e.g. the URL, the address)

bap2pecs · 2023-09-07T03:52:46Z

cmd/setupcommittee.go

+		return err
+	}
+
+	dataCommitteeContract := c.DataAvailability.L1.DataCommitteeAddress


there is dacSC defined below, this name is a bit confusing

I think we can rename dataCommitteeContract to dacSCAddress

bap2pecs · 2023-09-07T03:53:33Z

cmd/setupcommittee.go

+		return err
+	}
+
+	const nSignatures = 1


in this command, we assume the user is always going to only add 1 DAC member. I think we should support multiple per the interface

function setupCommittee( uint _requiredAmountOfSignatures, string[] calldata urls, bytes calldata addrsBytes )

so here nSignatures will be a user param

bap2pecs · 2023-09-07T03:57:19Z

cmd/setupcommittee.go

+	if err != nil {
+		return err
+	}
+	log.Infof("decrypting dac member key from: %v", c.DataAvailability.PrivateKey.Path)


why do we use fmt.Println sometimes and log in other places? do you know the differences?

bap2pecs · 2023-09-07T04:08:16Z

cmd/setupcommittee.go

+	}
+	dacMemberAddress := dacMemberKey.Address
+	fmt.Println("dacMemberAddress: ", dacMemberAddress)
+	dacServiceURL := fmt.Sprintf("http://%s:%d", c.DataAvailability.RPC.Host, c.DataAvailability.RPC.Port)


what if the service url is HTTPS? if we ask the user to specify the URL, this problem will be resolved

bap2pecs · 2023-09-07T04:19:37Z

cmd/setupcommittee.go

+	addrsBytes := []byte{}
+	urls := []string{}
+
+	// load dac member from keystore file


I don't think we need to load the DAC member from the keystore file. also given that if we were to support multiple DAC member, we need to load multiple files and that will need multiple passwords to decrypt those files which can be too complicated.

can we keep it simple by just asking the user to specify the list of DAC member addresses and URLs in the config file?

yes, in general, the admin is the one who sends the L1 tx to setup the committee, but this admin:

shouldn't be part of the committee

shouldn't know the private keys of the committee

this is intentional, as the hole idea is that the DAC and the admin of the network are different entities

arnaubennassar

The DAC config shouldn't be part of the config of the node. Instead, the CLI to setup the committee should have a custom input (either in the form of a config file, or directly through the CLI) that has the following data:

N required signatures
List of DAC members {address, URL}

arnaubennassar · 2023-09-07T08:56:55Z

cmd/main.go

+				&cli.StringFlag{
+					Name:     config.FlagPassword,
+					Aliases:  []string{"pw"},
+					Usage:    "the password do decrypt the key store file",


Suggested change

Usage: "the password do decrypt the key store file",

Usage: "the password to decrypt the key store file",

arnaubennassar · 2023-09-07T08:59:46Z

cmd/setupcommittee.go

+		return err
+	}
+
+	const nSignatures = 1


arnaubennassar · 2023-09-07T09:02:52Z

cmd/setupcommittee.go

+	addrsBytes := []byte{}
+	urls := []string{}
+
+	// load dac member from keystore file


yes, in general, the admin is the one who sends the L1 tx to setup the committee, but this admin:

shouldn't be part of the committee

shouldn't know the private keys of the committee

this is intentional, as the hole idea is that the DAC and the admin of the network are different entities

bap2pecs · 2023-09-07T09:14:49Z

@lesterli let's also modify the test at https://github.com/Snapchain/cdk-validium-node/blob/develop/test/e2e/datacommittee_test.go to use the CLI to register the committee so we can test this new feature

ToniRamirezM and others added 30 commits June 30, 2023 16:40

fix null effective_percentage

2ef0c25

fix forkID calculation

7980151

fix script

224a11e

generate json-schema + docs for node config file and network_custom

13bcd29

fix unittest

41c63ab

Effective GasPrice refactor+fixes (0xPolygonHermez#2247)

d055c40

* effective GasPrice refactor * bugs fixes and finalizer tests fixes * fix typo * fix calculate effective gasprice percentage * fix test gas price

Fix/0xPolygonHermez#2257 effective gas price receipt (0xPolygonHermez…

76dc7d5

…#2258) * effective gas price returned by the rpc in the receipt * linter

bugfix: fixing l2blocks timestamp for the fist batch (0xPolygonHermez…

87200f1

…#2260) * bugfix: fixing l2blocks timestamp for the fist batch Signed-off-by: Nikolay Nedkov <[email protected]> * fix finalizer unit test --------- Signed-off-by: Nikolay Nedkov <[email protected]>

add more comments, and removed fields PrivateKeyPath and PrivateKeyPa…

9f39433

…ssword from etherman.Config that are not in use

add info to git action

b21258d

add info to git action

7197f4e

fix github action

7e3d34d

Merge remote-tracking branch 'origin/develop' into feature/0xPolygonH…

8566eed

…ermez#2200-add-documentation-for-node-config-file-2

updated comments

b6e034f

updated comments

71541ee

Fix/0xPolygonHermez#2263 gas used (0xPolygonHermez#2264)

623089a

* fix fea2scalar and gas used * suggestion * fix fea2scalar * suggestion

Only return a tx from the pool if tx is in pending status (0xPolygonH…

40b084b

…ermez#2273) * Return a tx from the pool only if it is * fix TestGetTransactionByHash --------- Co-authored-by: agnusmor <[email protected]>

Merge pull request 0xPolygonHermez#2252 from 0xPolygonHermez/feature/0…

17a95f5

…xPolygonHermez#2200-add-documentation-for-node-config-file-2 Feature/0xPolygonHermez#2200 generate json-schema + docs for node config file and network_custom

fix documentation with config file

867aaaf

improve: adding check to skip appending effectivePercentage if curren…

198243a

…t forkId is under 5. Signed-off-by: Nikolay Nedkov <[email protected]>

Merge pull request 0xPolygonHermez#2275 from 0xPolygonHermez/improve/…

c00e4fa

…add-check-for-fork-id-to-skip-effectivePercentage improve: adding check to skip appending effectivePercentage if current forkId is under 5.

Fiex effectiveGasprice unsigned txs with forkId lower than 5 (0xPolyg…

861dbba

…onHermez#2278)

feat: adding functionality to stop sequencer on specific batch num fr…

e7e9f5b

…om config param. Signed-off-by: Nikolay Nedkov <[email protected]>

Merge pull request 0xPolygonHermez#2282 from 0xPolygonHermez/feature/…

caa9a3a

…stop-sequencer-on-batch-num feat: adding functionality to stop sequencer on specific batch num from config param.

patch: adding print for X-Real-IP in JSON-RPC

a4d0b88

Signed-off-by: Nikolay Nedkov <[email protected]>

Fix checkIfSynced (0xPolygonHermez#2289)

32bf764

[Rehashing] Check logs order and fix blockhash and blockNumber in the…

b5d5145

… log conversion (0xPolygonHermez#2280) * fix and check order * linter

Merge pull request 0xPolygonHermez#2286 from 0xPolygonHermez/patch/ad…

2b1ea5f

…d-print-for-X-Real-IP patch: adding print for X-Real-IP in JSON-RPC

arnaubennassar and others added 23 commits August 24, 2023 14:45

Rename dac

59028cf

gen docs and mocks

0a84fe9

Fix env prefix

8fa620b

WIP

74ad1fb

WIP

33e23d5

WIP

298a66f

Merge pull request 0xPolygon#13 from 0xPolygon/cdk

53a4ff4

Rename to cdk

Updated README

c95bf34

Updated README

a524449

Updated README

5fe7487

Updated README

1e872a0

Updated README

78f8663

Updated README

215eafb

Updated README

a8dbf2c

Updated README

6ef010a

Updated README

81ea6cf

Updated README

a3f9fc7

Merge pull request 0xPolygon#19 from 0xPolygon/readme

9cbba5e

Updated README

Updated README

841a667

Merge pull request 0xPolygon#20 from 0xPolygon/readme

9da5739

Updated README

Feat: SNA-287 zkvalidium integration (#1)

f1211f0

Merge remote-tracking branch 'upstream/develop' into develop

fbd595d

add a new cli cmd to setup data committee

5e2dd2d

lesterli requested a review from bap2pecs September 6, 2023 11:56

lesterli changed the title ~~Feat: SNA-301&302 use cmd to setup data committee~~ Feat: SNA-301 & SNA-302 use cmd to setup data committee Sep 7, 2023

lesterli changed the title ~~Feat: SNA-301 & SNA-302 use cmd to setup data committee~~ Feat: use cmd to setup data committee Sep 7, 2023

bap2pecs requested changes Sep 7, 2023

View reviewed changes

arnaubennassar reviewed Sep 7, 2023

View reviewed changes

bap2pecs force-pushed the develop branch from fbd595d to 9711a4d Compare January 16, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: use cmd to setup data committee #3

Feat: use cmd to setup data committee #3

lesterli commented Sep 6, 2023

bap2pecs left a comment

bap2pecs Sep 7, 2023

bap2pecs Sep 7, 2023

bap2pecs Sep 7, 2023

arnaubennassar Sep 7, 2023

bap2pecs Sep 7, 2023

bap2pecs Sep 7, 2023

bap2pecs Sep 7, 2023

arnaubennassar Sep 7, 2023

arnaubennassar left a comment

arnaubennassar Sep 7, 2023

arnaubennassar Sep 7, 2023

arnaubennassar Sep 7, 2023

bap2pecs commented Sep 7, 2023

	Usage: "the password do decrypt the key store file",
	Usage: "the password to decrypt the key store file",

Feat: use cmd to setup data committee #3

Are you sure you want to change the base?

Feat: use cmd to setup data committee #3

Conversation

lesterli commented Sep 6, 2023

Summary

Test Plan

bap2pecs left a 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

arnaubennassar left a 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

bap2pecs commented Sep 7, 2023