status-go icon indicating copy to clipboard operation
status-go copied to clipboard

NodeConfig, make sure that absolute paths are saved

Open friofry opened this issue 1 year ago • 1 comments

Problem

in some places DataDir is expected to be relative path

 loadNodeConfig:
    conf.RootDataDir = b.rootDataDir
    conf.DataDir = filepath.Join(b.rootDataDir, conf.DataDir)
    conf.KeyStoreDir = filepath.Join(b.rootDataDir, conf.KeyStoreDir)

or

func defaultNodeConfig(installationID string) (*params.NodeConfig, error) {
...
	nodeConfig.DataDir = api.DefaultDataDir
}

and other palces expect absolute paths:

NewNodeConfig:
    config := &NodeConfig{
        NetworkID:              networkID,
        RootDataDir:            dataDir,
        DataDir:                dataDir,

We need to make sure that all paths in NodeConfig are stored in absolute form (or if that's not possible, relative, but it should be consistent). And the code that concatenates absolute paths should be fixed conf.DataDir = filepath.Join(b.rootDataDir, conf.DataDir)

(it was found while I was debugging tests in https://github.com/status-im/status-go/pull/5527)

Implementation

DataDir, KeyStoreDir, RootDataDir, LogDir, LogFile should be absolute

Functions like getDefaultDataDir should be used correctly. And names of fields and methods should reflect the path type (perhaps it makes sense to introduce a wrapper type for FilePath to support both forms).

Acceptance Criteria

See status-im/status-react#9942

[status-im/status-react#9942] Upgradable paths in configs

Storing absolute path for different configs breaks compatibility on iOS
as app's dir is changed after upgrade. The solution is to store relative
paths and to concatenate it with `backend.rootDataDir`. The only
exception is `LogFile` as it is stored outside `backend.rootDataDir` on
Android. `LogDir` config was added to allow adding of custom dir for log
file.
Configs concerned:
`DataDir`
`LogDir`
`LogFile`
`KeystoreDir`
`BackupDisabledDataDir`

We need to make sure that mobile handles this correctly.

friofry avatar Jul 23 '24 13:07 friofry

[status-im/status-react#9942] Upgradable paths in configs

Storing absolute path for different configs breaks compatibility on iOS
as app's dir is changed after upgrade. The solution is to store relative
paths and to concatenate it with `backend.rootDataDir`. The only
exception is `LogFile` as it is stored outside `backend.rootDataDir` on
Android. `LogDir` config was added to allow adding of custom dir for log
file.
Configs concerned:
`DataDir`
`LogDir`
`LogFile`
`KeystoreDir`
`BackupDisabledDataDir`

thanks for the info, so I think our only solution would be make sure that all paths in NodeConfig are stored in relative path? and this was also my old understanding, but I didn't expect it was not consistent 🙂 @friofry

qfrank avatar Jul 24 '24 09:07 qfrank

I stumbled upon the same issue today. NodeConfig handling is far from ideal. I came up with an ad-hoc solution for my problem, which I believe will work for this issue as well: store only relative paths and add methods that return concatenated paths. Example from my changes:

diff --git a/api/geth_backend.go b/api/geth_backend.go
index b0580a0de..6873ad77d 100644
--- a/api/geth_backend.go
+++ b/api/geth_backend.go
@@ -2079,12 +2079,6 @@ func (b *GethStatusBackend) loadNodeConfig(inputNodeCfg *params.NodeConfig) erro
         }
     }
 
-    if len(conf.LogDir) == 0 {
-        conf.LogFile = filepath.Join(b.rootDataDir, conf.LogFile)
-    } else {
-        conf.LogFile = filepath.Join(conf.LogDir, conf.LogFile)
-    }
-
     b.config = conf
 
     if inputNodeCfg != nil && inputNodeCfg.RuntimeLogLevel != "" {


diff --git a/params/config.go b/params/config.go
index ca3ee5bf9..77d0a1b92 100644
--- a/params/config.go
+++ b/params/config.go
@@ -1196,12 +1196,19 @@ func LesTopic(netid int) string {
     }
 }
 
+func (c *NodeConfig) LogFilePath() string {
+    if len(c.LogDir) == 0 {
+        return filepath.Join(c.RootDataDir, c.LogFile)
+    }
+    return filepath.Join(c.LogDir, c.LogFile)
+}
+
 func (c *NodeConfig) LogSettings() logutils.LogSettings {
     return logutils.LogSettings{
         Enabled:         c.LogEnabled,
         Level:           c.LogLevel,
         Namespaces:      c.LogNamespaces,
-        File:            c.LogFile,
+        File:            c.LogFilePath(),
         MaxSize:         c.LogMaxSize,
         MaxBackups:      c.LogMaxBackups,
         CompressRotated: c.LogCompressRotated,

osmaczko avatar Dec 12 '24 20:12 osmaczko

Image

friofry avatar Sep 03 '25 12:09 friofry