fix(txsim): improved grpc server discovery, added default keypath value #4340
Conversation
zatarx
requested review from
cmwaters and
rach-id
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant KeyringManager
User->>CLI: Execute txsim command
alt Keyring provided
CLI->>KeyringManager: Use provided keyring
else Default keyring
CLI->>KeyringManager: Initialize keyring with default node home
end
CLI->>CLI: Process command with --feegrant flag
CLI-->User: Return execution result
sequenceDiagram
participant Run
participant buildGrpcConn
participant gRPCServer
Run->>buildGrpcConn: Call with endpoint and TLS config
buildGrpcConn->>gRPCServer: Attempt TLS handshake
alt Handshake succeeds
gRPCServer-->>buildGrpcConn: Secure connection established
else Handshake fails
buildGrpcConn->>gRPCServer: Fallback to insecure credentials
end
buildGrpcConn-->>Run: Return gRPC connection
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/txsim/run.go (2)
30-30: Consider clarifying the intent of settingInsecureSkipVerify: true.Having
InsecureSkipVerifyenabled avoids server certificate validation, which is a security concern in production. If this code is strictly for testing or local development, it would be good to add a clarifying comment indicating that this setting is not for production usage.var defaultTLSConfig = &tls.Config{ - InsecureSkipVerify: true + InsecureSkipVerify: true, // For testing only; not recommended in production }
178-206: Recommend adding a health check or domain verification before falling back to insecure credentials.The new
buildGrpcConnfunction attempts a TLS handshake, logs a warning on failure, and then falls back toinsecure.NewCredentials(). While this is convenient, consider:
- Performing a simple health check (e.g., verifying that the endpoint is actually gRPC capable) rather than defaulting to insecure on any TLS handshake issue.
- Logging clarity: including the endpoint domain or IP in the warning for easier troubleshooting.
- Possibly verifying domain identity if the environment is not strictly local or ephemeral.
test/cmd/txsim/cli_test.go (1)
56-78: Good coverage for the new default keypath usage.This new test,
TestTxsimDefaultKeypath, validates the fallback to default keyring behavior. Consider adding a negative test (e.g., verifying error handling if the default home directory is not writable or absent). That would ensure robust coverage of edge cases around key generation and usage.test/cmd/txsim/cli.go (1)
88-88: Fallback to default node home is user friendly.Changing to a default key path (
app.DefaultNodeHome) when neither--key-pathnor environment variables are provided is helpful for local or dev usage. Confirm that production deployments set an explicit key path to avoid accidental usage of this fallback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/cmd/txsim/cli.go(2 hunks)test/cmd/txsim/cli_test.go(1 hunks)test/txsim/run.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
test/txsim/run.go (2)
5-9: Imports look appropriate.The added imports (
crypto/tlsandnet) are necessary to establish manual TCP/TLS connections before handing control to gRPC. This approach is logical for checking handshake validity. No immediate issues here.
53-55: Error message is clear and concise.The initialization of
connviabuildGrpcConnis appropriately handled, and the returned error is well-wrapped. Good job ensuring the user sees the gRPC endpoint in the failure message.test/cmd/txsim/cli.go (1)
65-67: Documentation updates look good.The help text now correctly references
TXSIM_KEYPATHand includes the--feegrantflag in the usage example, improving discoverability for new features.
Yes. I want a way to more easily fill blocks to their max capacity. Currently Celestia blocks can contain 8 MiB of blob data but each individual blob is limited to ~2 MiB. This means that a user needs to submit 4 x ~2 MiB (or slightly smaller) blobs in order to fill a block. Even when I tried doing that via txsim, I wasn't able to consistently fill blocks so there may be other issues needed to accomplish my goal. You can skip this and/or extract it to a new issue so that it doesn't block the work in this PR. |
test/cmd/txsim/cli_test.go
Outdated
| t.Log("start test") | ||
| t.Log(grpcAddr) |
There was a problem hiding this comment.
Should these be deleted? They only seem helpful ad development time.
| t.Log("start test") | |
| t.Log(grpcAddr) |
rootulp
changed the title
test/txsim/run.go
Outdated
| @@ -170,3 +173,34 @@ func (o *Options) WithPollTime(pollTime time.Duration) *Options { | |||
| o.pollTime = pollTime | |||
| return o | |||
| } | |||
|
|
|||
| // GRPC applies the config if the handshake succeeds; otherwise, it falls back to an insecure connection. | |||
There was a problem hiding this comment.
| // GRPC applies the config if the handshake succeeds; otherwise, it falls back to an insecure connection. | |
| // buildGrpcConn applies the config if the handshake succeeds; otherwise, it falls back to an insecure connection. |
|
I noticed that test account keyring backend is |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/cmd/txsim/cli_test.go (1)
57-87: Improve test robustness and cleanup.Consider these improvements to make the test more robust:
- Use
deferfor account cleanup to ensure it runs even if the test fails- Add assertions to verify the account was created successfully
- Consider reducing the timeout from 10 seconds if the test typically completes faster
Here's a suggested refactor:
func TestTxsimDefaultKeypath(t *testing.T) { _, _, grpcAddr := setup(t) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() cdc := encoding.MakeConfig(app.ModuleEncodingRegisters...).Codec kr, err := keyring.New(app.Name, keyring.BackendTest, app.DefaultNodeHome, nil, cdc) if err != nil { t.Fatal("Keyring failed with", err) } - kr.NewAccount(testfactory.TestAccName, testfactory.TestAccMnemo, "", "", hd.Secp256k1) + err = kr.NewAccount(testfactory.TestAccName, testfactory.TestAccMnemo, "", "", hd.Secp256k1) if err != nil { t.Fatal("NewAccount failed with", err) } + defer func() { + if err := kr.Delete(testfactory.TestAccName); err != nil { + t.Errorf("Failed to delete test account: %v", err) + } + }() + + // Verify account was created + info, err := kr.Key(testfactory.TestAccName) + require.NoError(t, err) + require.Equal(t, testfactory.TestAccName, info.Name) cmd := command() cmd.SetArgs([]string{ "--blob", "1", "--grpc-endpoint", grpcAddr, "--seed", "1223", "--poll-time", "1s", "--feegrant", }) err = cmd.ExecuteContext(ctx) - - // NewAccount is not idempotent, Delete handles teardown - kr.Delete(testfactory.TestAccName) require.NoError(t, err) }🧰 Tools
🪛 golangci-lint (1.62.2)
68-68: Error return value of
kr.NewAccountis not checked(errcheck)
85-85: Error return value of
kr.Deleteis not checked(errcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/cmd/txsim/cli_test.go(2 hunks)test/txsim/run.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/txsim/run.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
test/cmd/txsim/cli_test.go
68-68: Error return value of kr.NewAccount is not checked
(errcheck)
85-85: Error return value of kr.Delete is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
test/cmd/txsim/cli_test.go (1)
15-15: LGTM!The new import is required for using
hd.Secp256k1in the new test function and is correctly placed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/cmd/txsim/cli_test.go (1)
85-85:⚠️ Potential issueCheck error return from Delete.
The error return from
kr.Deleteshould be checked to ensure proper cleanup.Apply this diff to add error handling:
- kr.Delete(testfactory.TestAccName) + if err := kr.Delete(testfactory.TestAccName); err != nil { + t.Errorf("Failed to delete test account: %v", err) + }🧰 Tools
🪛 golangci-lint (1.62.2)
85-85: Error return value of
kr.Deleteis not checked(errcheck)
🧹 Nitpick comments (1)
test/cmd/txsim/cli_test.go (1)
57-87: Add a test function comment.Please add a comment explaining the purpose of this test function, particularly what aspects of the default keypath behavior and new flags it verifies.
Apply this diff to add documentation:
+// TestTxsimDefaultKeypath verifies that the txsim command works correctly with +// the default keypath and new flags (--poll-time and --feegrant). func TestTxsimDefaultKeypath(t *testing.T) {🧰 Tools
🪛 golangci-lint (1.62.2)
85-85: Error return value of
kr.Deleteis not checked(errcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/cmd/txsim/cli_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
test/cmd/txsim/cli_test.go
85-85: Error return value of kr.Delete is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
test/cmd/txsim/cli_test.go (1)
15-15: LGTM!The new import is correctly placed and necessary for using the
Secp256k1key type in the new test.
|
Looking at the ci right now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
168-169: Updated Test Skip List for Race ModeThe updated
-skipflag now excludesTestTxsimDefaultKeypath(along with several other tests) during race condition testing. This change aligns with the PR objective of addressing the new default keypath functionality while isolating tests that may cause race issues. For future maintainability, consider adding an in-code comment or extracting the skip test list into a Makefile variable to ease updates as more tests are added or modified.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
|
@rootulp could you trigger the ci please? If there's anything else that needs to be done within this PR, lmk |
Overview
Fixes #4223
--feegrantflag to the example of txsim.buildGRPCConnnow filters out most of the invalid URLs, but if for example google.com:443 is provided, then it passes thru the function. IMO, healthcheck grpc endpoint must be introduced to solve this item (mb thru a separate issue/adr).buildGrpcConnaren't failing anymore.