aergoio / aergo

aergo blockchain kernel
http://docs.aergo.io
MIT License
214 stars 44 forks source link

config: use relative path for npkey #288

Closed kroggen closed 11 months ago

kroggen commented 11 months ago

In the [p2p] section of the config.toml the npkey appears to be using only full paths

This PR changes the code to work like this:

If there is no "/" and no "." at the beginning of npkey (no full path specified) then prepend the path of the auth folder to it

So a config like this:

npkey = "bp01.key"

Expects the bp01.key to be inside the auth folder

I don't know if this is the best approach

Please leave your opinion

kroggen commented 11 months ago

There is no risk of hardfork here, but the change can affect existing nodes

@kslee8282 Please check if compatible

I also don't know if the auth folder is the best place, or if the path should use the "aergo home" as base

kslee8282 commented 11 months ago

Let's check if the Docker instance starts up the same way after building topic/config-npkey-path.

kslee8282 commented 11 months ago

This function does not work in the existing Docker environment.

Oct 13 01:42:44.505 INF ../go/aergo/cmd/aergosvr/aergosvr.go:100 > AERGO SVR STARTED branch=topic/config-npkey-path module=asvr revision=750c2b53 Oct 13 01:42:44.505 INF ../go/aergo/cmd/aergosvr/aergosvr.go:103 > Enable Profiling on localhost: 6060 module=asvr panic: Failed to load Keyfile 'bmt1.key' Invalid keyfile path '/aergo/auth/bmt1.key'. Check the key file exists.

goroutine 1 [running]: github.com/aergoio/aergo/v2/p2p/p2pkey.InitNodeInfo(0xc000165a20, 0xc0001538c0, {0x16654b0, 0x18}, 0xc00014dce0) /go/aergo/p2p/p2pkey/nodekey.go:55 +0x607 main.rootRun(0x1ea99e0?, {0x136fbee?, 0x4?, 0x4?}) /go/aergo/cmd/aergosvr/aergosvr.go:114 +0x1b1 github.com/spf13/cobra.(Command).execute(0x1ea99e0, {0xc00012c010, 0x4, 0x4}) /go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:830 +0x663 github.com/spf13/cobra.(Command).ExecuteC(0x1ea99e0) /go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:914 +0x2ef github.com/spf13/cobra.(*Command).Execute(...) /go/pkg/mod/github.com/spf13/cobra@v0.0.5/command.go:864 main.main() /go/aergo/cmd/aergosvr/aergosvr.go:35 +0x25

kslee8282 commented 11 months ago

Perhaps this feature will have a negative impact on node installation in AEM.( Aergo Enterprise Manager)

kroggen commented 11 months ago

In which folder is the key located on the docker container? When using without docker the aergosvr does not find the file I put it in home, I put it in home/auth, and the key file is not found

kroggen commented 11 months ago

I found that it is searching for the file on the current working directory

One solution to this is to change the CWD to the HomeDir on initialization, like this:

err := os.Chdir(homePath)
if err != nil {
    panic(err)
}

Or try the auth or home path only on failure:

    if p2pCfg.NPKey != "" {
        priv, pub, err = p2putil.LoadKeyFile(p2pCfg.NPKey)
        if err != nil && len(p2pCfg.NPKey) > 0 && p2pCfg.NPKey[0] != '/' && p2pCfg.NPKey[0] != '.' {
            NPKeyFilePath := filepath.Join(baseCfg.AuthDir, NPKeyFilePath)
            priv, pub, err = p2putil.LoadKeyFile(NPKeyFilePath)
        }
        if err != nil {
            panic("Failed to load Keyfile '" + p2pCfg.NPKey + "' " + err.Error())
        }

@gokch @hayarobi What do you think? Which option would be better?

I also do not know how to access the homePath from another package

hayarobi commented 11 months ago

Most of command line tool treat cwd as base of relative path. Think of tools such as ls,find,grep or others. The aergosvr just follows that convention. @kroggen After reading your idea, I also have an idea, which is similar with shell; A leading tilde(~) is expanded to user's home directory. e.g) ~/auth/key1.key is expanded such like /Users/hayarobipark/auth/key1.key.

kroggen commented 11 months ago

But I am referring to the "aergo home" directory, not the user's home.

Because in the config.toml the data folder, auth folder, mempool dump are all by default in the "aergo home", so I was expecting that the files we put there in the config without a full path would also be expected to be in the "aergo home"

hayarobi commented 11 months ago

But I am referring to the "aergo home" directory, not the user's home.

Because in the config.toml the data folder, auth folder, mempool dump are all by default in the "aergo home", so I was expecting that the files we put there in the config without a full path would also be expected to be in the "aergo home"

I agree with you at some extents. There are a few of config fields that indicate file path such as npcert or dumpfilepath, and there are all based on cwd now. We need to keep sementic consistence of these fields. And I think it's better to preserve backward compatibility at least before the hardfork v4 or aergo release v3.0.

kroggen commented 11 months ago

OK, I will close this for now as I was able to manage it for the integration tests using aergosvr directly

Later the code can be updated to search on the "aergo home" if the file was not found on the CWD