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

Jump to bottom

node: drop support for static & trusted node list files #25610

Merged
merged 10 commits into from
Oct 12, 2022
Merged

node: drop support for static & trusted node list files #25610

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
88faf1a
node: dont read persistent node files anymore
s1na Aug 26, 2022
964f98a
minor
s1na Aug 26, 2022
d27f844
Revert "minor"
s1na Aug 31, 2022
954a1ee
Revert "node: dont read persistent node files anymore"
s1na Aug 31, 2022
f6244e6
Raise error if legacy node files exist
s1na Aug 31, 2022
a7cc4e6
unexport checkLegacyFiles
s1na Aug 31, 2022
7568922
node: simplify legacy path check
holiman Sep 28, 2022
f4ae67d
node: use the right logger
holiman Oct 5, 2022
5b00125
node: fix typo
holiman Oct 5, 2022
7c5b80c
node: simplify warnings and avoid copying locks
holiman Oct 5, 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
56 changes: 17 additions & 39 deletions node/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/p2p"
"github.com/ethereum/go-ethereum/p2p/enode"
"github.com/ethereum/go-ethereum/rpc"
)

Expand Down Expand Up @@ -341,7 +340,7 @@ func (c *Config) ResolvePath(path string) string {
}
if oldpath != "" && common.FileExist(oldpath) {
if warn {
c.warnOnce(&c.oldGethResourceWarning, "Using deprecated resource file %s, please move this file to the 'geth' subdirectory of datadir.", oldpath)
c.warnOnce(&c.oldGethResourceWarning, false, "Using deprecated resource file %s, please move this file to the 'geth' subdirectory of datadir.", oldpath)
}
return oldpath
}
Expand Down Expand Up @@ -394,48 +393,23 @@ func (c *Config) NodeKey() *ecdsa.PrivateKey {
return key
}

// StaticNodes returns a list of node enode URLs configured as static nodes.
func (c *Config) StaticNodes() []*enode.Node {
return c.parsePersistentNodes(&c.staticNodesWarning, c.ResolvePath(datadirStaticNodes))
// CheckLegacyFiles inspects the datadir for signs of legacy static-nodes
// and trusted-nodes files. If they exist it raises an error.
func (c *Config) checkLegacyFiles() {
c.checkLegacyFile(&c.staticNodesWarning, c.ResolvePath(datadirStaticNodes))
c.checkLegacyFile(&c.trustedNodesWarning, c.ResolvePath(datadirTrustedNodes))
}

// TrustedNodes returns a list of node enode URLs configured as trusted nodes.
func (c *Config) TrustedNodes() []*enode.Node {
return c.parsePersistentNodes(&c.trustedNodesWarning, c.ResolvePath(datadirTrustedNodes))
}

// parsePersistentNodes parses a list of discovery node URLs loaded from a .json
// file from within the data directory.
func (c *Config) parsePersistentNodes(w *bool, path string) []*enode.Node {
// checkLegacyFile will only raise an error if a file at the given path exists.
func (c *Config) checkLegacyFile(w *bool, path string) {
// Short circuit if no node config is present
if c.DataDir == "" {
return nil
return
}
if _, err := os.Stat(path); err != nil {
return nil
}
c.warnOnce(w, "Found deprecated node list file %s, please use the TOML config file instead.", path)

// Load the nodes from the config file.
var nodelist []string
if err := common.LoadJSON(path, &nodelist); err != nil {
log.Error(fmt.Sprintf("Can't load node list file: %v", err))
return nil
}
// Interpret the list as a discovery node array
var nodes []*enode.Node
for _, url := range nodelist {
if url == "" {
continue
}
node, err := enode.Parse(enode.ValidSchemes, url)
if err != nil {
log.Error(fmt.Sprintf("Node URL %s: %v\n", url, err))
continue
}
nodes = append(nodes, node)
return
}
return nodes
c.warnOnce(w, true, "Ignoring deprecated node list file %s. Please use the TOML config file instead.", path)
Copy link
Contributor

@holiman holiman Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, what's the point of using warnOnce?

  • It uses the config.Logger, which is: // Logger is a custom logger to use with the p2p.Server . I don't see why we need to use that one here.
  • It uses the w to decide if it has already warned, in your case &c.staticNodesWarning. But is there practically any actual risk of that happening multiple times?
Suggested change
c.warnOnce(w, true, "Ignoring deprecated node list file %s. Please use the TOML config file instead.", path)
log.Error("Ignoring deprecated node list. Please use the TOML config file instead.", "path", path)

Copy link
Contributor

Choose a reason for hiding this comment

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

The checkLegacyFiles is only called from here:

	if node.server.Config.StaticNodes == nil || node.server.Config.TrustedNodes == nil {
		node.config.checkLegacyFiles()
	}

So I don't see how that could happen multiple times, so you can just skip that whole dance with &c.staticNodesWarning. I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

... and thus also remove

-       staticNodesWarning     bool
-       trustedNodesWarning    bool

Copy link
Contributor

Choose a reason for hiding this comment

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

Using c.Logger is good style. All p2p / node code uses the logger passed via config. It's useful in unit tests.

}

// KeyDirConfig determines the settings for keydirectory
Expand Down Expand Up @@ -485,7 +459,7 @@ func getKeyStoreDir(conf *Config) (string, bool, error) {

var warnLock sync.Mutex

func (c *Config) warnOnce(w *bool, format string, args ...interface{}) {
func (c *Config) warnOnce(w *bool, err bool, format string, args ...interface{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this seems like a micro-customization. Is it that big of a deal if this method always does WARN instead of ERR ?

I can live with the way it is though, just think it'd be cleaner to leave it be

warnLock.Lock()
defer warnLock.Unlock()

Expand All @@ -496,6 +470,10 @@ func (c *Config) warnOnce(w *bool, format string, args ...interface{}) {
if l == nil {
l = log.Root()
}
l.Warn(fmt.Sprintf(format, args...))
if err {
l.Error(fmt.Sprintf(format, args...))
} else {
l.Warn(fmt.Sprintf(format, args...))
}
*w = true
}
7 changes: 2 additions & 5 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,8 @@ func New(conf *Config) (*Node, error) {
node.server.Config.PrivateKey = node.config.NodeKey()
node.server.Config.Name = node.config.NodeName()
node.server.Config.Logger = node.log
if node.server.Config.StaticNodes == nil {
node.server.Config.StaticNodes = node.config.StaticNodes()
}
if node.server.Config.TrustedNodes == nil {
node.server.Config.TrustedNodes = node.config.TrustedNodes()
if node.server.Config.StaticNodes == nil || node.server.Config.TrustedNodes == nil {
node.config.checkLegacyFiles()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if node.server.Config.StaticNodes == nil || node.server.Config.TrustedNodes == nil {
node.config.checkLegacyFiles()
}
node.config.checkLegacyFiles()

It's a pretty cheap check... Might help poke people to remember to clean it up. Also, just doing a geth .. dumpconfig spits out

$ cat conf.toml| grep "icNodes\|edNodes"
StaticNodes = []
TrustedNodes = []

I.E: empty but non-nil. So a lot of default configs have these empty lists and would therefore silently avoid the legacy file-check.

if node.server.Config.NodeDatabase == "" {
node.server.Config.NodeDatabase = node.config.NodeDB()
Expand Down