fix: replace DHT with private peer discovery #3041
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3041 +/- ##
===========================================
- Coverage 63.38% 63.30% -0.09%
===========================================
Files 422 422
Lines 29915 29954 +39
===========================================
Hits 18963 18963
- Misses 10111 10150 +39
Partials 841 841
|
brewmaster012
requested review from
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
zetaclient/tss/tss_signer.go (3)
Line range hint
246-267: Enhance error handling in Sign and SignBatch methods.Both methods have similar error handling patterns that could be improved:
- Error messages contain typos ("signuature")
- Some error conditions lack specific error types
Apply these improvements:
-return [65]byte{}, fmt.Errorf("signuature verification fail") +return [65]byte{}, fmt.Errorf("signature verification failed: %w", err)Consider creating custom error types for better error handling:
var ( ErrSignatureVerification = errors.New("signature verification failed") ErrSignatureDecoding = errors.New("signature decoding failed") )Also applies to: 349-370
Line range hint
258-267: Refactor signature verification logic.The signature verification code is duplicated between
SignandSignBatchmethods.Extract the common verification logic into a helper function:
func verifyAndDecodeSig(signature keysign.Signature, digest []byte, pubkey []byte) ([65]byte, error) { var sigBytes [65]byte if err := decodeSignatureParts(&sigBytes, signature); err != nil { return [65]byte{}, fmt.Errorf("failed to decode signature: %w", err) } if err := verifySignatureBytes(sigBytes, digest, pubkey); err != nil { return [65]byte{}, fmt.Errorf("failed to verify signature: %w", err) } return sigBytes, nil }Also applies to: 361-370
Line range hint
233-242: Consider implementing retry mechanism for failed signatures.Both
SignandSignBatchmethods post blame data but don't attempt to retry the signing operation.Consider implementing a retry mechanism with exponential backoff for transient failures:
- Add retry configuration parameters
- Implement exponential backoff
- Only retry for specific error conditions
- Add metrics for retry attempts
Would you like me to provide a detailed implementation of this retry mechanism?
Also applies to: 336-345
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
- cmd/zetaclientd/start.go (3 hunks)
- go.mod (3 hunks)
- zetaclient/tss/tss_signer.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cmd/zetaclientd/start.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/tss/tss_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/tss/tss_signer.go
[warning] 189-189: zetaclient/tss/tss_signer.go#L189
Added line #L189 was not covered by tests
🔇 Additional comments (4)
cmd/zetaclientd/start.go (1)
220-220: LGTM: TSS server setup with whitelisted peers.The implementation correctly integrates the whitelisted peers with the TSS server setup, aligning with the PR objective to replace DHT with private peer discovery.
go.mod (3)
244-244: LGTM: Direct dependency on base58 is appropriate.The change from indirect to direct dependency on
github.com/mr-tron/base58is correct as it's now explicitly used in the codebase.
371-372: LGTM: Updated go-tss dependency.The replacement of
gitlab.com/thorchain/tss/go-tsswith a specific commit from the ZetaChain fork aligns with the PR objective of implementing private peer discovery.
337-337: Verify compatibility with go-libp2p.The addition of
github.com/libp2p/go-libp2p-core v0.20.1might need verification as this version could be outdated relative to the replacedgo-libp2pdependency.Consider evaluating if this dependency can be replaced with the newer
github.com/libp2p/go-libp2pinterfaces, asgo-libp2p-coremight be deprecated in future releases.✅ Verification successful
Let me gather more information about the libp2p dependencies to verify compatibility.
Let me check the actual usage of these dependencies in the code to make a more informed decision.
Dependency version is compatible with current implementation
The codebase shows no direct usage of
go-libp2p-coreimports. Instead, it correctly uses the newergithub.com/libp2p/go-libp2p/core/*path structure, as evidenced by imports likegithub.com/libp2p/go-libp2p/core/peerandgithub.com/libp2p/go-libp2p/core/crypto. Thego-libp2p-coredependency is likely a transitive dependency required by other modules and its version (v0.20.1) is compatible with the custom fork of go-libp2p being used (github.com/zeta-chain/go-libp2p).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the libp2p-core version is compatible with the replaced go-libp2p version # Test: Search for version constraints in go-libp2p rg -A 5 "go-libp2p-core" "go.mod"Length of output: 157
Script:
#!/bin/bash # Check for go-libp2p version and any replace directives rg "github.com/libp2p/go-libp2p " go.mod # Check for any replace directives related to libp2p rg "^replace.*libp2p" go.mod # Search for actual usage of libp2p-core in the codebase rg -l "github.com/libp2p/go-libp2p-core" # Check if there are any go.sum entries for these dependencies rg "github.com/libp2p/go-libp2p(-core)? v" go.sumLength of output: 501
Script:
#!/bin/bash # Search for actual imports and usage of go-libp2p-core rg -A 3 "github.com/libp2p/go-libp2p-core" --type go # Check for any imports of the newer go-libp2p interfaces that might overlap rg -A 3 "github.com/libp2p/go-libp2p" --type go | grep -v "go-libp2p-core" # Look for any migration guides or deprecation notices in the repository rg -i "migrate|deprecat.*libp2p" -g "*.md"Length of output: 3325
in a few other 8123 handlers
|
@gartnera : do you have any idea why the tss migration e2e test failed in this PR? |
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
Looks like it's been failing in the nightlies for a few days |
Don't think it affects migration of TSS as keygen should be handled correctly. |
resolved conflict of changelo.md and go.mod
until the PR commit is merged.
* import go-tss lib that removes DHT * replace DHT with authenticated discovery * use JSON serialization; add metric * add new telemetry: 8123/connectedpeers; fix deadlock in a few other 8123 handlers * use squashed go-tss commit * clean up interface * address review comments * remove whiteliste peers * fmt * remove rendezvous * use merged go-tss connection gater * use latest go-tss from PR#34 * use merged #34 in go-tss lib * add ping RTT to telemetry * changelog * make linter happy * pingrtt * finer resolution on pingrtt time (milliseconds => nanoseconds) * removed comments * bump go-tss to the merged commit in master branch * revert back to the go-tss commit until the PR commit is merged. --------- Co-authored-by: pwu <[email protected]>
* feat: whitelist connection gater (#3028) * bump go tss to remove dht * add whitelist fields * disable whitelist for localnet * bump go-tss * resolve whitelisted peers wip * dont disable whitelist in e2e tests * cleanup whitelist fields from config and fix e2e tests * bump go-tss * cleanup * bump go tss * use node accounts to get whitelisted peers * bump go tss * changelog * fix unit test * bump go tss * remove usage of pointers in node accounts * fix unit test * revert back to using keygen for whitelist peers --------- Co-authored-by: Alex Gartner <[email protected]> * fix: replace DHT with private peer discovery (#3041) * import go-tss lib that removes DHT * replace DHT with authenticated discovery * use JSON serialization; add metric * add new telemetry: 8123/connectedpeers; fix deadlock in a few other 8123 handlers * use squashed go-tss commit * clean up interface * address review comments * remove whiteliste peers * fmt * remove rendezvous * use merged go-tss connection gater * use latest go-tss from PR#34 * use merged #34 in go-tss lib * add ping RTT to telemetry * changelog * make linter happy * pingrtt * finer resolution on pingrtt time (milliseconds => nanoseconds) * removed comments * bump go-tss to the merged commit in master branch * revert back to the go-tss commit until the PR commit is merged. --------- Co-authored-by: pwu <[email protected]> * skip depositor fee calculation on irrelevant txs * update changelog --------- Co-authored-by: skosito <[email protected]> Co-authored-by: brewmaster012 <[email protected]> Co-authored-by: pwu <[email protected]> Co-authored-by: Charlie Chen <[email protected]>
New 8123 telemetry methods:
/connectedpeersreturns a list of connected peers in p2p network (with connection gater, should really contain the tss signers, but we'll see.)/pingrttreturns the most recent of map tss signers => ping data--RTT in nanoseconds using libp2p Ping protocol--if no response RTT=-1This PR address the performance e2e test failure introduced by removing the DHT peer discovery service.
The actual change is in go-tss PR
Description
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Chores