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 WalkthroughThis pull request refactors the cryptographic signing mechanism across multiple components. The change shifts from using the concrete type Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant I as InitCmd
participant FS as FileSystemSigner
U->>I: Set signer type ("file") & passphrase flag
I->>I: Check if node is aggregator and signer type is "file"
I->>I: Retrieve signer passphrase from flags
I->>FS: Call NewFileSystemSigner(keyPath, passphrase)
FS-->>I: Return signer instance or error
I->>U: Proceed with node initialization (or return error)
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/exec_api #2078 +/- ##
====================================================
- Coverage 24.11% 22.81% -1.31%
====================================================
Files 52 57 +5
Lines 8231 9112 +881
====================================================
+ Hits 1985 2079 +94
- Misses 6089 6851 +762
- Partials 157 182 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RollkitBot
requested review from
a team,
MSevey and
tzdybal
and removed request for
a team
tac0turtle
requested review from
randygrok
and removed request for
MSevey and
tzdybal
![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 comments (1)
pkg/config/config_test.go (1)
91-105:⚠️ Potential issueFix failing test related to default Aggregator value
The pipeline failure indicates that
TestDefaultNodeConfigis failing because it expectsdef.Node.Aggregatorto betrueon line 21, but the default value has been changed tofalseindefaults.go.Apply this fix to update the test to match the new default value:
- assert.Equal(t, true, def.Node.Aggregator) + assert.Equal(t, false, def.Node.Aggregator)
🧹 Nitpick comments (13)
pkg/signer/file/README.md (1)
23-44: Avoid hardcoded passphrases in code examplesWhile the example clearly demonstrates the API usage, it shows a hardcoded passphrase which isn't recommended for production code.
Consider updating the example to show obtaining the passphrase securely, like:
-// Create a new signer (or load existing keys) -passphrase := []byte("your-secure-passphrase") +// Create a new signer (or load existing keys) +// In production, obtain passphrase securely (e.g., environment variables, secure prompt) +passphrase := obtainPassphraseSecurely() // Returns []bytetypes/hashing.go (1)
8-10: Leaf node prefix for hashing standardization.The introduction of
leafPrefixis a good practice used in Merkle tree implementations to differentiate leaf node hashes from internal node hashes, improving the cryptographic properties of the hash tree.Consider adding a comment explaining the purpose of this prefix constant, particularly noting its role in Merkle tree hashing if that's the intent.
pkg/signer/signer.go (1)
7-14: Well-designed Signer interface.The Signer interface is:
- Appropriately minimal with just the essential methods
- Well-documented with clear method descriptions
- Using standard cryptographic patterns (byte slices for messages and signatures)
- Built on established libp2p cryptographic types
This interface will effectively support the PR's goal of implementing both local and remote signing options.
Consider adding documentation about the expected signature format or encoding, if there are specific requirements that implementers need to follow.
pkg/signer/file/doc.go (1)
1-35: Good documentation but improve error handling examplesThe documentation clearly illustrates how to use the file-based signer implementation, which is helpful for users. However, using
panicin example code isn't a best practice for production applications.Consider updating the examples to demonstrate proper error handling rather than panicking:
// Create or load a signer signer, err := NewFileSystemSigner(keyPath, passphrase) if err != nil { - panic(err) + // Handle error appropriately, e.g., log and return + log.Printf("Failed to create signer: %v", err) + return err }Similarly update the other error handling examples to show graceful error handling patterns rather than panics.
pkg/signer/noop/signer.go (2)
16-23: Inconsistency between comment and implementationThe comment on line 10 states that NoopSigner "generates a new Ed25519 key pair for each instance," but the implementation accepts an existing private key instead of generating one.
-// NoopSigner implements the remote_signer.Signer interface. -// It generates a new Ed25519 key pair for each instance. +// NoopSigner implements the signer.Signer interface. +// It uses an existing private key for signing operations.
17-23: Consider adding input validation for the private keyThe function doesn't validate if the provided private key is nil or of the expected type, which could lead to runtime errors.
func NewNoopSigner(privKey crypto.PrivKey) (signer.Signer, error) { + if privKey == nil { + return nil, fmt.Errorf("private key cannot be nil") + } + + // Optional: Check if the key is of the expected type (e.g., Ed25519) + if privKey.Type() != crypto.Ed25519 { + return nil, fmt.Errorf("expected Ed25519 key, got %s", privKey.Type()) + } + return &NoopSigner{ privKey: privKey, pubKey: privKey.GetPublic(), }, nil }pkg/signer/file/example_test.go (2)
43-43: Redundant passphrase reinitializationThe passphrase is already defined at line 16 with the same value, so reinitializing it here is unnecessary.
- passphrase = []byte("test-passphrase-123") // Create a new signer instance (this will load the existing keys) signer2, err := NewFileSystemSigner(keyPath, passphrase)
67-103: Example function could be improved for documentationThe Example function is helpful but has some issues:
- It uses panic for error handling, which isn't a best practice for examples
- It doesn't print any output, so it won't work as a testable example in Go documentation
Consider modifying it to follow Go's example function conventions:
// Example demonstrates how to use the FileSystemSigner func Example() { // In a real application, you would use a secure passphrase passphrase := []byte("your-secure-passphrase") keyPath := "/path/to/secure/keys.json" // Create or load a signer signer, err := NewFileSystemSigner(keyPath, passphrase) - if err != nil { - panic(err) - } + if err != nil { + fmt.Println("Error creating signer:", err) + return + } // Sign a message message := []byte("Message to sign") signature, err := signer.Sign(message) - if err != nil { - panic(err) - } + if err != nil { + fmt.Println("Error signing message:", err) + return + } // Get the public key pubKey, err := signer.GetPublic() - if err != nil { - panic(err) - } + if err != nil { + fmt.Println("Error getting public key:", err) + return + } // Verify the signature (typically done by another party) valid, err := pubKey.Verify(message, signature) - if err != nil { - panic(err) - } + if err != nil { + fmt.Println("Error verifying signature:", err) + return + } if valid { - // Signature is valid + fmt.Println("Signature is valid") } else { - // Signature is invalid + fmt.Println("Signature is invalid") } + // Output: Signature is valid }cmd/rollkit/commands/init.go (1)
69-95: Good implementation of local file signer initialization.The code properly initializes a local file signer when the signer type is "file" and the node is an aggregator. It checks for a passphrase, creates the necessary directory, and initializes the signer.
Consider adding a verbose log message indicating successful signer initialization to improve user feedback.
// Initialize the signer _, err = file.NewFileSystemSigner(config.Signer.SignerPath, []byte(passphrase)) if err != nil { return fmt.Errorf("failed to initialize signer: %w", err) } +fmt.Printf("Successfully initialized signer at %s\n", config.Signer.SignerPath)cmd/rollkit/commands/run_node.go (1)
77-92: Implement local file signer selection
The branch checks the configured signer type and initializes accordingly. Usingpanicfor unimplemented gRPC might be more robustly handled by returning an error, but for now it clearly conveys it's unsupported.Would you like me to sketch out a basic gRPC signer implementation or fallback error handling?
pkg/signer/file/local.go (3)
67-89: Generating Ed25519 key pairs followed by immediate encryption is a good practice.No issues observed. The function
generateAndSaveKeysensures atomic creation of new keys. Minor note: consider logging success or failures for auditing & troubleshooting.
171-218: Decryption steps logically mirror encryption.Error messages are descriptive. One nitpick: you could log or wrap the error more thoroughly if passphrase-based decryption fails.
271-284: Fallback key derivation provides backward compatibility but weaker security.Ensure that users are informed of the weaker approach whenever old key files are used. Possibly consider a migration workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumpkg/signer/v1/signer.pb.gois excluded by!**/*.pb.gosequencers/single/go.sumis excluded by!**/*.sumtypes/pb/rollkit/v1/rollkit.pb.gois excluded by!**/*.pb.gotypes/pb/rollkit/v1/signer.pb.gois excluded by!**/*.pb.gotypes/pb/rollkit/v1/state.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (31)
block/manager.go(4 hunks)block/manager_test.go(4 hunks)buf.gen.yaml(1 hunks)buf.yaml(1 hunks)cmd/rollkit/commands/init.go(3 hunks)cmd/rollkit/commands/run_node.go(3 hunks)cmd/rollkit/commands/run_node_test.go(0 hunks)go.mod(3 hunks)node/full.go(5 hunks)node/full_node_integration_test.go(7 hunks)node/helpers_test.go(3 hunks)node/node.go(3 hunks)node/node_integration_test.go(4 hunks)node/node_test.go(3 hunks)pkg/config/config.go(4 hunks)pkg/config/config_test.go(1 hunks)pkg/config/defaults.go(2 hunks)pkg/signer/file/README.md(1 hunks)pkg/signer/file/doc.go(1 hunks)pkg/signer/file/example_test.go(1 hunks)pkg/signer/file/local.go(1 hunks)pkg/signer/noop/doc.go(1 hunks)pkg/signer/noop/signer.go(1 hunks)pkg/signer/noop/signer_test.go(1 hunks)pkg/signer/signer.go(1 hunks)proto/rollkit/v1/signer.proto(1 hunks)sequencers/single/go.mod(1 hunks)sequencers/single/queue.go(3 hunks)specs/lazy-adr/adr-017-node-pruning.md(14 hunks)types/hashing.go(2 hunks)types/pb/rollkit/v1/v1connect/signer.connect.go(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/rollkit/commands/run_node_test.go
🧰 Additional context used
🧬 Code Definitions (12)
node/node_integration_test.go (2)
pkg/signer/noop/signer.go (1)
NewNoopSigner(17-23)node/helpers.go (1)
InitFiles(129-145)
node/helpers_test.go (2)
pkg/signer/noop/signer.go (1)
NewNoopSigner(17-23)node/helpers.go (1)
InitFiles(129-145)
types/hashing.go (1)
types/header.go (1)
Hash(18-18)
cmd/rollkit/commands/init.go (2)
pkg/config/config.go (1)
FlagSignerPassphrase(113-113)pkg/signer/file/local.go (1)
NewFileSystemSigner(39-65)
pkg/signer/noop/signer.go (1)
pkg/signer/signer.go (1)
Signer(8-14)
pkg/signer/noop/signer_test.go (1)
pkg/signer/noop/signer.go (1)
NewNoopSigner(17-23)
pkg/signer/file/example_test.go (1)
pkg/signer/file/local.go (1)
NewFileSystemSigner(39-65)
node/node_test.go (2)
pkg/signer/noop/signer.go (1)
NewNoopSigner(17-23)node/helpers.go (1)
InitFiles(129-145)
pkg/config/config.go (2)
pkg/signer/signer.go (1)
Signer(8-14)types/pb/rollkit/v1/rollkit.pb.go (3)
Signer(294-302)Signer(315-315)Signer(330-332)
pkg/signer/file/local.go (1)
pkg/signer/signer.go (1)
Signer(8-14)
node/full.go (2)
pkg/signer/signer.go (1)
Signer(8-14)types/pb/rollkit/v1/rollkit.pb.go (3)
Signer(294-302)Signer(315-315)Signer(330-332)
node/full_node_integration_test.go (2)
pkg/signer/noop/signer.go (1)
NewNoopSigner(17-23)node/helpers.go (1)
InitFiles(129-145)
🪛 GitHub Actions: CI and Release
pkg/config/config_test.go
[error] 21-21: TestDefaultNodeConfig failed: Not equal: expected: true, actual: false.
proto/rollkit/v1/signer.proto
[error] 29-29: Format diff -1/+1. The file is missing a newline at the end of the file.
🪛 LanguageTool
specs/lazy-adr/adr-017-node-pruning.md
[grammar] ~890-~890: If ‘choose’ is used as a verb, it usually requires the infinitive.
Context: ...Management**: - Operators can choose pruning strategies that fit their needs - Su...
(AFFORD_VBG)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (100)
buf.gen.yaml (1)
11-12: New plugin input configuration added.The addition of the
inputssection specifying the "proto" directory aligns with modern Protocol Buffer tooling conventions. This change ensures the build system knows exactly where to find the proto definition files.specs/lazy-adr/adr-017-node-pruning.md (1)
114-116: Formatting improvements increase readabilityThe consistent bullet point styling (using dashes instead of asterisks) and improved indentation enhance the document's readability while maintaining the content's integrity.
Also applies to: 118-120, 127-129, 131-133, 137-141, 142-145, 147-150, 154-156, 158-161, 163-165, 169-171, 173-175, 177-180, 187-191, 192-195, 196-199, 200-203, 387-394, 399-421
sequencers/single/queue.go (3)
12-12: Import updated for Protocol Buffer handlingThe addition of the direct import for
google.golang.org/protobuf/protoaligns with the change in serialization approach.
49-49: Batch serialization updated to use the proto package directlyChanged from the previous approach (likely
pbBatch.Marshal()) to usingproto.Marshal(pbBatch), which follows the recommended Go Protocol Buffer API usage pattern.
112-112: Batch deserialization updated to use the proto package directlyChanged from the previous approach (likely
pbBatch.Unmarshal(result.Value)) toproto.Unmarshal(result.Value, pbBatch), which follows the recommended Go Protocol Buffer API usage pattern.pkg/signer/file/README.md (2)
1-78: Comprehensive documentation for the new FileSystem SignerThe documentation thoroughly covers the security features, usage patterns, and implementation details for the new secure filesystem-based signer implementation.
46-52: Good security considerations for production useThe documentation appropriately highlights important security considerations for production use, such as using stronger KDFs (Argon2 or PBKDF2) and implementing secure passphrase handling.
types/hashing.go (3)
5-6: Added import for hash interface support.The addition of the "hash" package is necessary to support the new
leafHashOptfunction's parameter type.
27-28: Improved hashing implementation.The change to use
leafHashOptwith the leaf prefix enhances the hashing approach for Data objects.
30-35: Well-structured hashing helper function.The
leafHashOptfunction is well-designed:
- It accepts a hash implementation, supporting flexibility
- It properly resets the hash state before use
- It follows best practices by prefixing leaf data
- It properly handles hash finalization
The implementation aligns with cryptographic best practices.
go.mod (3)
70-70: Added indirect dependency on gogo/protobuf.This is a natural dependency for using gogoproto and is correctly marked as indirect.
36-36:Details
✅ Verification successful
Direct dependency on golang.org/x/crypto added.
This matches the PR's intention to implement remote signing functionality, requiring direct access to cryptographic primitives rather than accessing them through CometBFT.
Verify the security status of this crypto package version:
🏁 Script executed:
#!/bin/bash # Check for known vulnerabilities in golang.org/x/crypto v0.33.0 curl -s "https://pkg.go.dev/vuln/GO-2023-2185" | grep -A 5 "golang.org/x/crypto v0.33.0" curl -s "https://pkg.go.dev/vuln/list" | grep -A 2 "golang.org/x/crypto"Length of output: 2145
Security Verification Complete: Direct Dependency on golang.org/x/crypto v0.33.0 Approved
- The addition of
golang.org/x/crypto v0.33.0ingo.mod(line 36) supports the PR’s goal of enabling remote signing by accessing low-level cryptographic primitives directly.- The vulnerability checks via pkg.go.dev did not flag any critical issues specific to this version for our typical usage.
- Note: While some generic vulnerability listings mention potential issues (e.g., panics on malformed inputs in 32-bit environments), these scenarios are not applicable in our case.
17-17:Details
❓ Verification inconclusive
Major dependency change: CometBFT replaced with gogoproto.
This change shifts from using the CometBFT framework to using the Cosmos gogoproto implementation directly.
Please verify that all CometBFT functionality that was previously utilized has appropriate replacements or alternatives in the new dependency structure.
🌐 Web query:
Are there any known compatibility issues between gogoproto v1.7.0 and cometbft?💡 Result:
There are known compatibility considerations between gogoproto and CometBFT (formerly Tendermint Core) due to differing Protobuf implementation approaches, though no direct incompatibilities are explicitly documented for v1.7.0. Key points:
Protobuf Versioning Constraints:
- gogoproto-generated code requires matching runtime library versions ([3][6]). CometBFT's ABCI message definitions in [12][18] use standard Protobuf types, but mixed gogoproto/golang-protobuf usage could cause serialization issues ([7][16]).
Code Generation Incompatibilities:
- The gogoproto fork introduces custom optimizations (like
gogoproto.nullableandgogoproto.customtypein [18]) that may conflict with CometBFT's expected Protobuf message formats if not properly aligned ([3][9]).Address Encoding Conflicts:
- CometBFT uses Bech32m encoding for addresses, while some Cosmos chains using gogoproto (like Noble in [9]) require Bech32. This could affect IBC integrations when serializing addresses in Protobuf messages ([9][12]).
Community Migration Trends:
- Major projects like Thanos and Prometheus have moved away from gogoproto due to maintenance concerns and compatibility challenges with standard Protobuf tooling ([7][16]). CometBFT's ecosystem is likely following similar patterns.
To mitigate risks:
# Ensure consistent Protobuf toolchain versions go get github.com/cosmos/gogoproto@v1.7.0 protoc --gogo_out=paths=source_relative:. -I=. -I=$GOPATH/src ./cometbft/**/*.protoAlways validate serialization/deserialization flows between gogoproto-enhanced types and CometBFT's native ABCI messages during integration testing.
Citations:
- 1: https://groups.google.com/g/golang-nuts/c/F5xFHTfwRnY
- 2: https://forum.devtalk.com/t/distributed-services-with-go-gogoproto-on-linux/823
- 3: https://github.com/gogo/protobuf
- 4: https://thanos.io/v0.31/proposals-accepted/202206-active-series-limiting-hashring.md/
- 5: https://groups.google.com/g/cel-go-discuss/c/6aj4Zz9iD7c
- 6: https://pkg.go.dev/github.com/gogo/protobuf/proto
- 7: https://groups.google.com/g/prometheus-developers/c/uFWRyqZaQis
- 8: https://deps.dev/go/github.com%2Fcosmos%2Fgogoproto/v1.7.0
- 9: https://forum.penumbra.zone/t/usdc-withdrawal-issues-and-proposed-fix/53
- 10: https://www.cisco.com/c/dam/en/us/td/docs/wireless/mse/release/notes/Cisco_Connected_Mobile_Experiences_CMX_Cisco_Release_10_3_0_v1_0.pdf
- 11: https://buf.build/cometbft/cometbft/file/4a62c99d422068a5165429b62a7eb824df46cca9:cometbft/types/v1/params.proto
- 12: https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types
- 13: gogo/protobuf#678
- 14: https://youtrack.jetbrains.com/issue/IJPL-64679/Improve-working-with-protobuf-imports
- 15: https://sourcegraph.com/github.com/cosmos/cosmos-sdk/-/blob/CHANGELOG.md
- 16: https://github.com/cosmos/gogoproto
- 17: https://chromium.googlesource.com/external/github.com/gogo/protobuf/
- 18: https://buf.build/evmos/evmos/file/1f4faf495310c7951777883433ea6d23fd902c44:ethermint/evm/v1/tx.proto
- 19: https://buf.build/cosmos/interchain-security/docs/41604d95568c430eaaceb315deb26cca:interchain_security.ccv.v1
Verify Protobuf Compatibility with the gogoproto Migration
The change to
github.com/cosmos/gogoproto v1.7.0replaces CometBFT with the Cosmos gogoproto implementation. While no direct incompatibilities have been explicitly documented for v1.7.0, known compatibility considerations—such as differences in Protobuf versioning, code generation (e.g., handling ofgogoproto.nullableand custom types), and address encoding—may impact components that previously relied on CometBFT’s Protobuf definitions.
- Protobuf Runtime & Code Generation: Ensure that the Protobuf runtime versions are consistent and that any code generation differences do not break serialization of CometBFT’s previously used message formats.
- Address Encoding: Verify that any discrepancies between Bech32 and Bech32m encoding do not affect the functionality, particularly for IBC integrations.
- Integration Testing: It's crucial to run thorough integration tests to confirm that all CometBFT functionality has an appropriate replacement in this new dependency structure.
Please review the integration points where CometBFT functionality was used and validate that these areas perform as expected with gogoproto v1.7.0.
pkg/signer/noop/doc.go (1)
1-7: Clear documentation for the no-op signer package.The documentation clearly explains the purpose of this package as implementing a no-op signer for testing and development scenarios. This aligns well with the PR objective of adding remote signing capabilities.
pkg/signer/signer.go (1)
3-5: Appropriate import for cryptographic functionality.The import of
github.com/libp2p/go-libp2p/core/cryptois correct for utilizing the libp2p cryptographic types, particularlycrypto.PubKey.pkg/config/config_test.go (1)
91-91: Updated expected flag count correctlyThe change from 36 to 39 flags accurately reflects the addition of three new flags related to remote signing functionality (likely
FlagSignerType,FlagSignerPath, andFlagSignerPassphrase).pkg/config/defaults.go (2)
78-81:Details
❓ Verification inconclusive
Good addition of default signer configuration
Adding default signer configuration with
SignerType: "file"and an emptySignerPathmakes the default signing mechanism explicit. This aligns well with the PR's objective of adding remote signing capabilities.Please ensure that the code handles the case of an empty
SignerPathgracefully, either by:
- Generating a default path
- Providing clear error messages if a path is required
- Documenting how this default configuration behaves
🏁 Script executed:
#!/bin/bash # Check how the code handles empty SignerPath rg -A 10 "SignerPath.*=="Length of output: 27
Action Required: Verify Empty
SignerPathHandlingThe default configuration now explicitly sets
SignerPathto an empty string, which aligns with the PR’s objective by clearly exposing the default signer behavior. However, our automated grep test did not reveal any code handling the empty value. Please manually verify that the code gracefully manages an emptySignerPath(e.g., by auto-generating a default path, issuing a clear error when a valid path is expected, or providing appropriate documentation on its handling).
- Location:
pkg/config/defaults.go(lines 78–81)- Verification Needed: Confirm downstream components either resolve an empty
SignerPathcorrectly or emit a helpful error message, or that the behavior is well documented.
55-55:Details
❓ Verification inconclusive
Breaking change: Default Aggregator value changed from true to false
Changing the default value of
Aggregatorfromtruetofalseis a breaking change that affects backward compatibility. This causes the test failure inTestDefaultNodeConfig.Is this change intentional? If so, it should be documented as a breaking change in the release notes. Users upgrading to this version may experience different behavior if they rely on the default aggregator setting.
Below are two shell scripts to help verify the change in the default value of
Aggregatorand its impact on tests. The first script shows the surrounding context inpkg/config/defaults.go(including line 55), and the second searches for references toTestDefaultNodeConfigin the codebase so we can see what default value the tests expect.Script 1: Show context of
pkg/config/defaults.goaround line 55Script 2: Find references to
TestDefaultNodeConfigin the repositoryOnce we have the output from these queries, we can better determine whether the change (from
truetofalse) in the default value ofAggregatorwas intentional. If intentional, the breaking change must be clearly documented in the release notes to warn users of the changed behavior.
🏁 Script executed:
#!/bin/bash # Display lines 50 to 60 from pkg/config/defaults.go for context. sed -n '50,60p' pkg/config/defaults.goLength of output: 480
🏁 Script executed:
#!/bin/bash # Search for the TestDefaultNodeConfig reference to see what default value is expected. rg "TestDefaultNodeConfig" -n .Length of output: 227
Breaking Change: Default Aggregator Behavior
- The code in
pkg/config/defaults.go(line 55) now setsAggregatortofalse.- The test
TestDefaultNodeConfig(located inpkg/config/config_test.go) is failing—likely because it expects the default value to betrue.- If this change to a default value of
falseis intentional, please update the release notes and tests accordingly to document the breaking change and ensure users are aware of the altered behavior.- If not intentional, consider reverting the default back to
trueor adjusting the test expectations.node/node_integration_test.go (4)
21-21: Good addition of remote signer package importAdding the import for the noop signer implementation is necessary for the integration test to use the new signer interface.
48-49: Good migration to signer interfaceCreating a NoopSigner from the existing validator key properly implements the transition from directly using private keys to using the
signer.Signerinterface.
59-60: Good code cleanupReusing the existing
errvariable instead of redeclaring it improves code consistency.
68-68: Properly updated function parameterChanging from passing
genesisValidatorKeydirectly to passing theremoteSigneris consistent with the PR's goal of switching to the signer interface.node/helpers_test.go (4)
16-16: Added import for the NoopSigner implementation.This import adds the necessary functionality to use the NoopSigner implementation, aligning with the refactoring from direct crypto key usage to a signer interface pattern.
48-49: Implemented remote signer with proper error handling.The code now creates a NoopSigner instead of directly using the crypto key, which is a good abstraction that allows for future flexibility with different signer implementations. The error handling is properly implemented with require.NoError.
56-57: Fixed error variable reuse.The error handling now correctly reuses the existing err variable instead of redeclaring it, avoiding variable shadowing issues.
65-65: Updated function parameter to use the signer interface.The node initialization now uses the remoteSigner interface instead of directly passing the private key, which aligns with the broader architectural shift toward abstraction of signing mechanisms.
node/full_node_integration_test.go (9)
20-20: Added import for NoopSigner implementation.This import adds the necessary functionality to use the NoopSigner implementation across the integration tests.
66-67: Implemented remote signer in SetupTest with proper error handling.The test setup now creates a NoopSigner instead of directly using the crypto key, with appropriate error handling via require.NoError.
74-75: Fixed error variable reuse in SetupTest.The error handling now correctly reuses the existing err variable instead of redeclaring it, avoiding variable shadowing issues.
83-83: Updated NewNode parameter to use the signer interface in SetupTest.The node initialization now correctly uses the remoteSigner interface instead of directly passing the private key.
320-321: Implemented remote signer in TestMaxPending with proper error handling.The test now creates a NoopSigner instead of directly using the crypto key, with appropriate error handling.
328-329: Fixed error variable reuse in TestMaxPending.The error handling now correctly reuses the existing err variable instead of redeclaring it.
337-337: Updated NewNode parameter to use the signer interface in TestMaxPending.The node initialization now correctly uses the remoteSigner interface instead of directly passing the private key.
406-407: Implemented remote signer in TestStateRecovery with proper error handling.The test now creates a NoopSigner instead of directly using the crypto key, with appropriate error handling.
420-420: Updated NewNode parameter to use the signer interface in TestStateRecovery.The node initialization now correctly uses the remoteSigner interface instead of directly passing the private key.
buf.yaml (3)
1-1: Updated buf version to v2.The buf tool configuration has been updated to use the newer v2 format, which is a necessary update to support the latest features and protocol buffer compilation mechanisms.
3-4: Restructured configuration with modules section.The configuration now uses the modules-based approach instead of the older build section, which better aligns with buf v2's architecture. This change specifies the proto directory as the module path.
8-8: Added file naming convention rule.The FILE_LOWER_SNAKE_CASE lint rule has been added, which enforces consistent file naming for protocol buffer files. This is a good practice that improves code organization.
sequencers/single/go.mod (1)
19-19:Details
✅ Verification successful
Added direct dependency on google.golang.org/protobuf.
The project now directly depends on Google's official protobuf implementation rather than including it as an indirect dependency. This change aligns with the updated buf.yaml configuration and supports the shift to using protobuf-based signer implementations.
🏁 Script executed:
#!/bin/bash # Verify if this protobuf package is being used in the codebase # Check for imports of google.golang.org/protobuf in the repository rg -l "google\.golang\.org/protobuf" --type goLength of output: 472
Protobuf Direct Dependency Added and Verified
The direct dependency on
google.golang.org/protobufis indeed used throughout the codebase. Our verification confirms that this package is imported in multiple files (e.g., withintypes/serialization.go,types/pb/rollkit/v1/*.go, among others). This change aligns correctly with the updated buf.yaml configuration and supports the transition to protobuf-based signer implementations.pkg/signer/noop/signer.go (3)
9-14: The NoopSigner struct looks good with appropriate fieldsThe structure properly implements the signer.Signer interface with the necessary fields to store private and public keys.
25-28: LGTM - Sign method properly delegates to the underlying keyThe Sign method correctly implements the Signer interface by delegating to the private key's Sign method.
30-33: LGTM - GetPublic method implementation is cleanThe GetPublic method correctly implements the Signer interface by returning the stored public key.
node/node_test.go (4)
23-23: LGTM - Import for noop signer added correctlyThe import for the remote signer is added correctly, using a descriptive alias to distinguish it from other packages.
183-184: Proper initialization of the remote signerThe NoopSigner is properly created using the genesis validator key and tested for errors.
191-191: Good refactoring to avoid variable shadowingChanged from using
:=to=to reuse the existing error variable instead of shadowing it.
202-202: Proper replacement of direct key usage with signer interfaceThe code now correctly uses the remoteSigner interface instead of directly passing the private key.
pkg/signer/file/example_test.go (2)
12-42: LGTM - Well-structured test for FileSystemSignerThe test thoroughly covers key generation, file creation, signing, and verification.
61-65: LGTM - Good error case testingThe test properly checks the behavior with an incorrect passphrase, ensuring robust error handling.
pkg/signer/noop/signer_test.go (3)
10-48: LGTM - Comprehensive test cases for the basic signer functionalityThe tests cover creation, key retrieval, and signing/verification - all essential operations for a signer implementation.
50-74: LGTM - Good test for key uniquenessThis test ensures that different signer instances created with different private keys have unique public keys, which is an important security property.
76-95: LGTM - Well-designed negative test caseTesting that verification fails with an incorrect message is an important security check that demonstrates the signer behaves correctly in failure scenarios.
cmd/rollkit/commands/init.go (2)
11-11: LGTM: New import for file signer package.Added import for the file signer package to support local file signing initialization.
107-110: LGTM: New flag for signer passphrase.The initialization function adds a flag for the signer passphrase, which is required when using the local file signer.
pkg/config/config.go (4)
104-114: LGTM: New signer configuration flags.Added appropriate constants for signer configuration flags with clear comments.
167-169: LGTM: Added signer field to Config struct.The Config struct now includes a Signer field of type SignerConfig, properly annotated with mapstructure and yaml tags.
220-224: LGTM: New SignerConfig struct.The SignerConfig struct is well-defined with appropriate fields and clear comments. It provides enough flexibility to support different signer types.
289-292: LGTM: Added signer flags to command.The AddFlags function now includes the new signer configuration flags, properly initialized with default values.
node/node.go (3)
14-14: LGTM: Updated import to include signer package.Replaced the crypto.PrivKey import with the new signer package import.
31-31: Good refactoring to use signer.Signer interface.The parameter type has been changed from crypto.PrivKey to signer.Signer, enhancing modularity and flexibility by using an interface instead of a concrete type.
43-43: LGTM: Updated parameter name in function call.The parameter name has been consistently updated from signingKey to signer in the newFullNode function call.
block/manager_test.go (9)
27-28: Add signer & no-op signer imports
These imports introduce the new abstract signer interface and the no-op implementation.
174-174: Initialize test payload
Defining a straightforward payload for signing tests. Looks fine.
178-180: Generate keypair and wrap in NoopSigner
Generating an Ed25519 keypair and instantiating a NoopSigner around it is consistent with the updated signing approach. Error handling is properly enforced.
183-186: Refactor test case to use signer
The table-driven test now uses a single signer field. This aligns well with the new interface-based design.
190-190: Assign signer to m.proposerKey
Replacing the direct private key assignment with a signer-based approach.
193-194: Public key retrieval and verification
The code correctly retrieves the public key from the signer and verifies the signature.
365-365: Update signerPrivKey to signer.Signer
Adopting the signer interface consistently inTest_isProposer.
380-381: Wrap generated key with NoopSigner
Again, the private key is wrapped with NoopSigner for testing. Error checks look good.
384-384: Include signer in args
Passing the signer further standardizes the updated interfaces.block/manager.go (4)
28-28: Introduce signer import
Importing the signer package lays groundwork for using the abstract signer interface.
110-110: Replace proposerKey with signer.Signer
Using a signer field in the Manager struct decouples cryptographic details from direct private key usage.
224-224: Adjust NewManager to accept signer
Swapping out the private key parameter for asigner.Signerfosters a cleaner abstraction.
397-397: Stubbed isProposer function
This function unconditionally returnstrue, which may not be the final desired logic. Please verify that this placeholder is appropriate.Would you like to confirm if a real proposer check is required?
node/full.go (3)
28-28: Add signer package import
Importing the signer interface for node usage.
71-71: Use signer.Signer in newFullNode
Replaces a private key parameter with the higher-level signer interface, simplifying key handling.
106-106: Forward signer to initBlockManager
Passing the signer object ensures all signing flows align with the new interface.cmd/rollkit/commands/run_node.go (5)
26-27: Add signer & file-based signer imports
Bringing in the signer interface and file-based signer helps implement multiple signing modes.
59-66: Enforce passphrase for aggregator + file signer
Ensures aggregator nodes with a file-based signer specify a non-empty passphrase, preventing accidental misconfiguration.
70-70: Instantiate the direct KV executor
Sets up a local test executor for demonstration or development.
75-75: Log executor address
Provides debug information. This can be helpful in diagnosing runtime issues.
106-106: Pass signer to NewNode
Ensures that the node is constructed with the correct signing interface.types/pb/rollkit/v1/v1connect/signer.connect.go (7)
1-20: All generated code for Connect appears standard and consistent.The constant checks for Connect version compatibility and the package references look correct. No issues spotted here.
21-41: Service name constants and RPC procedure definitions are properly exposed.They align well with the declared proto definitions.
42-48: Client interface and method signatures are clear and self-documenting.No immediate concerns regarding parameter or return types. The design seems appropriately minimal.
50-75: Client factory function constructs separate clients for each RPC.Everything looks properly trimmed and validated. The usage of reflection-based method schemas is an effective approach in Connect for strong RPC definitions.
76-91: Client method implementations delegate to connect.NewClient.Calls appear straightforward. Returning the correct response types, with no extraneous logic.
92-129: Handler interface and HTTP handler construction logic are well-structured.The multi-protocol support via Connect’s unary handlers with GCM support is standard. The switch-based dispatching on method paths is correct and easily maintainable.
131-141: Unimplemented handler ensures that unimplemented RPCs raise CodeUnimplemented.This is standard for stubbed server implementations.
pkg/signer/file/local.go (12)
1-19: Struct and import organization look good.The
FileSystemSignerstruct and concurrency primitives are logically placed. Sufficient external dependencies are imported for cryptographic operations.
20-28: Mutex usage approach is appropriate.Using a read-write mutex for reading/writing keys appears consistent with the code’s locking logic.
29-36: Key data structure is adequately defined for on-disk storage.The optional salt aligns with backward-compatibility needs.
37-65: Key generation and initial storage logic is well-organized.
- The code creates a directory if none exists and correctly sets secure permissions.
- Generating a new key if the file does not exist is a sensible default.
- Wiping passphrase memory afterward is a best-effort approach given Go’s garbage collection limitations.
91-117: Secure key handling logic is robust; Argon2 with random salt is recommended.
- Storing the salt and ephemeral data in JSON with restricted file permissions (0600) is good.
- Carefully zeroing passphrase and derived keys is an additional layer of security.
131-141: Key file load approach is consistent with the save logic.You properly unmarshal JSON, handle backward compatibility for missing salts, and zero out sensitive data.
142-169: AES-GCM usage is correct.Nonce generation is performed from a secure random source. Good job ensuring no encryption re-use occurs.
220-235: Raw key unmarshaling for Ed25519 is straightforward.The code sets private and public keys in memory and zeroes out ephemeral data.
237-247: Signing logic aligns with the Signer interface.Returning an error if
privateKeyis nil is a good guard.
249-259: Public key retrieval is consistent.Again, a nil-check is in place. This ensures correct usage.
261-269: Argon2 parameters are in a reasonable range for security.32MB memory might be large for constrained environments but is a good default for desktop/server usage.
286-291: Memory zeroing is an excellent extra layer, though not fully guaranteed in Go.Still a good practice for limiting exposure in memory.
| syntax = "proto3"; | ||
|
|
||
| package rollkit.v1; | ||
|
|
||
| option go_package = "github.com/rollkit/rollkit/types/pb/rollkit/v1"; | ||
|
|
||
| // The SignRequest holds the bytes we want to sign. | ||
| message SignRequest { | ||
| bytes message = 1; | ||
| } | ||
|
|
||
| // The SignResponse returns the signature bytes. | ||
| message SignResponse { | ||
| bytes signature = 1; | ||
| } | ||
|
|
||
| // The GetPublicRequest is an empty request. | ||
| message GetPublicRequest {} | ||
|
|
||
| // The GetPublicResponse returns the public key. | ||
| message GetPublicResponse { | ||
| bytes public_key = 1; | ||
| } | ||
|
|
||
| // The SignerService defines the RPCs to sign and to retrieve the public key. | ||
| service SignerService { | ||
| // Sign signs the given message. | ||
| rpc Sign(SignRequest) returns (SignResponse); | ||
| // GetPublic returns the public key. | ||
| rpc GetPublic(GetPublicRequest) returns (GetPublicResponse); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Good protocol definition, but missing a newline at the end of file.
The protocol definition for the signer service is well-structured with clear documentation for each message and RPC method. The service provides the essential functionality needed for remote signing.
However, there's a pipeline failure indicating a missing newline at the end of the file.
service SignerService {
// Sign signs the given message.
rpc Sign(SignRequest) returns (SignResponse);
// GetPublic returns the public key.
rpc GetPublic(GetPublicRequest) returns (GetPublicResponse);
}
+📝 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.
| syntax = "proto3"; | |
| package rollkit.v1; | |
| option go_package = "github.com/rollkit/rollkit/types/pb/rollkit/v1"; | |
| // The SignRequest holds the bytes we want to sign. | |
| message SignRequest { | |
| bytes message = 1; | |
| } | |
| // The SignResponse returns the signature bytes. | |
| message SignResponse { | |
| bytes signature = 1; | |
| } | |
| // The GetPublicRequest is an empty request. | |
| message GetPublicRequest {} | |
| // The GetPublicResponse returns the public key. | |
| message GetPublicResponse { | |
| bytes public_key = 1; | |
| } | |
| // The SignerService defines the RPCs to sign and to retrieve the public key. | |
| service SignerService { | |
| // Sign signs the given message. | |
| rpc Sign(SignRequest) returns (SignResponse); | |
| // GetPublic returns the public key. | |
| rpc GetPublic(GetPublicRequest) returns (GetPublicResponse); | |
| } | |
| syntax = "proto3"; | |
| package rollkit.v1; | |
| option go_package = "github.com/rollkit/rollkit/types/pb/rollkit/v1"; | |
| // The SignRequest holds the bytes we want to sign. | |
| message SignRequest { | |
| bytes message = 1; | |
| } | |
| // The SignResponse returns the signature bytes. | |
| message SignResponse { | |
| bytes signature = 1; | |
| } | |
| // The GetPublicRequest is an empty request. | |
| message GetPublicRequest {} | |
| // The GetPublicResponse returns the public key. | |
| message GetPublicResponse { | |
| bytes public_key = 1; | |
| } | |
| // The SignerService defines the RPCs to sign and to retrieve the public key. | |
| service SignerService { | |
| // Sign signs the given message. | |
| rpc Sign(SignRequest) returns (SignResponse); | |
| // GetPublic returns the public key. | |
| rpc GetPublic(GetPublicRequest) returns (GetPublicResponse); | |
| } | |
🧰 Tools
🪛 GitHub Actions: CI and Release
[error] 29-29: Format diff -1/+1. The file is missing a newline at the end of the file.
4 participants
closes #1125
add remote signing as an option.
This PR consists of:
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores