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

Fee recipient: checksum log #10664

Merged
merged 23 commits into from
May 11, 2022
Merged

Fee recipient: checksum log #10664

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e0e9449
adding checksum check at validator client and beacon node
james-prysm May 5, 2022
e6b4b0e
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 5, 2022
aff2a3a
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 9, 2022
c8ed60a
adding validation and logs on validator client startup
james-prysm May 9, 2022
7dfa86a
moving the log and validation
james-prysm May 9, 2022
8136bf8
fixing unit tests
james-prysm May 9, 2022
3725970
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 9, 2022
af23f2d
adding test for back checksum on validator client
james-prysm May 9, 2022
f382876
fixing bazel
james-prysm May 9, 2022
8713f69
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 9, 2022
a3f88d4
addressing comments
james-prysm May 10, 2022
c0ba991
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 10, 2022
6f11fe7
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 10, 2022
c4bf45f
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 10, 2022
7a73269
fixing log display
james-prysm May 10, 2022
b542030
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 10, 2022
ee13966
Merge branch 'develop' into fee-recipient-checksum
james-prysm May 11, 2022
b5d8579
Update beacon-chain/node/config.go
rauljordan May 11, 2022
634d54b
Apply suggestions from code review
rauljordan May 11, 2022
e056ebc
breaking up lines
james-prysm May 11, 2022
3a2b1c0
fixing unit test
james-prysm May 11, 2022
74f92f0
ugh another fix to unit test
james-prysm May 11, 2022
c15f102
Merge refs/heads/develop into fee-recipient-checksum
prylabs-bulldozer[bot] May 11, 2022
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
11 changes: 10 additions & 1 deletion beacon-chain/node/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/ethereum/go-ethereum/common"
"github.com/pkg/errors"
"github.com/prysmaticlabs/prysm/cmd"
"github.com/prysmaticlabs/prysm/cmd/beacon-chain/flags"
"github.com/prysmaticlabs/prysm/config/params"
Expand Down Expand Up @@ -117,7 +118,15 @@ func configureExecutionSetting(cliCtx *cli.Context) error {
if !common.IsHexAddress(ha) {
return fmt.Errorf("%s is not a valid fee recipient address", ha)
}
c.DefaultFeeRecipient = common.HexToAddress(ha)
mixedcaseAddress, err := common.NewMixedcaseAddressFromString(ha)
if err != nil {
return errors.Wrapf(err, "could not decode fee recipient %s", ha)
}
checksumAddress := common.HexToAddress(ha)
if !mixedcaseAddress.ValidChecksum() {
log.Warnf("fee recipient %s is not a valid checksum address. The checksummed address is %s and this will be used as the fee recipient", ha, checksumAddress.Hex())
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's consider this log from the user's perspective. Many do not know what a checksum is, so we should instead warn them that a checksummed address is safer to prevent against spelling mistakes in Ethereum addresses. The provided address was not checksummed and will be used as is when setting transaction fee recipients

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll go ahead and update this

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something about how we recommend using a mixed-case address to prevent against spelling mistakes in your fee recipient Ethereum address. The provided address is not a valid checksum (mixed-case) address. Not sure if we should link people to a specific place, but something worth thinking about

}
c.DefaultFeeRecipient = checksumAddress
params.OverrideBeaconConfig(c)
return nil
}
9 changes: 7 additions & 2 deletions beacon-chain/node/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ func TestConfigureProofOfWork(t *testing.T) {

func TestConfigureExecutionSetting(t *testing.T) {
params.SetupTestConfigCleanup(t)
hook := logTest.NewGlobal()

app := cli.App{}
set := flag.NewFlagSet("test", 0)
Expand All @@ -102,11 +103,15 @@ func TestConfigureExecutionSetting(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, common.HexToAddress("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"), params.BeaconConfig().DefaultFeeRecipient)

require.NoError(t, set.Set(flags.SuggestedFeeRecipient.Name, "0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"))
assert.LogsContain(t, hook,
"is not a valid checksum address",
)
require.NoError(t, set.Set(flags.SuggestedFeeRecipient.Name, "0xaAaAaAaaAaAaAaaAaAAAAAAAAaaaAaAaAaaAaaAa"))
cliCtx = cli.NewContext(&app, set, nil)
err = configureExecutionSetting(cliCtx)
require.NoError(t, err)
assert.Equal(t, common.HexToAddress("0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"), params.BeaconConfig().DefaultFeeRecipient)
assert.Equal(t, common.HexToAddress("0xaAaAaAaaAaAaAaaAaAAAAAAAAaaaAaAaAaaAaaAa"), params.BeaconConfig().DefaultFeeRecipient)

}

func TestConfigureNetwork(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions validator/node/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ go_test(
"//config/fieldparams:go_default_library",
"//config/validator/service:go_default_library",
"//encoding/bytesutil:go_default_library",
"//testing/assert:go_default_library",
"//testing/require:go_default_library",
"//validator/accounts:go_default_library",
"//validator/accounts/wallet:go_default_library",
Expand Down
10 changes: 9 additions & 1 deletion validator/node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,8 +539,16 @@ func feeRecipientConfig(cliCtx *cli.Context) (*validatorServiceConfig.FeeRecipie
if !common.IsHexAddress(option.FeeRecipient) {
return nil, errors.New("fee recipient is not a valid eth1 address")
}
mixedcaseAddress, err := common.NewMixedcaseAddressFromString(option.FeeRecipient)
if err != nil {
return nil, errors.Wrapf(err, "could not decode fee recipient %s", option.FeeRecipient)
}
checksumAddress := common.BytesToAddress(feebytes)
if !mixedcaseAddress.ValidChecksum() {
log.Warnf("fee recipient %s is not a valid checksum address. The checksummed address is %s and this will be used as the fee recipient", option.FeeRecipient, checksumAddress.Hex())
}
frConfig.ProposeConfig[bytesutil.ToBytes48(decodedKey)] = &validatorServiceConfig.FeeRecipientOptions{
FeeRecipient: common.BytesToAddress(feebytes),
FeeRecipient: checksumAddress,
}
}
}
Expand Down
18 changes: 14 additions & 4 deletions validator/node/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
fieldparams "github.com/prysmaticlabs/prysm/config/fieldparams"
validator_service_config "github.com/prysmaticlabs/prysm/config/validator/service"
"github.com/prysmaticlabs/prysm/encoding/bytesutil"
"github.com/prysmaticlabs/prysm/testing/assert"
"github.com/prysmaticlabs/prysm/testing/require"
"github.com/prysmaticlabs/prysm/validator/accounts"
"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
Expand Down Expand Up @@ -312,6 +313,8 @@ func TestUnmarshalFromURL(t *testing.T) {
}

func TestFeeRecipientConfig(t *testing.T) {
hook := logTest.NewGlobal()

type feeRecipientFlag struct {
dir string
url string
Expand All @@ -326,12 +329,13 @@ func TestFeeRecipientConfig(t *testing.T) {
want func() *validator_service_config.FeeRecipientConfig
urlResponse string
wantErr string
wantLog string
}{
{
name: "Happy Path Config file File",
name: "Happy Path Config file File, bad checksum",
args: args{
feeRecipientFlagValues: &feeRecipientFlag{
dir: "./testdata/good-prepare-beacon-proposer-config.json",
dir: "./testdata/good-prepare-beacon-proposer-config-badchecksum.json",
url: "",
defaultfee: "",
},
Expand All @@ -342,15 +346,16 @@ func TestFeeRecipientConfig(t *testing.T) {
return &validator_service_config.FeeRecipientConfig{
ProposeConfig: map[[fieldparams.BLSPubkeyLength]byte]*validator_service_config.FeeRecipientOptions{
bytesutil.ToBytes48(key1): {
FeeRecipient: common.HexToAddress("0x50155530FCE8a85ec7055A5F8b2bE214B3DaeFd3"),
FeeRecipient: common.HexToAddress("0xae967917c465db8578ca9024c205720b1a3651A9"),
},
},
DefaultConfig: &validator_service_config.FeeRecipientOptions{
FeeRecipient: common.HexToAddress("0x6e35733c5af9B61374A128e6F85f553aF09ff89A"),
FeeRecipient: common.HexToAddress("0xae967917c465db8578ca9024c205720b1a3651A9"),
},
}
},
wantErr: "",
wantLog: "is not a valid checksum address",
},
{
name: "Happy Path Config file File multiple fee recipients",
Expand Down Expand Up @@ -506,6 +511,11 @@ func TestFeeRecipientConfig(t *testing.T) {
require.ErrorContains(t, tt.wantErr, err)
return
}
if tt.wantLog != "" {
assert.LogsContain(t, hook,
tt.wantLog,
)
}
w := tt.want()
require.DeepEqual(t, w, got)
})
Expand Down
10 changes: 10 additions & 0 deletions validator/node/testdata/good-prepare-beacon-proposer-config-badchecksum.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"proposer_config": {
"0xa057816155ad77931185101128655c0191bd0214c201ca48ed887f6c4c6adf334070efcd75140eada5ac83a92506dd7a": {
"fee_recipient": "0xae967917c465db8578ca9024c205720b1a3651A9"
}
},
"default_config": {
"fee_recipient": "0xae967917c465db8578ca9024c205720b1a3651A9"
}
}