-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: checkpointing/integrate bls-sig tx into the message flow #120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You guys are awesome to finally have figured it all out!
Just minor comments, no blocker 🎉
babylond start --home ./.testnet/node0/babylond \ | ||
--keyring-backend test \ | ||
--chain-id chain-test \ | ||
--from node0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find --from
to be an intuitive name. I see it says "Name or address of private key with which to sign". Maybe call it --signing-key-id
or --from-name
(because it's passed to WithFromName
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm also surprised that Cosmos uses --from
other than --from-name
. See here. There's another flag called --name
but it's rarely used. Maybe it's better to stick with --from
just to be consistent with Cosmos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure okay, if this is expected 👍
cmd/babylond/cmd/root.go
Outdated
clientCtx := client.Context{}. | ||
WithChainID(appOpts.Get(flags.FlagChainID).(string)). | ||
WithFromName(appOpts.Get(flags.FlagFrom).(string)) | ||
privSigner, err := app.InitClientContext(clientCtx, backend) | ||
privSigner, err := app.InitPrivSigner(clientCtx, "", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comma would benefit from being put in a variable name, I have no idea what it might be doing.
x/checkpointing/keeper/bls_signer.go
Outdated
@@ -39,6 +40,10 @@ func (k Keeper) SendBlsSig(ctx sdk.Context, epochNum uint64, lch types.LastCommi | |||
|
|||
// insert the message into the transaction | |||
fs := pflag.NewFlagSet("", pflag.ContinueOnError) | |||
// TODO: hardcoded for now, will set fees as a parameter for the checkpointing module | |||
fs.String(flags.FlagFees, "", "Fees to pay along with transaction; eg: 10uatom") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.String(flags.FlagFees, "", "Fees to pay along with transaction; eg: 10uatom") | |
fs.String(flags.FlagFees, "", "Fees to pay along with transaction; eg: 10ubbn") |
cmd/babylond/cmd/root.go
Outdated
// TODO: use BackendTest of keyring for now because the current integration test does not pass FlagKeryingBackend | ||
privSigner, err := app.InitClientContext(clientCtx, keyring.BackendTest) | ||
backend := cast.ToString(appOpts.Get(flags.FlagKeyringBackend)) | ||
chainID := cast.ToString(appOpts.Get(flags.FlagChainID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so imo the problem place is here. Nodes in testnet are started without this flag and read config options from config files.
Imo this thing should not be read from flags as it is already in genesis file (which is already generated by other commands). You should somehow get access to server config and then you could use GenesisDocFromFile
functionf from "github.com/tendermint/tendermint/types"
package, to retrieve the chaind id from it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @KonradStaniec for looking into it. Yes, you are right. It is possible to get these parameters from some config files generated beforehand. After some quick search, I found that backend
can be retrieved from nodex/babylond/config/client.toml
, chainID
can be retrieved from nodex/babylond/config/genesis.json
, and fromName
can be retrieved from nodex/babylond/config/config.toml
. Let me try to access them from the code.
cbc5670
to
96522d3
Compare
…ting/bls-sig-integration
Thanks @KonradStaniec for your advice. I hardcoded these parameters to pass the integration test. I'm merging the PR, for now, to not block the other tasks. Will make another PR to access the parameters from the config file. |
This PR aims to enable the automatic generation of bls-sig transactions and aggregation. This is the last piece of the BLS signer. In particular, this PR:
babylond start
we need to additionally set--keyring-backend
,--chain-id
, and--from
. The parameters need to be consistent with the ones used when callingbabylond testnet
. For example,Now, following the instructions in readme to start a node, we should see the response of sending the bls-sig transaction at the end of each epoch. For example,
I also add @KonradStaniec as a reviewer because these additional flags are necessary for the integration tests. And thanks to @vitsalis for the help.