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.

Sign up for GitHub

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

R4R: Bech32 Empty Addresses #2601

Merged
merged 7 commits into from
Feb 8, 2019
Merged

R4R: Bech32 Empty Addresses #2601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2720883
empty addresses
sunnya97 Oct 26, 2018
bdce019
PENDING
sunnya97 Oct 27, 2018
cef9ea2
Merge branch 'develop' into sunny/bech32_empty_address
jackzampolin Nov 15, 2018
2039080
fixed addr len
sunnya97 Nov 16, 2018
0090817
asdf
sunnya97 Nov 16, 2018
0f92f6a
merged develop
sunnya97 Feb 8, 2019
7cdcc7a
works?
sunnya97 Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ BREAKING CHANGES
- Remove Get prefix from all TxBuilder's getters.
* [\#3522](https://github.com/cosmos/cosmos-sdk/pull/3522) Get rid of double negatives: Coins.IsNotNegative() -> Coins.IsAnyNegative().


* Tendermint


Expand All @@ -55,6 +56,7 @@ FEATURES
* [\#3477][distribution] new query endpoint "delegator_validators"
* [\#3514](https://github.com/cosmos/cosmos-sdk/pull/3514) Provided a lazy loading implementation of Keybase that locks the underlying
storage only for the time needed to perform the required operation. Also added Keybase reference to TxBuilder struct.
* [types] [\#2580](https://github.com/cosmos/cosmos-sdk/issues/2580) Addresses now Bech32 empty addresses to an empty string

* Tendermint

Expand Down
43 changes: 43 additions & 0 deletions types/address.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/json"
"errors"
"fmt"
"strings"

"github.com/tendermint/tendermint/crypto"
"github.com/tendermint/tendermint/crypto/encoding/amino"
Expand Down Expand Up @@ -71,12 +72,21 @@ func AccAddressFromHex(address string) (addr AccAddress, err error) {

// AccAddressFromBech32 creates an AccAddress from a Bech32 string.
func AccAddressFromBech32(address string) (addr AccAddress, err error) {
if len(strings.TrimSpace(address)) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to do this direction too (decoding empty strings as valid bech32-addresses)? That seems like it could lead to UX confusion or griefing (tricking users into sending coins to empty addresses, for example).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. @sunnya97 what's the new empty address in bech32 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That should be caught by the ValidateBasic of the Msgs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like its reasonable to make all msg types ensure that the address is tmhash.size digest in their validate basics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really, that seems kinda annoying, why not ensure it at the type level instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets make a new issue for this though, its basically entirely independent of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite follow - I'm concerned about this particular addition, which will accept "" (the empty string) as a valid bech32-address. Previously that wasn't the case, and any address when de-serialized in GetFromBech32 would be automatically checked for the correct length and checksum. Addresses can still be []byte, that's not my concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the more underlying problem is that check tx may pass a tx with invalid address. That should ideally be caught asap. Then any built tx would fail in it's check tx phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not just do the length check in these function calls (TM address length)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should return an error for invalid addresses here, rather than returning an invalid AccAddress{}.
For simplicity we should use a single global constant tmhash.TruncatedSize (definitely not 32 bytes, but 20).
And if the length is invalid, this function should return an error immediately.

return AccAddress{}, nil
}

bech32PrefixAccAddr := GetConfig().GetBech32AccountAddrPrefix()

bz, err := GetFromBech32(address, bech32PrefixAccAddr)
if err != nil {
return nil, err
}

if len(bz) != AddrLen {
return nil, errors.New("Incorrect address length")
}

return AccAddress(bz), nil
}

Expand Down Expand Up @@ -141,7 +151,12 @@ func (aa AccAddress) Bytes() []byte {

// String implements the Stringer interface.
func (aa AccAddress) String() string {
if aa.Empty() {
return ""
}

bech32PrefixAccAddr := GetConfig().GetBech32AccountAddrPrefix()

bech32Addr, err := bech32.ConvertAndEncode(bech32PrefixAccAddr, aa.Bytes())
if err != nil {
panic(err)
Expand Down Expand Up @@ -187,12 +202,21 @@ func ValAddressFromHex(address string) (addr ValAddress, err error) {

// ValAddressFromBech32 creates a ValAddress from a Bech32 string.
func ValAddressFromBech32(address string) (addr ValAddress, err error) {
if len(strings.TrimSpace(address)) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

return ValAddress{}, nil
}

bech32PrefixValAddr := GetConfig().GetBech32ValidatorAddrPrefix()

bz, err := GetFromBech32(address, bech32PrefixValAddr)
if err != nil {
return nil, err
}

if len(bz) != AddrLen {
return nil, errors.New("Incorrect address length")
Copy link
Member

Choose a reason for hiding this comment

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

can this common error live somewhere?

}

return ValAddress(bz), nil
}

Expand Down Expand Up @@ -258,7 +282,12 @@ func (va ValAddress) Bytes() []byte {

// String implements the Stringer interface.
func (va ValAddress) String() string {
if va.Empty() {
return ""
}

bech32PrefixValAddr := GetConfig().GetBech32ValidatorAddrPrefix()

bech32Addr, err := bech32.ConvertAndEncode(bech32PrefixValAddr, va.Bytes())
if err != nil {
panic(err)
Expand Down Expand Up @@ -304,12 +333,21 @@ func ConsAddressFromHex(address string) (addr ConsAddress, err error) {

// ConsAddressFromBech32 creates a ConsAddress from a Bech32 string.
func ConsAddressFromBech32(address string) (addr ConsAddress, err error) {
if len(strings.TrimSpace(address)) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

return ConsAddress{}, nil
}

bech32PrefixConsAddr := GetConfig().GetBech32ConsensusAddrPrefix()

bz, err := GetFromBech32(address, bech32PrefixConsAddr)
if err != nil {
return nil, err
}

if len(bz) != AddrLen {
return nil, errors.New("Incorrect address length")
}

return ConsAddress(bz), nil
}

Expand Down Expand Up @@ -380,7 +418,12 @@ func (ca ConsAddress) Bytes() []byte {

// String implements the Stringer interface.
func (ca ConsAddress) String() string {
if ca.Empty() {
return ""
}

bech32PrefixConsAddr := GetConfig().GetBech32ConsensusAddrPrefix()

bech32Addr, err := bech32.ConvertAndEncode(bech32PrefixConsAddr, ca.Bytes())
if err != nil {
panic(err)
Expand Down
21 changes: 19 additions & 2 deletions types/address_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
)

var invalidStrs = []string{
"",
"hello, world!",
"0xAA",
"AAA",
Expand All @@ -35,6 +34,24 @@ func testMarshal(t *testing.T, original interface{}, res interface{}, marshal fu
require.Equal(t, original, res)
}

func TestEmptyAddresses(t *testing.T) {
require.Equal(t, (types.AccAddress{}).String(), "")
require.Equal(t, (types.ValAddress{}).String(), "")
require.Equal(t, (types.ConsAddress{}).String(), "")

accAddr, err := types.AccAddressFromBech32("")
require.True(t, accAddr.Empty())
require.Nil(t, err)

valAddr, err := types.ValAddressFromBech32("")
require.True(t, valAddr.Empty())
require.Nil(t, err)

consAddr, err := types.ConsAddressFromBech32("")
require.True(t, consAddr.Empty())
require.Nil(t, err)
}

func TestRandBech32PubkeyConsistency(t *testing.T) {
var pub ed25519.PubKeyEd25519

Expand Down Expand Up @@ -202,7 +219,7 @@ func TestConfiguredPrefix(t *testing.T) {
config := types.GetConfig()
config.SetBech32PrefixForAccount(prefix+"acc", prefix+"pub")
acc := types.AccAddress(pub.Address())
require.True(t, strings.HasPrefix(acc.String(), prefix+"acc"))
require.True(t, strings.HasPrefix(acc.String(), prefix+"acc"), acc.String())
bech32Pub := types.MustBech32ifyAccPub(pub)
require.True(t, strings.HasPrefix(bech32Pub, prefix+"pub"))

Expand Down
10 changes: 5 additions & 5 deletions x/distribution/types/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ func (msg MsgSetWithdrawAddress) GetSignBytes() []byte {

// quick validity check
func (msg MsgSetWithdrawAddress) ValidateBasic() sdk.Error {
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.WithdrawAddr == nil {
if msg.WithdrawAddr.Empty() {
return ErrNilWithdrawAddr(DefaultCodespace)
}
return nil
Expand Down Expand Up @@ -78,10 +78,10 @@ func (msg MsgWithdrawDelegatorReward) GetSignBytes() []byte {

// quick validity check
func (msg MsgWithdrawDelegatorReward) ValidateBasic() sdk.Error {
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
return nil
Expand Down Expand Up @@ -114,7 +114,7 @@ func (msg MsgWithdrawValidatorCommission) GetSignBytes() []byte {

// quick validity check
func (msg MsgWithdrawValidatorCommission) ValidateBasic() sdk.Error {
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
return nil
Expand Down
4 changes: 2 additions & 2 deletions x/gov/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (msg MsgSubmitProposal) ValidateBasic() sdk.Error {
if !validProposalType(msg.ProposalType) {
return ErrInvalidProposalType(DefaultCodespace, msg.ProposalType)
}
if len(msg.Proposer) == 0 {
if msg.Proposer.Empty() {
return sdk.ErrInvalidAddress(msg.Proposer.String())
}
if !msg.InitialDeposit.IsValid() {
Expand Down Expand Up @@ -172,7 +172,7 @@ func (msg MsgVote) Type() string { return TypeMsgVote }

// Implements Msg.
func (msg MsgVote) ValidateBasic() sdk.Error {
if len(msg.Voter.Bytes()) == 0 {
if msg.Voter.Empty() {
return sdk.ErrInvalidAddress(msg.Voter.String())
}
if msg.ProposalID < 0 {
Expand Down
2 changes: 1 addition & 1 deletion x/slashing/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (msg MsgUnjail) GetSignBytes() []byte {

// quick validity check
func (msg MsgUnjail) ValidateBasic() sdk.Error {
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrBadValidatorAddr(DefaultCodespace)
}
return nil
Expand Down
8 changes: 4 additions & 4 deletions x/slashing/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ const (

// The Double Sign Jail period ends at Max Time supported by Amino (Dec 31, 9999 - 23:59:59 GMT)
var (
DoubleSignJailEndTime = time.Unix(253402300799, 0)
DefaultMinSignedPerWindow sdk.Dec = sdk.NewDecWithPrec(5, 1)
DefaultSlashFractionDoubleSign sdk.Dec = sdk.NewDec(1).Quo(sdk.NewDec(20))
DefaultSlashFractionDowntime sdk.Dec = sdk.NewDec(1).Quo(sdk.NewDec(100))
DoubleSignJailEndTime = time.Unix(253402300799, 0)
DefaultMinSignedPerWindow = sdk.NewDecWithPrec(5, 1)
DefaultSlashFractionDoubleSign = sdk.NewDec(1).Quo(sdk.NewDec(20))
DefaultSlashFractionDowntime = sdk.NewDec(1).Quo(sdk.NewDec(100))
)

// Parameter store keys
Expand Down
4 changes: 2 additions & 2 deletions x/staking/keeper/query_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ func (k Keeper) GetAllRedelegations(ctx sdk.Context, delegator sdk.AccAddress, s
iterator := sdk.KVStorePrefixIterator(store, delegatorPrefixKey) // smallest to largest
defer iterator.Close()

srcValFilter := !(srcValAddress.Empty() || srcValAddress == nil)
dstValFilter := !(dstValAddress.Empty() || dstValAddress == nil)
srcValFilter := !(srcValAddress.Empty())
dstValFilter := !(dstValAddress.Empty())

for ; iterator.Valid(); iterator.Next() {
redelegation := types.MustUnmarshalRED(k.cdc, iterator.Value())
Expand Down
20 changes: 10 additions & 10 deletions x/staking/types/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,10 @@ func (msg MsgCreateValidator) GetSignBytes() []byte {
// quick validity check
func (msg MsgCreateValidator) ValidateBasic() sdk.Error {
// note that unmarshaling from bech32 ensures either empty or valid
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
if msg.Value.Amount.LTE(sdk.ZeroInt()) {
Expand Down Expand Up @@ -174,7 +174,7 @@ func (msg MsgEditValidator) GetSignBytes() []byte {

// quick validity check
func (msg MsgEditValidator) ValidateBasic() sdk.Error {
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return sdk.NewError(DefaultCodespace, CodeInvalidInput, "nil validator address")
}

Expand Down Expand Up @@ -222,10 +222,10 @@ func (msg MsgDelegate) GetSignBytes() []byte {

// quick validity check
func (msg MsgDelegate) ValidateBasic() sdk.Error {
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
if msg.Value.Amount.LTE(sdk.ZeroInt()) {
Expand Down Expand Up @@ -270,13 +270,13 @@ func (msg MsgBeginRedelegate) GetSignBytes() []byte {

// quick validity check
func (msg MsgBeginRedelegate) ValidateBasic() sdk.Error {
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.ValidatorSrcAddr == nil {
if msg.ValidatorSrcAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
if msg.ValidatorDstAddr == nil {
if msg.ValidatorDstAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
if msg.SharesAmount.LTE(sdk.ZeroDec()) {
Expand Down Expand Up @@ -313,10 +313,10 @@ func (msg MsgUndelegate) GetSignBytes() []byte {

// quick validity check
func (msg MsgUndelegate) ValidateBasic() sdk.Error {
if msg.DelegatorAddr == nil {
if msg.DelegatorAddr.Empty() {
return ErrNilDelegatorAddr(DefaultCodespace)
}
if msg.ValidatorAddr == nil {
if msg.ValidatorAddr.Empty() {
return ErrNilValidatorAddr(DefaultCodespace)
}
if msg.SharesAmount.LTE(sdk.ZeroDec()) {
Expand Down