Skip to content
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

fix: Use the account specified in app.toml to sign BLS transactions #348

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

vitsalis
Copy link
Member

@vitsalis vitsalis commented Apr 13, 2023

Couple of facets on this bug:

  1. The account specified in app.toml was not actually getting used, but instead always the account specified as the delegator in priv_validator_key.json is getting used. This is evident from the clientCtx being built with a "From Address" being the key specified in that file.
  2. Up to this point, the signer of the AddBlsSig message (i.e. the one that sent the transaction), was always the validator address (see GetSigners for this message type). When doing the fix for (1), there was a nasty bug in which it always expected 2 signatures (with one being from the delegator address). This is resolved by introducing a signer attribute to the AddBlsSig message, as we do with other messages. The signer is the one that submits the transaction, not the one to which the BLS key corresponds to. For example, it should be perfectly possible that I sent my BLS signature to someone else and they submit it for me.
  3. We want the BBN node to be functional for people that do not have a keyring or are not validators. To that end, given that we currently use the key specified in app.toml to initialize the priv signer, I changed the default value to the empty string and added checks so that it doesn't get used in the typical case. However, if you're a validator and you have specified a key that does not exist, your application will panic. We need to be careful with this and add some sanity checks in our docs for our validators so that they don't encounter this scenario.

Also, make proto-gen for some reason produced all kinds of different changes on the files.

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@@ -56,12 +59,14 @@ func (m *MsgAddBlsSig) ValidateBasic() error {
}

func (m *MsgAddBlsSig) GetSigners() []sdk.AccAddress {
signer, err := sdk.ValAddressFromBech32(m.BlsSig.SignerAddress)
signer, err := sdk.AccAddressFromBech32(m.Signer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a nice catch 🙈 👍

proto/babylon/checkpointing/v1/tx.proto Show resolved Hide resolved
x/checkpointing/keeper/bls_signer.go Outdated Show resolved Hide resolved
@KonradStaniec
Copy link
Collaborator

So point 1 seems ok.

Point 2 makes me a little nervous, as it changes the assumption that the one who submitted the BLS signature transaction is validator himself. And this makes a bit sense as actually our bls sig is as a vote in standard pos protocol.
Are we sure this assumption is not used anywhere for any security properties/penalties? (or planned to be used in the future)

Point 3 seems as seem something to work on in future. I was thinking of adding some flag to node like --validator which would signal to node that it is running in validator context, and it could do some more checks on startup. Then we could even ignore private signer when node is not running in validator node.

@vitsalis
Copy link
Member Author

vitsalis commented Apr 14, 2023

So point 1 seems ok.

Point 2 makes me a little nervous, as it changes the assumption that the one who submitted the BLS signature transaction is validator himself. And this makes a bit sense as actually our bls sig is as a vote in standard pos protocol. Are we sure this assumption is not used anywhere for any security properties/penalties? (or planned to be used in the future)

We don't have this assumption up to this point. We have added a configuration value in app.toml to specify the key that is going to be used to submit the BLS signature just for this reason. Basically the reasoning is that only the validator can create the BLS signature, however the BLS signature can be wrapped inside a transaction and submitted by any individual. So what happened in (2) was a bug, as otherwise there would be no reason for us to add a configuration value to specify the key that is going to be used, it should always be the address of the validator.

However, I understand the concern that this might break something somewhere else. In theory it shouldn't but I'm not an expert of the checkpointing module. Maybe we can merge this and ask @gitferry to take a look when he gets back? However, judging by the handler of the AddBlsSig message, it seems that the signer of the transaction is only used to pay for the fees, while the structure inside the Bls Signature is the one that is being used for deciding on the validity of the bls signature.

In my opinion, we should allow other addresses to submit the BLS signature as we have the requirement that the keyring that submits the transaction is a test keyring and accessible. It would not be secure for our validators to have all of their self-delegated funds in a test keyring stored in the same machine as their validator which is also used for automatically submitting transactions. If I was a validator, I would create a password protected keyring to create my validator and would also have a separate test keyring for submitting BLS signature transactions.

Point 3 seems as seem something to work on in future. I was thinking of adding some flag to node like --validator which would signal to node that it is running in validator context, and it could do some more checks on startup. Then we could even ignore private signer when node is not running in validator node.

Agreed, this might be an interesting approach. Although I don't very much like the node explicitly knowing if it's a validator or not, but this might help us avoid some complexity.

@vitsalis vitsalis merged commit 3388e0f into dev Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants