all: make nodes publish coinbase address to stats server #1967
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to publish node coinbase addresses to the stats server, enabling better node identification and monitoring. The changes modify the ethstats service to extract and send the coinbase address from the node's account manager, and update deployment scripts to make stats server configuration more flexible through environment variables.
Changes:
- Modified ethstats service to fetch and publish coinbase address from the first account in AccountManager
- Added new
Coinbasefield to nodeInfo struct that is sent to the stats server during authentication - Refactored deployment scripts to extract hardcoded stats server configuration into environment variables with sensible defaults
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ethstats/ethstats.go | Added AccountManager interface method, coinbase field to Service struct, and logic to extract and publish coinbase address |
| cicd/testnet/start.sh | Extracted stats server configuration to environment variables, changed GC_MODE default from archive to full |
| cicd/mainnet/start.sh | Extracted stats server configuration to environment variables, changed GC_MODE default from archive to full |
| cicd/local/start.sh | Extracted stats server configuration to environment variables, added ethstats flag to XDC command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accounts := am.Accounts() | ||
| if len(accounts) > 0 { | ||
| coinbase := accounts[0] | ||
| ethstats.coinbase = coinbase.String0x() |
There was a problem hiding this comment.
The variable coinbase is of type accounts.Account, not common.Address. The String0x() method is defined on common.Address. You need to access the Address field of the Account struct. Change this to coinbase.Address.String0x().
| ethstats.coinbase = coinbase.String0x() | |
| ethstats.coinbase = coinbase.Address.String0x() |
| fi | ||
|
|
||
| gc_mode=archive | ||
| gc_mode=full |
There was a problem hiding this comment.
The default GC_MODE has been changed from "archive" to "full". This is a significant behavioral change that could affect historical data retention. Archive mode stores all historical state, while full mode only stores recent state. Please ensure this change is intentional and documented, as it may impact nodes that rely on historical state queries.
| fi | ||
|
|
||
| gc_mode=archive | ||
| gc_mode=full |
There was a problem hiding this comment.
The default GC_MODE has been changed from "archive" to "full". This is a significant behavioral change that could affect historical data retention. Archive mode stores all historical state, while full mode only stores recent state. Please ensure this change is intentional and documented, as it may impact nodes that rely on historical state queries.
| if am := backend.AccountManager(); am != nil { | ||
| accounts := am.Accounts() | ||
| if len(accounts) > 0 { | ||
| coinbase := accounts[0] | ||
| ethstats.coinbase = coinbase.String0x() | ||
| } | ||
| } |
There was a problem hiding this comment.
The new coinbase functionality added in the New() function lacks test coverage. Consider adding a test case to verify that the coinbase address is correctly extracted from the AccountManager and stored in the Service struct, as well as a test case for when AccountManager is nil or has no accounts.
| engine: engine, | ||
| server: node.Server(), | ||
| node: parts[0], | ||
| coinbase: "", // will be set below |
There was a problem hiding this comment.
coinbase: "" this line can be removed
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that