feat: Add CreateValidator access control feature #406

0Tech · 2021-12-17T11:09:59Z

Description

The first requirement of the consortium is an access control of CreateValidator message. To implement this feature while not modifying the existing modules, this patch introduces two new modules.

consortium module, the first one, has two proposals. UpdateConsortiumParamsProposal changes the parameter of the module by which you may disable the entire consortium related features. You CANNOT enable the module again after disabling it, because it would bring the public chain back to the consortium chain. Therefore, you must enable it by changing the default genesis state (the default is off). The second one, UpdateValidatorAuthsProposal changes the authorization information of the validators. It is incremental now, but the semantic is subject to change (it could be restful-like). The module has no transactions, but has three queries, one for querying the parameters, and the others for querying validator authorizations.

stakingplus module, which was not included in the first plan, was introduced to mitigate a possible DoS attack vector. Basically, it is a thin wrapper of the original staking module, and it queries validator auth infos from the consortium module. If an operator attempts to create a validator without being authorized first, the transaction will be rejected.

closes: #384

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-12-17T11:20:30Z

Codecov Report

❗ No coverage uploaded for pull request base (main@3913cf0). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #406   +/-   ##
=======================================
  Coverage        ?   53.87%           
=======================================
  Files           ?      683           
  Lines           ?    69906           
  Branches        ?        0           
=======================================
  Hits            ?    37659           
  Misses          ?    29225           
  Partials        ?     3022

0Tech · 2021-12-17T11:14:25Z

proto/lbm/consortium/v1/consortium.proto

+// ValidatorAuth defines authorization info of a validator.
+message ValidatorAuth {
+  string operator_address = 1 [(gogoproto.moretags) = "yaml:\"operator_address\""];
+  bool   creation_allowed = 2 [(gogoproto.moretags) = "yaml:\"creation_allowed\""];
+}


I will simplify the structure if the requirement of the consortium chain is fixed and no further details are added.

simapp/app.go

0Tech · 2021-12-17T11:18:34Z

simapp/app.go

-		staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
+		stakingplus.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsortiumKeeper),


Replace staking with the wrapper stakingplus.

simapp/app.go

x/consortium/abci.go

x/consortium/types/keys.go

x/consortium/types/genesis.go

x/consortium/types/expected_keepers.go

x/consortium/types/errors.go

x/consortium/keeper/validator.go

egonspace

Looks great!

dudong2

Great!

zemyblue · 2021-12-20T12:52:13Z

x/consortium/client/cli/tx.go

+	FlagAllowedValidatorDelete = "delete"
+)
+
+// NewCmdSubmitUpdateConsortiumParamsProposal implements the command to submit a update-consortium-params proposal


to submit a update-consortium-params
->
"an"

And below, I think all a need to be change to an in front of update or don't use an. 😅

zemyblue · 2021-12-20T13:02:32Z

x/consortium/client/cli/tx.go

+			deletingValidators := parseCommaSeparated(deletingValidatorsStr)
+
+			createAuths := func(addings, deletings []string) ([]*types.ValidatorAuth, error) {
+				auths := []*types.ValidatorAuth{}


How about changing like following.

var auths []*types.ValidatorAuth

zemyblue · 2021-12-20T13:18:07Z

x/consortium/client/cli/tx.go

+				return auths, nil
+			}
+
+			auths, err := createAuths(addingValidators, deletingValidators)


What happen if one of the lists to add and one to delete are the same?

I was concerned about the case too, so it checks the duplication in the function and if it's the case, it will emit an error.

And please add a unittest for this case.

zemyblue · 2021-12-20T13:21:17Z

x/consortium/client/rest/query.go

+			return
+		}
+
+		allowedOperators := []string{}


If possible, I think following code is good.

var allowedOperators []string

zemyblue · 2021-12-20T13:24:35Z

x/consortium/genesis.go

+	}
+
+	for _, auth := range validatorAuths {
+		keeper.SetValidatorAuth(ctx, auth)


This function can be occurred error. So need to handle the error.

zemyblue · 2021-12-20T13:28:53Z

x/consortium/keeper/grpc_query.go

+		return nil, status.Error(codes.InvalidArgument, "invalid request")
+	}
+
+	auths := []*types.ValidatorAuth{}


var auths []*types.ValidatorAuth

zemyblue · 2021-12-20T13:34:42Z

x/consortium/keeper/keeper.go

+//
+// CONTRACT: the parameter Subspace must have the param key table already initialized
+func NewKeeper(
+	cdc codec.BinaryMarshaler, key sdk.StoreKey,


If possible, I think it's good to have each line for every params.

zemyblue · 2021-12-20T13:35:28Z

x/consortium/keeper/keeper.go

+	return ctx.Logger().With("module", "x/"+types.ModuleName)
+}
+
+// Clean up the states


to Cleanup

zemyblue · 2021-12-20T13:47:14Z

x/consortium/module.go

+	return []abci.ValidatorUpdate{}
+}
+
+//____________________________________________________________________________


Is it necessary the following codes?

zemyblue · 2021-12-20T13:57:00Z

x/consortium/types/expected_keepers.go

+	StakingKeeper interface {
+		IterateValidators(ctx sdk.Context, fn func(index int64, validator stakingtypes.ValidatorI) (stop bool))
+	}


Good 👍

I thought It's good to describe more explanation about this interface.

zemyblue · 2021-12-20T14:06:35Z

The proposal_handler does not have event log.
I think It's good to add the handling result as event log.

0Tech · 2021-12-21T04:07:41Z

The proposal_handler does not have event log. I think It's good to add the handling result as event log.

I added events to the module. Thank you for your suggestion.

zemyblue · 2021-12-21T04:42:26Z

x/consortium/client/cli/tx.go

@@ -0,0 +1,211 @@
+package cli


I didn't find unittest of this cli codes.
How about adding unittest of thease cli commands?

You can reference how to developing the cli unittest in https://github.com/line/lbm-sdk/blob/main/x/bank/client/cli/cli_test.go

I will make a new issue on this.
If you agree, please approve this PR :)

x/consortium/client/cli/tx.go

zemyblue · 2021-12-21T11:59:25Z

If you change some profo files. Please check the client/docs/statik/statik.go file is changed.
When I check it, the proto was changed, but the statik.go file was not changed. There may be no changes to the statik.go file, but I have no idea what was modified in statik.go. So please check it.

You can update statik.go file with update-swagger-docs command.

* main: (44 commits) feat: rewrite issue template and move PR template (#410) fix: validate validator addresses in update-validator-auths proposal (#411) feat: add `bankplus` function to restrict to send coin to inactive smart contract. (#400) build(deps): bump actions/setup-go from 2.1.4 to 2.1.5 (#408) ci: fix branch name on ci script (#409) feat: Add CreateValidator access control feature (#406) build(deps): bump github.com/spf13/viper from 1.9.0 to 1.10.1 (#403) build(deps): bump github.com/rs/zerolog from 1.26.0 to 1.26.1 (#402) fix: fix query signing infos command (#407) build(deps): bump github.com/spf13/cobra from 1.1.3 to 1.3.0 (#399) build(deps): github.com/ulikunitz/xz from 0.5.5 to 0.5.10 (#398) build(deps): bump actions/stale from 3 to 4.1.0 (#396) build(deps): bump actions/cache from 2.1.3 to 2.1.7 (#386) feat: Add the `instantiate_permission` in the `CodeInfoResponse` (#395) fix: fix bug where `StoreCodeAndInstantiateContract`, `UpdateContractStatus`, `UpdateContractStatusProposal` API does not work (#393) docs: modify with latest version of swagger REST interface docs. (#392) fix: fix invalid root hash by bumping up tm-db (#388) chore: fix swagger's config path for wasm (#391) build(deps): bump technote-space/get-diff-action from 5.0.1 to 5.0.2 (#379) fix: update allowance inside AllowedMsgAllowance (#383) ...

0Tech added 9 commits December 17, 2021 09:23

feat: add consortium proto

35de35e

feat: add consortium module

51b808e

feat: add consortium to simapp

3cebc2c

refactor: new protos

b96d363

refactor: consortium module

8ca7327

refactor: simapp

a4633ee

test: add unit-tests for consortium

084f1d3

feat: do not use hooks and endblocker of consortium

bb8eecb

feat: introduce stakingplus and replace staking with it

445a304

0Tech changed the title ~~Add CreateValidator access control feature~~ feat: Add CreateValidator access control feature Dec 17, 2021

0Tech commented Dec 17, 2021

View reviewed changes

0Tech added 4 commits December 17, 2021 12:15

chore: lint

ba32de7

chore: cleanup unused codes and add additional infos to errors

db7cd27

chore: lint

6f0ce0d

chore: revert hooks and blocker formats

2a1c36b

0Tech marked this pull request as ready for review December 20, 2021 00:01

0Tech requested review from egonspace, dudong2 and iproudhon December 20, 2021 00:01

docs: update CHANGELOG

4009b59

egonspace added this to the Ebony alpha milestone Dec 20, 2021

egonspace added the A: improvement Changes in existing functionality label Dec 20, 2021

egonspace assigned 0Tech Dec 20, 2021

egonspace approved these changes Dec 20, 2021

View reviewed changes

0Tech added 2 commits December 20, 2021 06:05

chore: fix var names and comments

d58123e

chore: lint

1245d2f

dudong2 approved these changes Dec 20, 2021

View reviewed changes

zemyblue reviewed Dec 20, 2021

View reviewed changes

0Tech added 3 commits December 21, 2021 00:47

refactor: apply feedbacks

a0ea004

feat: add event proto for consortium

bad8e0a

feat: emit events in consortium proposals

1c49042

zemyblue reviewed Dec 21, 2021

View reviewed changes

dudong2 reviewed Dec 21, 2021

View reviewed changes

x/consortium/client/cli/tx.go Show resolved Hide resolved

0Tech added 2 commits December 22, 2021 10:11

docs: add consortium apis to config

6d621e7

refactor: remove deprecated REST handlers

3136b1f

zemyblue approved these changes Dec 22, 2021

View reviewed changes

0Tech merged commit 0e7b3cf into Finschia:main Dec 23, 2021

0Tech deleted the consortium branch January 17, 2022 09:54

zemyblue mentioned this pull request Jan 26, 2022

docs: release 0.44.0-rc0 #420

Merged

5 tasks

0Tech added C:x/foundation x/foundation module A: feature New features C:x/stakingplus and removed A: improvement Changes in existing functionality labels Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add CreateValidator access control feature #406

feat: Add CreateValidator access control feature #406

0Tech commented Dec 17, 2021 •

edited

Loading

codecov bot commented Dec 17, 2021 •

edited

Loading

0Tech Dec 17, 2021

0Tech Dec 17, 2021

egonspace left a comment

dudong2 left a comment

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

0Tech Dec 21, 2021

zemyblue Dec 21, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue Dec 20, 2021

zemyblue commented Dec 20, 2021

0Tech commented Dec 21, 2021

zemyblue Dec 21, 2021

0Tech Dec 21, 2021

zemyblue commented Dec 21, 2021 •

edited

Loading

		staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
		stakingplus.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsortiumKeeper),

feat: Add CreateValidator access control feature #406

feat: Add CreateValidator access control feature #406

Conversation

0Tech commented Dec 17, 2021 • edited Loading

Description

codecov bot commented Dec 17, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egonspace left a comment

Choose a reason for hiding this comment

dudong2 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

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

zemyblue commented Dec 20, 2021

0Tech commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zemyblue commented Dec 21, 2021 • edited Loading

0Tech commented Dec 17, 2021 •

edited

Loading

codecov bot commented Dec 17, 2021 •

edited

Loading

zemyblue commented Dec 21, 2021 •

edited

Loading