Use separate file for address book store #33
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the functionality of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LocalStateManager
participant AddressBook
User->>LocalStateManager: SaveAddressBook()
LocalStateManager->>AddressBook: os.WriteFile(addressBookName)
AddressBook-->>LocalStateManager: Confirmation
User->>LocalStateManager: RetrieveP2PAddresses()
LocalStateManager->>AddressBook: loadAddressBook(seedPath)
AddressBook-->>LocalStateManager: List of addresses
LocalStateManager->>AddressBook: loadAddressBook(statePath)
AddressBook-->>LocalStateManager: List of addresses
LocalStateManager-->>User: Combined list of addresses
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.61.0)level=warning msg="[config_reader] The configuration option Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gartnera
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
storage/localstate_mgr.go (1)
Line range hint
177-195: Consider implementing atomic write operation for data safety.While the implementation is generally sound, consider these improvements for production reliability:
- Add validation for empty address map
- Implement atomic write using temporary file and rename operation to prevent data corruption on system crashes
Here's a suggested implementation:
func (fsm *FileStateMgr) SaveAddressBook(address map[peer.ID][]maddr.Multiaddr) error { if len(fsm.folder) < 1 { return errors.New("base file path is invalid") } + if len(address) == 0 { + return errors.New("empty address map") + } filePathName := filepath.Join(fsm.folder, addressBookName) + tempFile := filePathName + ".tmp" var buf bytes.Buffer for peer, addrs := range address { for _, addr := range addrs { if strings.Contains(addr.String(), "127.0.0.1") { continue } record := addr.String() + "/p2p/" + peer.String() + "\n" _, err := buf.WriteString(record) if err != nil { return errors.New("fail to write the record to buffer") } } } fsm.writeLock.Lock() defer fsm.writeLock.Unlock() - return os.WriteFile(filePathName, buf.Bytes(), 0o600) + if err := os.WriteFile(tempFile, buf.Bytes(), 0o600); err != nil { + return fmt.Errorf("failed to write temporary file: %w", err) + } + return os.Rename(tempFile, filePathName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- storage/localstate_mgr.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
storage/localstate_mgr.go (1)
27-27: LGTM: Well-defined constant for address book state file.The constant improves maintainability by centralizing the file name definition and clearly distinguishes it from the seed file.
storage/localstate_mgr.go
Outdated
| // RetrieveP2PAddresses loads addresses from both the seed address book | ||
| // and state address book | ||
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | ||
| if len(fsm.folder) < 1 { | ||
| return nil, errors.New("base file path is invalid") | ||
| } | ||
| filePathName := filepath.Join(fsm.folder, "address_book.seed") | ||
| filePathName = filepath.Clean(filePathName) | ||
|
|
||
| _, err := os.Stat(filePathName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fsm.writeLock.RLock() | ||
| input, err := ioutil.ReadFile(filePathName) | ||
| defer fsm.writeLock.RUnlock() | ||
|
|
||
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | ||
| seedPath = filepath.Clean(seedPath) | ||
| seedAddresses, err := loadAddressBook(seedPath) | ||
| if err != nil { | ||
| fsm.writeLock.RUnlock() | ||
| return nil, err | ||
| } | ||
| fsm.writeLock.RUnlock() | ||
| data := strings.Split(string(input), "\n") | ||
| var peerAddresses []maddr.Multiaddr | ||
| for _, el := range data { | ||
| // we skip the empty entry | ||
| if len(el) == 0 { | ||
| continue | ||
| } | ||
| addr, err := maddr.NewMultiaddr(el) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid address in address book %w", err) | ||
|
|
||
| savePath := filepath.Join(fsm.folder, addressBookName) | ||
| savePath = filepath.Clean(savePath) | ||
| savedAddresses, err := loadAddressBook(savePath) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return seedAddresses, nil | ||
| } | ||
| peerAddresses = append(peerAddresses, addr) | ||
| return seedAddresses, err | ||
| } | ||
| return peerAddresses, nil | ||
|
|
||
| return append(seedAddresses, savedAddresses...), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider deduplicating addresses for efficiency.
The implementation correctly loads addresses from both sources, but consider deduplicating the combined addresses to prevent potential issues with duplicate connections.
Here's a suggested implementation for deduplication:
func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) {
// ... existing code ...
- return append(seedAddresses, savedAddresses...), nil
+ // Deduplicate addresses
+ seen := make(map[string]bool)
+ unique := make([]maddr.Multiaddr, 0, len(seedAddresses)+len(savedAddresses))
+
+ for _, addr := range append(seedAddresses, savedAddresses...) {
+ addrStr := addr.String()
+ if !seen[addrStr] {
+ seen[addrStr] = true
+ unique = append(unique, addr)
+ }
+ }
+ return unique, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // RetrieveP2PAddresses loads addresses from both the seed address book | |
| // and state address book | |
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | |
| if len(fsm.folder) < 1 { | |
| return nil, errors.New("base file path is invalid") | |
| } | |
| filePathName := filepath.Join(fsm.folder, "address_book.seed") | |
| filePathName = filepath.Clean(filePathName) | |
| _, err := os.Stat(filePathName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| fsm.writeLock.RLock() | |
| input, err := ioutil.ReadFile(filePathName) | |
| defer fsm.writeLock.RUnlock() | |
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | |
| seedPath = filepath.Clean(seedPath) | |
| seedAddresses, err := loadAddressBook(seedPath) | |
| if err != nil { | |
| fsm.writeLock.RUnlock() | |
| return nil, err | |
| } | |
| fsm.writeLock.RUnlock() | |
| data := strings.Split(string(input), "\n") | |
| var peerAddresses []maddr.Multiaddr | |
| for _, el := range data { | |
| // we skip the empty entry | |
| if len(el) == 0 { | |
| continue | |
| } | |
| addr, err := maddr.NewMultiaddr(el) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid address in address book %w", err) | |
| savePath := filepath.Join(fsm.folder, addressBookName) | |
| savePath = filepath.Clean(savePath) | |
| savedAddresses, err := loadAddressBook(savePath) | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, nil | |
| } | |
| peerAddresses = append(peerAddresses, addr) | |
| return seedAddresses, err | |
| } | |
| return peerAddresses, nil | |
| return append(seedAddresses, savedAddresses...), nil | |
| // RetrieveP2PAddresses loads addresses from both the seed address book | |
| // and state address book | |
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | |
| if len(fsm.folder) < 1 { | |
| return nil, errors.New("base file path is invalid") | |
| } | |
| fsm.writeLock.RLock() | |
| defer fsm.writeLock.RUnlock() | |
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | |
| seedPath = filepath.Clean(seedPath) | |
| seedAddresses, err := loadAddressBook(seedPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| savePath := filepath.Join(fsm.folder, addressBookName) | |
| savePath = filepath.Clean(savePath) | |
| savedAddresses, err := loadAddressBook(savePath) | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, nil | |
| } | |
| return seedAddresses, err | |
| } | |
| // Deduplicate addresses | |
| seen := make(map[string]bool) | |
| unique := make([]maddr.Multiaddr, 0, len(seedAddresses)+len(savedAddresses)) | |
| for _, addr := range append(seedAddresses, savedAddresses...) { | |
| addrStr := addr.String() | |
| if !seen[addrStr] { | |
| seen[addrStr] = true | |
| unique = append(unique, addr) | |
| } | |
| } | |
| return unique, nil | |
| } |
There was a problem hiding this comment.
Doesn't really matter imo. libp2p should deduplicate it.
There was a problem hiding this comment.
@gartnera, understood. Since libp2p handles deduplication, manual deduplication may not be necessary.
(_/_)
( ••)
( --)
✏️ Learnings added
Learnt from: gartnera
PR: zeta-chain/go-tss#33
File: storage/localstate_mgr.go:0-0
Timestamp: 2024-10-23T23:04:45.574Z
Learning: libp2p deduplicates addresses, so manual deduplication of addresses in code is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
gartnera
force-pushed
the
seed-noclobber
branch
from
d2a913f to
62f4d14
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
storage/localstate_mgr_test.go (1)
Line range hint
97-104: Consider expanding test coverage for the new address book functionality.The current test only verifies saving to the new address book file. Given the architectural change to support both seed and state files, consider adding test cases for:
- Loading addresses from both seed and state files
- Handling missing or corrupted seed files
- Verifying the merged results from both sources
Here's a suggested test structure:
func (s *FileStateMgrTestSuite) TestRetrieveP2PAddresses(c *C) { // Setup test data seedAddresses := make(map[peer.ID][]maddr.Multiaddr) stateAddresses := make(map[peer.ID][]maddr.Multiaddr) // Test cases: // 1. Both files exist // 2. Only seed file exists // 3. Only state file exists // 4. Neither file exists // 5. Corrupted seed file // 6. Duplicate addresses in both files }storage/localstate_mgr.go (1)
Line range hint
177-195: Add input validation for robustness.The method should validate inputs and address formats before saving.
Consider adding these validations:
func (fsm *FileStateMgr) SaveAddressBook(address map[peer.ID][]maddr.Multiaddr) error { + if address == nil { + return errors.New("address map cannot be nil") + } if len(fsm.folder) < 1 { return errors.New("base file path is invalid") } filePathName := filepath.Join(fsm.folder, addressBookName) var buf bytes.Buffer for peer, addrs := range address { + if len(addrs) == 0 { + continue + } for _, addr := range addrs { + if addr == nil { + continue + } // we do not save the loopback addr if strings.Contains(addr.String(), "127.0.0.1") { continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- storage/localstate_mgr.go (4 hunks)
- storage/localstate_mgr_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
storage/localstate_mgr_test.go (1)
98-98: LGTM! The path change aligns with the new architecture.The update correctly uses the new
addressBookNameconstant, maintaining consistency with the implementation.storage/localstate_mgr.go (1)
27-27: LGTM: Good constant extraction.The constant extraction improves maintainability by centralizing the address book filename.
storage/localstate_mgr.go
Outdated
| func loadAddressBook(path string) ([]maddr.Multiaddr, error) { | ||
| _, err := os.Stat(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| input, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| data := strings.Split(string(input), "\n") | ||
| var peerAddresses []maddr.Multiaddr | ||
| for _, el := range data { | ||
| // we skip the empty entry | ||
| if len(el) == 0 { | ||
| continue | ||
| } | ||
| addr, err := maddr.NewMultiaddr(el) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid address in address book: %w", err) | ||
| } | ||
| peerAddresses = append(peerAddresses, addr) | ||
| } | ||
| return peerAddresses, nil | ||
| } |
There was a problem hiding this comment.
Enhance error handling and add file size limit.
The function should provide more context in errors and protect against extremely large files.
func loadAddressBook(path string) ([]maddr.Multiaddr, error) {
_, err := os.Stat(path)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to stat address book %s: %w", path, err)
+ }
+
+ // Protect against extremely large files
+ fileInfo, err := os.Stat(path)
+ if err != nil {
+ return nil, fmt.Errorf("failed to stat address book %s: %w", path, err)
+ }
+ if fileInfo.Size() > 10*1024*1024 { // 10MB limit
+ return nil, fmt.Errorf("address book %s exceeds size limit of 10MB", path)
}
input, err := os.ReadFile(path)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to read address book %s: %w", path, err)
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Doesn't really matter imo
There was a problem hiding this comment.
@gartnera, understood.
╭(◕﹏◕)╮
✏️ Learnings added
Learnt from: gartnera
PR: zeta-chain/go-tss#33
File: storage/localstate_mgr.go:0-0
Timestamp: 2024-10-23T23:04:27.635Z
Learning: When reviewing the `loadAddressBook` function, it's acceptable not to enforce additional error handling or file size limits as per the team's preference.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
gartnera
force-pushed
the
seed-noclobber
branch
from
62f4d14 to
07555ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
storage/localstate_mgr.go (2)
Line range hint
177-195: Enhance error handling with context.While the implementation is correct, the error message could be more descriptive to aid in debugging.
- return errors.New("fail to write the record to buffer") + return fmt.Errorf("failed to write address record to buffer: %w", err)
286-310: Enhance error messages with file path context.While the error handling is present, the error messages could be more descriptive by including the file path.
_, err := os.Stat(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to stat address book %s: %w", path, err) } input, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read address book %s: %w", path, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- storage/localstate_mgr.go (4 hunks)
- storage/localstate_mgr_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/localstate_mgr_test.go
🧰 Additional context used
🔇 Additional comments (1)
storage/localstate_mgr.go (1)
27-27: LGTM: Good practice to define file name as a constant.The constant name is descriptive and follows Go naming conventions.
storage/localstate_mgr.go
Outdated
| if err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return seedAddresses, nil | ||
| } | ||
| peerAddresses = append(peerAddresses, addr) | ||
| return seedAddresses, err |
There was a problem hiding this comment.
Fix redundant and incorrect error handling logic.
The nested if condition is redundant as the outer condition already checks for os.ErrNotExist.
if err != nil && !errors.Is(err, os.ErrNotExist) {
- if errors.Is(err, os.ErrNotExist) {
- return seedAddresses, nil
- }
return seedAddresses, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, nil | |
| } | |
| peerAddresses = append(peerAddresses, addr) | |
| return seedAddresses, err | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, err | |
| } |
gartnera
force-pushed
the
seed-noclobber
branch
from
07555ce to
c2b8cbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
storage/localstate_mgr.go (1)
Line range hint
177-195: Clean file path before writing.The file path should be cleaned to prevent path traversal vulnerabilities.
Apply this diff:
- filePathName := filepath.Join(fsm.folder, addressBookName) + filePathName := filepath.Clean(filepath.Join(fsm.folder, addressBookName))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- storage/localstate_mgr.go (4 hunks)
- storage/localstate_mgr_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/localstate_mgr_test.go
🔇 Additional comments (1)
storage/localstate_mgr.go (1)
27-27: LGTM: Good constant extraction.The constant improves maintainability by centralizing the address book filename.
storage/localstate_mgr.go
Outdated
| // RetrieveP2PAddresses loads addresses from both the seed address book | ||
| // and state address book | ||
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | ||
| if len(fsm.folder) < 1 { | ||
| return nil, errors.New("base file path is invalid") | ||
| } | ||
| filePathName := filepath.Join(fsm.folder, "address_book.seed") | ||
| filePathName = filepath.Clean(filePathName) | ||
|
|
||
| _, err := os.Stat(filePathName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fsm.writeLock.RLock() | ||
| input, err := ioutil.ReadFile(filePathName) | ||
| if err != nil { | ||
| fsm.writeLock.RUnlock() | ||
| defer fsm.writeLock.RUnlock() | ||
|
|
||
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | ||
| seedAddresses, err := loadAddressBook(seedPath) | ||
| if err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return nil, err | ||
| } | ||
| fsm.writeLock.RUnlock() | ||
| data := strings.Split(string(input), "\n") | ||
| var peerAddresses []maddr.Multiaddr | ||
| for _, el := range data { | ||
| // we skip the empty entry | ||
| if len(el) == 0 { | ||
| continue | ||
| } | ||
| addr, err := maddr.NewMultiaddr(el) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid address in address book %w", err) | ||
| } | ||
| peerAddresses = append(peerAddresses, addr) | ||
|
|
||
| savePath := filepath.Join(fsm.folder, addressBookName) | ||
| savedAddresses, err := loadAddressBook(savePath) | ||
| if err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return seedAddresses, err | ||
| } | ||
| return peerAddresses, nil | ||
|
|
||
| return append(seedAddresses, savedAddresses...), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling specificity.
The error handling could be more descriptive to aid in debugging.
Apply this diff:
seedAddresses, err := loadAddressBook(seedPath)
if err != nil && !errors.Is(err, os.ErrNotExist) {
- return nil, err
+ return nil, fmt.Errorf("failed to load seed address book: %w", err)
}
savePath := filepath.Join(fsm.folder, addressBookName)
savedAddresses, err := loadAddressBook(savePath)
if err != nil && !errors.Is(err, os.ErrNotExist) {
- return seedAddresses, err
+ return seedAddresses, fmt.Errorf("failed to load state address book: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // RetrieveP2PAddresses loads addresses from both the seed address book | |
| // and state address book | |
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | |
| if len(fsm.folder) < 1 { | |
| return nil, errors.New("base file path is invalid") | |
| } | |
| filePathName := filepath.Join(fsm.folder, "address_book.seed") | |
| filePathName = filepath.Clean(filePathName) | |
| _, err := os.Stat(filePathName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| fsm.writeLock.RLock() | |
| input, err := ioutil.ReadFile(filePathName) | |
| if err != nil { | |
| fsm.writeLock.RUnlock() | |
| defer fsm.writeLock.RUnlock() | |
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | |
| seedAddresses, err := loadAddressBook(seedPath) | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return nil, err | |
| } | |
| fsm.writeLock.RUnlock() | |
| data := strings.Split(string(input), "\n") | |
| var peerAddresses []maddr.Multiaddr | |
| for _, el := range data { | |
| // we skip the empty entry | |
| if len(el) == 0 { | |
| continue | |
| } | |
| addr, err := maddr.NewMultiaddr(el) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid address in address book %w", err) | |
| } | |
| peerAddresses = append(peerAddresses, addr) | |
| savePath := filepath.Join(fsm.folder, addressBookName) | |
| savedAddresses, err := loadAddressBook(savePath) | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, err | |
| } | |
| return peerAddresses, nil | |
| return append(seedAddresses, savedAddresses...), nil | |
| // RetrieveP2PAddresses loads addresses from both the seed address book | |
| // and state address book | |
| func (fsm *FileStateMgr) RetrieveP2PAddresses() ([]maddr.Multiaddr, error) { | |
| if len(fsm.folder) < 1 { | |
| return nil, errors.New("base file path is invalid") | |
| } | |
| fsm.writeLock.RLock() | |
| defer fsm.writeLock.RUnlock() | |
| seedPath := filepath.Join(fsm.folder, "address_book.seed") | |
| seedAddresses, err := loadAddressBook(seedPath) | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return nil, fmt.Errorf("failed to load seed address book: %w", err) | |
| } | |
| savePath := filepath.Join(fsm.folder, addressBookName) | |
| savedAddresses, err := loadAddressBook(savePath) | |
| if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return seedAddresses, fmt.Errorf("failed to load state address book: %w", err) | |
| } | |
| return append(seedAddresses, savedAddresses...), nil |
|
Part of the goal of replacing address_book.seed was to reset the peer list to a known good configuration. Would merging this PR undo that, and allow the entries that caused the crash loop to remain or is this PR intended to be layered on top of another mitigation that ensures unexpected peers aren't allowed? |
#33 should prevent us from loading unexpected peers. Nothing would prevent you from also managing/clearing |
gartnera
force-pushed
the
seed-noclobber
branch
from
c2b8cbf to
03068fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
storage/mock_localstatemanager.go (1)
24-26: Consider enhancing mock for better testability.The current implementation always returns nil values. Consider making the mock more configurable to support testing different scenarios, such as error conditions or specific address lists.
Here's a suggested enhancement:
type MockLocalStateManager struct { + // Add fields to configure mock behavior + BootstrapAddrs []maddr.Multiaddr + BootstrapError error } func (s *MockLocalStateManager) RetrieveBootstrapAddresses() ([]maddr.Multiaddr, error) { - return nil, nil + return s.BootstrapAddrs, s.BootstrapError }This would allow tests to:
- Configure specific addresses to be returned
- Simulate error conditions
- Verify behavior with empty vs. populated address lists
tss/tss.go (2)
Line range hint
83-89: Verify error handling for bootstrap address retrieval.The error handling appears to silently fall back to command-line bootstrap peers when
RetrieveBootstrapAddresses()fails. Consider logging the error for better observability and debugging.savedPeers, err := stateManager.RetrieveBootstrapAddresses() if err != nil { + t.logger.Debug().Err(err).Msg("failed to retrieve bootstrap addresses, falling back to command-line peers") bootstrapPeers = cmdBootstrapPeers } else { bootstrapPeers = savedPeers bootstrapPeers = append(bootstrapPeers, cmdBootstrapPeers...) }
Potential duplicate bootstrap peers should be deduplicated
The code in
tss/tss.goappends command-line bootstrap peers to saved peers without deduplication, while the peer discovery system (p2p/discovery.go) maintains aknownPeersmap to track unique peers. To maintain consistency and optimize peer discovery:
- Add deduplication logic in
tss/tss.gobefore passing bootstrap peers to the communication layer- Consider using a map-based approach similar to the peer discovery implementation to ensure uniqueness
Example solution:
seenPeers := make(map[string]struct{}) uniqueBootstrapPeers := make([]maddr.Multiaddr, 0) // Helper to add unique peers addUniquePeer := func(peer maddr.Multiaddr) { peerStr := peer.String() if _, exists := seenPeers[peerStr]; !exists { seenPeers[peerStr] = struct{}{} uniqueBootstrapPeers = append(uniqueBootstrapPeers, peer) } } // Add saved peers for _, peer := range savedPeers { addUniquePeer(peer) } // Add command-line peers for _, peer := range cmdBootstrapPeers { addUniquePeer(peer) } bootstrapPeers = uniqueBootstrapPeers🔗 Analysis chain
Line range hint
83-89: Verify the impact of combining bootstrap sources.The code appends command-line bootstrap peers to saved peers, which could lead to duplicate entries. Consider deduplicating the combined list to optimize peer discovery.
Let's verify if there's potential for duplicate entries in the bootstrap peers:
Let me gather more information about the peer handling logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any peer address deduplication logic in the codebase rg -A 5 "bootstrap.*peer"Length of output: 4155
Script:
#!/bin/bash # Search for peer handling and deduplication logic ast-grep --pattern 'func $_(bootstrapPeers []peer.AddrInfo) $_' # Check if there's any map-based peer tracking rg -A 5 "map\[peer\.ID\]" # Look for any peer list manipulation rg -A 5 "append.*peer"Length of output: 13524
storage/localstate_mgr.go (1)
201-216: Consider enhancing error messages for better debugging.While the implementation is clean, consider wrapping the error for better context:
func (fsm *FileStateMgr) RetrieveBootstrapAddresses() ([]maddr.Multiaddr, error) { if len(fsm.folder) < 1 { - return nil, errors.New("base file path is invalid") + return nil, fmt.Errorf("invalid base file path: %q", fsm.folder) } // ... rest of the function if err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, err + return nil, fmt.Errorf("failed to load seed addresses: %w", err) }keysign/ecdsa/keysign_old_test.go (1)
80-82: LGTM! Consider adding documentation.The mock implementation correctly returns
nilandos.ErrNotExist, consistent with the mock nature of the struct and matching the behavior ofRetrieveP2PAddresses.Consider adding a comment to document the mock behavior:
+// RetrieveBootstrapAddresses implements LocalStateManager interface with mock behavior func (s *MockLocalStateOldManager) RetrieveBootstrapAddresses() ([]maddr.Multiaddr, error) { return nil, os.ErrNotExist }keysign/ecdsa/keysign_test.go (1)
92-94: LGTM! Consider adding a comment to document the mock behavior.The implementation correctly returns
nilandos.ErrNotExistas expected for a mock implementation. This aligns with the PR's objective of managing address book files separately.Consider adding a comment to document the mock behavior:
+// RetrieveBootstrapAddresses implements LocalStateManager interface by returning os.ErrNotExist func (s *MockLocalStateManager) RetrieveBootstrapAddresses() ([]maddr.Multiaddr, error) { return nil, os.ErrNotExist }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
keysign/ecdsa/keysign_old_test.go(1 hunks)keysign/ecdsa/keysign_test.go(1 hunks)keysign/eddsa/keysign_test.go(1 hunks)storage/localstate_mgr.go(5 hunks)storage/localstate_mgr_test.go(1 hunks)storage/mock_localstatemanager.go(1 hunks)tss/tss.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/localstate_mgr_test.go
🧰 Additional context used
📓 Learnings (1)
storage/localstate_mgr.go (1)
Learnt from: gartnera
PR: zeta-chain/go-tss#33
File: storage/localstate_mgr.go:283-307
Timestamp: 2024-11-12T13:20:12.657Z
Learning: In the `loadAddressBook` function in `storage/localstate_mgr.go`, the `path` parameter will never be empty, so input validation for an empty path is unnecessary.
🔇 Additional comments (7)
storage/mock_localstatemanager.go (1)
24-26: Implementation follows mock patterns correctly.
The new method maintains consistency with other mock methods in the struct and provides the expected interface implementation.
tss/tss.go (1)
Line range hint 91-108: LGTM: Robust whitelist filtering implementation.
The implementation correctly filters bootstrap peers against the whitelist, ensuring only authorized peers are included. The error handling for peer address parsing is appropriate.
keysign/eddsa/keysign_test.go (1)
88-90: LGTM: Mock implementation aligns with PR objectives
The mock implementation correctly returns os.ErrNotExist to simulate the behavior of a non-existent bootstrap addresses file, which is consistent with the PR's goal of separating address book files.
storage/localstate_mgr.go (4)
27-27: LGTM! Clear constant naming and backward-compatible interface update.
The new constant addressBookName and the interface addition of RetrieveBootstrapAddresses are well-structured and maintain backward compatibility.
Also applies to: 54-54
178-178: LGTM! Good modernization of file operations.
The changes improve maintainability by using the constant and modernize the code by switching to os.WriteFile.
Also applies to: 196-196
218-235: LGTM! Well-structured implementation with good code reuse.
The implementation efficiently reuses RetrieveBootstrapAddresses and correctly combines addresses from both sources. The approach is clean and maintainable.
298-322: LGTM! Robust helper function with good error handling.
The implementation properly handles empty entries and provides clear error messages for invalid addresses. The function is well-structured and focused.
| } | ||
| addr, err := maddr.NewMultiaddr(el) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid address in address book: %w", err) |
There was a problem hiding this comment.
just to check can we skip invalid ones, or this wants to also ensure we have error in that case?
There was a problem hiding this comment.
If for whatever reason the address_book file is invalid it will just be ignored and only use the address_book.seed will be used. The address_book.seed must be valid.
Use separate files for the seed and saved state of the address book to allow for managing the address_book.seed under configuration control.
Both files will be loaded by
RetrieveP2PAddresses()butSaveAddressBook()will only write toaddress_bookSummary by CodeRabbit
New Features
Bug Fixes
Tests