Skip to content

Commit

Permalink
Merge branch 'master' into robert/validator-pubkey
Browse files Browse the repository at this point in the history
  • Loading branch information
robert-zaremba committed Oct 21, 2020
2 parents 62c1fc5 + f24ad5d commit bfc93a3
Show file tree
Hide file tree
Showing 15 changed files with 454 additions and 21 deletions.
6 changes: 3 additions & 3 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
*.swl
*.swm
*.swn
.vscode
.idea
*.pyc

# private files
Expand Down Expand Up @@ -43,8 +41,10 @@ sim_log_file
vagrant

# IDE
.idea/
.idea
*.iml
.dir-locals.el
.vscode

# Graphviz
dependency-graph.png
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ For more, please go to the [Cosmos SDK Docs](./docs/).

The Cosmos Hub application, `gaia`, has moved to its [own repository](https://github.com/cosmos/gaia). Go there to join the Cosmos Hub mainnet and more.

## Scaffolding
## Starport

If you are starting a new app or a new module we provide a [scaffolding tool](https://github.com/cosmos/scaffold) to help you get started and speed up development. If you have any questions or find a bug, feel free to open an issue in the repo.
If you are starting a new app or a new module you can use [Starport](https://github.com/tendermint/starport) to help you get started and speed up development. If you have any questions or find a bug, feel free to open an issue in the repo.

## Disambiguation

Expand Down
2 changes: 1 addition & 1 deletion docs/architecture/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Please add a entry below in your Pull Request for an ADR.
- [ADR 025: IBC Passive Channels](./adr-025-ibc-passive-channels.md)
- [ADR 026: IBC Client Recovery Mechanisms](./adr-026-ibc-client-recovery-mechanisms.md)
- [ADR 027: Deterministic Protobuf Serialization](./adr-027-deterministic-protobuf-serialization.md)
- [ADR 028: Public Key Addresses](./adr-028-public-key-addresses.md)
- [ADR 029: Fee Grant Module](./adr-029-fee-grant-module.md)
- [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md)
- [ADR 032: Typed Events](./adr-032-typed-events.md)

2 changes: 1 addition & 1 deletion docs/architecture/adr-020-protobuf-transaction-encoding.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
- 2020 May 14: Describe public key encoding
- 2020 June 08: Store `TxBody` and `AuthInfo` as bytes in `SignDoc`; Document `TxRaw` as broadcast and storage type.
- 2020 August 07: Use ADR 027 for serializing `SignDoc`.
- 2020 August 19: Move sequence field from `SignDoc` to `SignerInfo`.
- 2020 August 19: Move sequence field from `SignDoc` to `SignerInfo`, as discussed in [#6966](https://github.com/cosmos/cosmos-sdk/issues/6966).
- 2020 September 25: Remove `PublicKey` type in favor of `secp256k1.PubKey`, `ed25519.PubKey` and `multisig.LegacyAminoPubKey`.
- 2020 October 15: Add `GetAccount` and `GetAccountWithHeight` methods to the `AccountRetriever` interface.

Expand Down
168 changes: 168 additions & 0 deletions docs/architecture/adr-028-public-key-addresses.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
# ADR 028: Public Key Addresses

## Changelog

- 2020/08/18: Initial version

## Status

Proposed

## Abstract

This ADR defines a canonical 20-byte address format for new public key algorithms, multisig public keys, and module
accounts using string prefixes.

## Context

Issue [\#3685](https://github.com/cosmos/cosmos-sdk/issues/3685) identified that public key
address spaces are currently overlapping. One initial proposal was extending the address length and
adding prefixes for different types of addresses.

@ethanfrey explained an alternate approach originally used in https://github.com/iov-one/weave:

> I spent quite a bit of time thinking about this issue while building weave... The other cosmos Sdk.
> Basically I define a condition to be a type and format as human readable string with some binary data appended. This condition is hashed into an Address (again at 20 bytes). The use of this prefix makes it impossible to find a preimage for a given address with a different condition (eg ed25519 vs secp256k1).
> This is explained in depth here https://weave.readthedocs.io/en/latest/design/permissions.html
> And the code is here, look mainly at the top where we process conditions. https://github.com/iov-one/weave/blob/master/conditions.go
And explained how this approach should be sufficiently collision resistant:
> Yeah, AFAIK, 20 bytes should be collision resistance when the preimages are unique and not malleable. A space of 2^160 would expect some collision to be likely around 2^80 elements (birthday paradox). And if you want to find a collision for some existing element in the database, it is still 2^160. 2^80 only is if all these elements are written to state.
> The good example you brought up was eg. a public key bytes being a valid public key on two algorithms supported by the codec. Meaning if either was broken, you would break accounts even if they were secured with the safer variant. This is only as the issue when no differentiating type info is present in the preimage (before hashing into an address).
> I would like to hear an argument if the 20 bytes space is an actual issue for security, as I would be happy to increase my address sizes in weave. I just figured cosmos and ethereum and bitcoin all use 20 bytes, it should be good enough. And the arguments above which made me feel it was secure. But I have not done a deeper analysis.
In discussions in [\#5694](https://github.com/cosmos/cosmos-sdk/issues/5694), we agreed to go with an
approach similar to this where essentially we take the first 20 bytes of the `sha256` hash of
the key type concatenated with the key bytes, summarized as `Sha256(KeyTypePrefix || Keybytes)[:20]`.

## Decision

### Legacy Public Key Addresses Don't Change

`secp256k1` and multisig public keys are currently in use in existing Cosmos SDK zones. They use the following
address formats:

- secp256k1: `ripemd160(sha256(pk_bytes))[:20]`
- legacy amino multisig: `sha256(aminoCdc.Marshal(pk))[:20]`

We don't want to change existing addresses. So the addresses for these two key types will remain the same.

The current multisig public keys use amino serialization to generate the address. We will retain
those public keys and their address formatting, and call them "legacy amino" multisig public keys
in protobuf. We will also create multisig public keys without amino addresses to be described below.


### Canonical Address Format

We have three types of accounts we would like to create addresses for in the future:
- regular public key addresses for new signature algorithms (ex. `sr25519`).
- public key addresses for multisig public keys that don't use amino encoding
- module accounts: basically any accounts which cannot sign transactions and
which are managed internally by modules

To address all of these use cases we propose the following basic `AddressHash` function,
based on the discussions in [\#5694](https://github.com/cosmos/cosmos-sdk/issues/5694):

```go
func AddressHash(prefix string, contents []byte) []byte {
preImage := []byte(prefix)
if len(contents) != 0 {
preImage = append(preImage, 0)
preImage = append(preImage, contents...)
}
return sha256.Sum256(preImage)[:20]
}
```

`AddressHash` always take a string `prefix` as a starting point which should represent the
type of public key (ex. `sr25519`) or module account being used (ex. `staking` or `group`).
For public keys, the `contents` parameter is used to specify the binary contents of the public
key. For module accounts, `contents` can be left empty (for modules which don't manage "sub-accounts"),
or can be some module-specific content to specify different pools (ex. `bonded` or `not-bonded` for `staking`)
or managed accounts (ex. different accounts managed by the `group` module).

In the `preImage`, the byte value `0` is used as the separator between `prefix` and `contents`. This is a logical
choice given that `0` is an invalid value for a string character and is commonly used as a null terminator.

### Canonical Public Key Address Prefixes

All public key types will have a unique protobuf message type such as:

```proto
package cosmos.crypto.sr25519;
message PubKey {
bytes key = 1;
}
```

All protobuf messages have unique fully qualified names, in this example `cosmos.crypto.sr25519.PubKey`.
These names are derived directly from .proto files in a standardized way and used
in other places such as the type URL in `Any`s. Since there is an easy and obvious
way to get this name for every protobuf type, we can use this message name as the
key type `prefix` when creating addresses. For all basic public keys, `contents`
should just be the raw unencoded public key bytes.

Thus the canonical address for new public key types would be `AddressHash(proto.MessageName(pk), pk.Bytes)`.

### Multisig Addresses

For new multisig public keys, we define a custom address format not based on any encoding scheme
(amino or protobuf). This avoids issues with non-determinism in the encoding scheme. It also
ensures that multisig public keys which differ simply in the ordering of keys have the same
address by sorting child public keys first.

First we define a proto message for multisig public keys:
```proto
package cosmos.crypto.multisig;
message PubKey {
uint32 threshold = 1;
repeated google.protobuf.Any public_keys = 2;
}
```

We define the following `Address()` function for this public key:

```
func (multisig PubKey) Address() {
// first gather all the addresses of each nested public key
var addresses [][]byte
for key := range multisig.Keys {
addresses = append(joinedAddresses, key.Address())
}
// then sort them in ascending order
addresses = Sort(addresses)
// then concatenate them together
var joinedAddresses []byte
for addr := range addresses {
joinedAddresses := append(joinedAddresses, addr...)
}
// form the string prefix from the message name (cosmos.crypto.multisig.PubKey) and the threshold joined together
prefix := fmt.Sprintf("%s/%d", proto.MessageName(multisig), multisig.Threshold)
// use the standard AddressHash function
return AddressHash(prefix, joinedAddresses)
}
```

## Consequences

### Positive
- a simple algorithm for generating addresses for new public keys and module accounts

### Negative
- addresses do not communicate key type, a prefixed approach would have done this

### Neutral
- protobuf message names are used as key type prefixes

## References
9 changes: 7 additions & 2 deletions server/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ func interceptConfigs(rootViper *viper.Viper) (*tmcfg.Config, error) {

conf := tmcfg.DefaultConfig()

if _, err := os.Stat(configFile); os.IsNotExist(err) {
switch _, err := os.Stat(configFile); {
case os.IsNotExist(err):
tmcfg.EnsureRoot(rootDir)

if err = conf.ValidateBasic(); err != nil {
Expand All @@ -158,7 +159,11 @@ func interceptConfigs(rootViper *viper.Viper) (*tmcfg.Config, error) {
conf.P2P.SendRate = 5120000
conf.Consensus.TimeoutCommit = 5 * time.Second
tmcfg.WriteConfigFile(configFile, conf)
} else {

case err != nil:
return nil, err

default:
rootViper.SetConfigType("toml")
rootViper.SetConfigName("config")
rootViper.AddConfigPath(configPath)
Expand Down
24 changes: 24 additions & 0 deletions server/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"path"
"path/filepath"
"strings"
"testing"

Expand Down Expand Up @@ -444,3 +445,26 @@ func TestInterceptConfigsPreRunHandlerPrecedenceConfigDefault(t *testing.T) {
t.Error("RPCListenAddress is not using default")
}
}

// Ensure that if interceptConfigs encounters any error other than non-existen errors
// that we correctly return the offending error, for example a permission error.
// See https://github.com/cosmos/cosmos-sdk/issues/7578
func TestInterceptConfigsWithBadPermissions(t *testing.T) {
tempDir := t.TempDir()
subDir := filepath.Join(tempDir, "nonPerms")
if err := os.Mkdir(subDir, 0600); err != nil {
t.Fatalf("Failed to create sub directory: %v", err)
}
cmd := StartCmd(nil, "/foobar")
if err := cmd.Flags().Set(flags.FlagHome, subDir); err != nil {
t.Fatalf("Could not set home flag [%T] %v", err, err)
}

cmd.PreRunE = preRunETestImpl

serverCtx := &Context{}
ctx := context.WithValue(context.Background(), ServerContextKey, serverCtx)
if err := cmd.ExecuteContext(ctx); !os.IsPermission(err) {
t.Fatalf("Failed to catch permissions error, got: [%T] %v", err, err)
}
}
66 changes: 66 additions & 0 deletions x/distribution/legacy/v036/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@
package v036

import (
"fmt"
"strings"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
v034distr "github.com/cosmos/cosmos-sdk/x/distribution/legacy/v034"
"github.com/cosmos/cosmos-sdk/x/distribution/types"
v036gov "github.com/cosmos/cosmos-sdk/x/gov/legacy/v036"
)

// ----------------------------------------------------------------------------
Expand All @@ -13,6 +19,12 @@ import (

const (
ModuleName = "distribution"

// RouterKey is the message route for distribution
RouterKey = ModuleName

// ProposalTypeCommunityPoolSpend defines the type for a CommunityPoolSpendProposal
ProposalTypeCommunityPoolSpend = "CommunityPoolSpend"
)

type (
Expand Down Expand Up @@ -40,6 +52,14 @@ type (
DelegatorStartingInfos []v034distr.DelegatorStartingInfoRecord `json:"delegator_starting_infos"`
ValidatorSlashEvents []ValidatorSlashEventRecord `json:"validator_slash_events"`
}

// CommunityPoolSpendProposal spends from the community pool
CommunityPoolSpendProposal struct {
Title string `json:"title" yaml:"title"`
Description string `json:"description" yaml:"description"`
Recipient sdk.AccAddress `json:"recipient" yaml:"recipient"`
Amount sdk.Coins `json:"amount" yaml:"amount"`
}
)

func NewGenesisState(
Expand All @@ -66,3 +86,49 @@ func NewGenesisState(
ValidatorSlashEvents: slashes,
}
}

var _ v036gov.Content = CommunityPoolSpendProposal{}

// GetTitle returns the title of a community pool spend proposal.
func (csp CommunityPoolSpendProposal) GetTitle() string { return csp.Title }

// GetDescription returns the description of a community pool spend proposal.
func (csp CommunityPoolSpendProposal) GetDescription() string { return csp.Description }

// GetDescription returns the routing key of a community pool spend proposal.
func (csp CommunityPoolSpendProposal) ProposalRoute() string { return RouterKey }

// ProposalType returns the type of a community pool spend proposal.
func (csp CommunityPoolSpendProposal) ProposalType() string { return ProposalTypeCommunityPoolSpend }

// ValidateBasic runs basic stateless validity checks
func (csp CommunityPoolSpendProposal) ValidateBasic() error {
err := v036gov.ValidateAbstract(csp)
if err != nil {
return err
}
if !csp.Amount.IsValid() {
return types.ErrInvalidProposalAmount
}
if csp.Recipient.Empty() {
return types.ErrEmptyProposalRecipient
}

return nil
}

// String implements the Stringer interface.
func (csp CommunityPoolSpendProposal) String() string {
var b strings.Builder
b.WriteString(fmt.Sprintf(`Community Pool Spend Proposal:
Title: %s
Description: %s
Recipient: %s
Amount: %s
`, csp.Title, csp.Description, csp.Recipient, csp.Amount))
return b.String()
}

func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(CommunityPoolSpendProposal{}, "cosmos-sdk/CommunityPoolSpendProposal", nil)
}
1 change: 1 addition & 0 deletions x/genutil/legacy/v036/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ func Migrate(appState types.AppMap, _ client.Context) types.AppMap {
v036Codec := codec.NewLegacyAmino()
cryptocodec.RegisterCrypto(v036Codec)
v036gov.RegisterLegacyAminoCodec(v036Codec)
v036distr.RegisterLegacyAminoCodec(v036Codec)

// migrate genesis accounts state
if appState[v034genAccounts.ModuleName] != nil {
Expand Down
Loading

0 comments on commit bfc93a3

Please sign in to comment.