feat: checkpointing/implement BLS signer #104

gitferry · 2022-08-26T06:18:32Z

This is a draft PR for implementing BLS signer. This PR aims to prepare a MsgAddBlsSig and send it to Tendermint at the end of each epoch. I'm not sure if it works. More importantly, I'm not sure how to test if the is actually sent to Tendermint. Is there a light-weight way to do this?

aakoshh

Getting there for sure!

I don't know if this can be tested during the SDK simulation tests.

About unit tests, there is a MockClient in the SDK that returns errors at broadcast attemts, so it's not exactly good for us, but a similar client could be used for testing that broadcasting happens at all during block processing. You could pass a client context to the keeper, and in tests it would be a mock client. Similar things happen in other CLI command tests, but most of them don't use a Client, they just set various properties of the context.

How to test if you pass the right client context in the real program flow? Maybe we can just rely on integration tests for that part. The unit tests ensure that the right thing gets signed and the right broadcast method is used, and the integration test ensures that the configuration is correct, that eventually the checkpoint is created on the ledger.

x/checkpointing/keeper/bls_signer.go

aakoshh · 2022-08-26T20:36:04Z

x/checkpointing/keeper/bls_signer.go

+	fs := pflag.NewFlagSet("checkpointing", pflag.ContinueOnError)
+	err = fs.Set(flags.FlagFrom, k.GetParams(ctx).String())


What is this bit doing exactly?

To be honest, I'm not sure. I thought this is a necessary ingredient to compose a transaction, but I'm not sure I'm doing it right. See here.

x/checkpointing/keeper/bls_signer.go

aakoshh · 2022-08-26T20:44:02Z

x/checkpointing/keeper/bls_signer.go

+	if err != nil {
+		return err
+	}
+	err = tx.GenerateOrBroadcastTxCLI(client.Context{}, fs, msg)


Don't you have to set some fields on the client context, such as BroadcastMode and Client?

Basically everything that happens in the CLI to initialize the context. Maybe this can be injected into the keeper.

Thanks for sharing the link. I was not sure what to put in the context. Let me run some tests

x/checkpointing/abci.go

gitferry · 2022-08-29T13:43:29Z

Thanks, @aakoshh and @vitsalis for the discussion. I have updated the draft PR and it's ready for review. I'm still struggling with injecting the client.Context to the checkpointing keeper. In the unit test, I used some utilities to customize a client.Context and sent it to the keeper SendBlsSig() and It worked.

But I still think we should inject the client.Context when initializing the checkpointing keeper. I left that as a TODO as I haven't figured out how to do that. For example, the client.Context requires a keyring. How do we get the keyring when starting the checkpointing keeper?

aakoshh · 2022-08-29T19:38:20Z

How do we get the keyring when starting the checkpointing keeper?

The same way we do when we run a client CLI command? It has to be in the home directory, like a wallet, doesn't it? The operator has to make sure that the node has the files available. And it should fail during startup, not later during epoch switches.

aakoshh

Nice work with the mocks!

Perhaps it's possible to inject the client context to the app, rather than the config, from the root. See comments.

aakoshh · 2022-08-29T22:22:10Z

Makefile

@@ -120,6 +120,16 @@ $(BUILDDIR)/:

 .PHONY: build build-linux

+mockgen_cmd=go run github.com/golang/mock/mockgen@v1.6.0
+
+mocks: $(MOCKS_DIR)


Maybe instead of a one-time task that you run manually, the actual output files could be targets that depend on the inputs, so that if the input changes, then the mock is automatically regenerated. And tests could depend on the mock files to force them to be generated.

That would be cool. But I'm not sure how to do that in Makefile. Could you please show me an example?

This is a good post about Make: https://www.olioapps.com/blog/the-lost-art-of-the-makefile/

It would look something like this:

mockgen_cmd=go run github.com/golang/mock/mockgen@v1.6.0 -package mocks testutil/mocks/checkpointing_expected_keepers.go: x/checkpointing/types/expected_keepers.go $(MOCKS_DIR) $(mockgen_cmd) -source=$< -destination $@ testutil/mocks/bls_signer.go: x/checkpointing/keeper/bls_signer.go $(MOCKS_DIR) $(mockgen_cmd) -source=$< -destination $@ MOCK_FILES = \ bls_signer.go \ checkpointing_expected_keepers.go MOCK_PATHS := $(patsubst %, testutil/mocks/%, $(MOCK_FILES)) run-tests: $(MOCK_PATHS) ...

The only difference is the regeneration if the source changes.

Makefile

aakoshh · 2022-08-29T22:23:53Z

app/app.go

+	pvKeyFile := nodeCfg.PrivValidatorKeyFile()
+	if err := tmos.EnsureDir(filepath.Dir(pvKeyFile), 0777); err != nil {
+		panic(err)
+	}
+	pvStateFile := nodeCfg.PrivValidatorStateFile()
+	if err := tmos.EnsureDir(filepath.Dir(pvStateFile), 0777); err != nil {
+		panic(err)
+	}


Does this allow non-validator nodes to run, a.k.a. "full nodes"?

Yes...It's wired that I cannot get the flagWithTendermint from newApp because it is not accessible. Maybe we need to add a new flag for this. I added a TODO.

app/app.go

…ting/bls-signer

gitferry · 2022-08-31T02:02:12Z

Thanks, @aakoshh for your advice! I managed to inject client context into the checkpointing keeper at root.go newApp. Please see the updates.

aakoshh · 2022-08-31T15:27:33Z

app/utils.go

+		return nil, err
+	}
+	pvStateFile := nodeCfg.PrivValidatorStateFile()
+	err = tmos.EnsureDir(filepath.Dir(pvStateFile), 0777)


Not returning if err != nil?

aakoshh · 2022-08-31T15:28:25Z

app/test_helpers.go

+	nodeCfg := tmconfig.DefaultConfig()
+	pvKeyFile := nodeCfg.PrivValidatorKeyFile()
+	err := tmos.EnsureDir(filepath.Dir(pvKeyFile), 0777)
+	if err != nil {
+		return nil, err
+	}
+	pvStateFile := nodeCfg.PrivValidatorStateFile()
+	err = tmos.EnsureDir(filepath.Dir(pvStateFile), 0777)
+	if err != nil {
+		return nil, err
+	}


The same checks seem to happen in InitClientContext, so maybe we don't have to do it here?

aakoshh · 2022-08-31T15:30:38Z

app/test_helpers.go

+	if err != nil {
+		return nil, err
+	}
+	privSigner, _ := InitClientContext(client.Context{}, keyring.BackendMemory)


Can you remind me what the PV keyfile and state file are, versus an in-memory keyring? Why is one backed by files and the other by memory?

PV keyfile and state file are used in Tendermint for consensus. Please see here. We wrap PV to include BLS keys. Keyring is to store account keys. Keyring have alternative backends but we use in-memory backend only for testing.

aakoshh · 2022-08-31T15:31:34Z

app/test_helpers.go

+	privSigner, _ := InitClientContext(client.Context{}, keyring.BackendMemory)
+	privSigner.WrappedPV.Clean(pvKeyFile, pvStateFile)


Is this not going to remove something of value by accident?

I don't think it will, not in testing. This method is only used to remove the files generated during testing. Otherwise, these key/state files will be generated everywhere after running tests.

aakoshh · 2022-08-31T15:35:02Z

x/checkpointing/keeper/bls_signer_test.go

+	signer.EXPECT().GetAddress().Return(addr1)
+	signer.EXPECT().SignMsgWithBls(gomock.Eq(signBytes)).Return(bls12381.Sign(blsPrivKey1, signBytes), nil)
+	err := ckptkeeper.SendBlsSig(ctx, epochNum, lch)
+	require.NoError(t, err)


So what't the client in this test, what does it do for broadcast? Is it possible to assert that the right thing got broadcasted?

Thanks for pointing this out. I used BroadcastAsync mode in utils.go InitClientContext. See here. Basically, it does not need to wait for any confirmation of the transaction. We can switch to alternative modes to get more information about the transactions that are sent. I just thought Async mode is good for unit test. We can make it configurable for integration test.

aakoshh

This is a tough one, well done! It looks good, but I guess the only way to see if it works is to run the node and check the logs.

This reverts commit af43849.

* slashing tx validation * fix * fix * fix * fix * fix check * fix tests and checks * fix * fix checks * fix data types * fix * update e2e tests * fix comments * fix comments * fix comments * fix comments * change to slashing rate * fix comments and checks * fix checks * hardcode slashing rate to 100% to pass tests * fix docstring * more comments * add slashing rate check in checktransactions * change staker addr to change addr * pass change addr in msgcreatedelegation * change address in msgbtcundelegate * error handling for change addr * verbose errors * fix fee checks and validation for slashing tx on rounded slashing rate * more comments * fix docstring * more comments in fee checks * remove staking output info in msgundelegate * remove receiver func Validate * fix tests and include change address in MsgCreateBTCDelegation request * fix tests and pass change address in MsgBTCUndelegate * fix naming and remove dead code * fix e2e tests - pass change addr in types.MsgCreateBTCDelegation * fix imports * fix import issue * fix imports * fix imports * fix * fix args in tx * fix imports * change address in undelegation and delegation creation * keep Validate func * add more comments * fix proto comments * fix slashing rate validation * fix validation * more comments * fix checks in validation and don't depend on change address * remove change address proto * remove change address usages * fix receiver func * remove todos * fix args * fix func sigs * fix imports * fix defs * fix imports * fix formatting * change to ceil(round up) * lock time 0 * fix checks * move fee checks * fix doc string * convert to btcutil.amount * dust relay tx fee * fix dust fn * fix mod * add todo * fix docstring * changes in build logic and fix tests * rename vars * fix tests * fix slashing rate in tests to avoid change amount being too low * improve slashing rate validity and fix tests * improvise tests * fix comments * fix spacing * fix slashing rate in tests * docstrings * docstring * more comments * docstring * move btcstaking errors to btcstaking/ dir * move validation fn inside btcstaking/ dir * enforce checks if slashing/change address are same * imrpove precision check * fix slashing rate * fix tests

add bls signer

f927baa

gitferry requested review from aakoshh and vitsalis August 26, 2022 06:18

aakoshh reviewed Aug 26, 2022

View reviewed changes

gitferry added 3 commits August 29, 2022 17:15

added mocks

9cfba3b

finished unit test of sendBlsSig

f719b4f

rm unnecessary json

146e957

gitferry marked this pull request as ready for review August 29, 2022 13:34

aakoshh reviewed Aug 29, 2022

View reviewed changes

inject client context into newApp

3ba03c8

gitferry force-pushed the checkpointing/bls-signer branch from 44b9e5d to 3ba03c8 Compare August 31, 2022 01:44

Merge branch 'main' of github.com:babylonchain/babylon into checkpoin…

1c23555

…ting/bls-signer

add some TODOs

d842b71

aakoshh reviewed Aug 31, 2022

View reviewed changes

aakoshh approved these changes Aug 31, 2022

View reviewed changes

gitferry merged commit af43849 into main Sep 1, 2022

gitferry deleted the checkpointing/bls-signer branch September 1, 2022 02:09

gitferry restored the checkpointing/bls-signer branch September 1, 2022 02:10

gitferry added a commit that referenced this pull request Sep 1, 2022

Revert "feat: checkpointing/implement BLS signer (#104)"

84debe7

This reverts commit af43849.

gitferry mentioned this pull request Sep 1, 2022

chore: BLS signer refactor #110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: checkpointing/implement BLS signer #104

feat: checkpointing/implement BLS signer #104

gitferry commented Aug 26, 2022

aakoshh left a comment

aakoshh Aug 26, 2022

gitferry Aug 27, 2022

aakoshh Aug 26, 2022

gitferry Aug 28, 2022

gitferry commented Aug 29, 2022

aakoshh commented Aug 29, 2022

aakoshh left a comment

aakoshh Aug 29, 2022

gitferry Aug 31, 2022

aakoshh Aug 31, 2022 •

edited

Loading

aakoshh Aug 29, 2022

gitferry Aug 31, 2022

gitferry commented Aug 31, 2022 •

edited

Loading

aakoshh Aug 31, 2022

aakoshh Aug 31, 2022

aakoshh Aug 31, 2022

gitferry Sep 1, 2022

aakoshh Aug 31, 2022

gitferry Sep 1, 2022 •

edited

Loading

aakoshh Aug 31, 2022

gitferry Sep 1, 2022 •

edited

Loading

aakoshh left a comment

		fs := pflag.NewFlagSet("checkpointing", pflag.ContinueOnError)
		err = fs.Set(flags.FlagFrom, k.GetParams(ctx).String())

		privSigner, _ := InitClientContext(client.Context{}, keyring.BackendMemory)
		privSigner.WrappedPV.Clean(pvKeyFile, pvStateFile)

feat: checkpointing/implement BLS signer #104

feat: checkpointing/implement BLS signer #104

Conversation

gitferry commented Aug 26, 2022

aakoshh 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

gitferry commented Aug 29, 2022

aakoshh commented Aug 29, 2022

aakoshh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakoshh Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Aug 31, 2022 • 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

gitferry Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

aakoshh left a comment

Choose a reason for hiding this comment

aakoshh Aug 31, 2022 •

edited

Loading

gitferry commented Aug 31, 2022 •

edited

Loading

gitferry Sep 1, 2022 •

edited

Loading

gitferry Sep 1, 2022 •

edited

Loading