fix: nonce_mismatch log parsing #4067
Conversation
InoMurko
requested review from
rootulp and
evan-forbes
and removed request for
a team
📝 WalkthroughWalkthroughThe changes involve a modification to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
User->>System: Trigger nonce mismatch
System->>System: Call ParseExpectedSequence
System->>System: Check error message with strings.Contains
alt Error message matches
System-->>User: Return expected sequence number
else Error message does not match
System-->>User: Handle error
end
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
🧹 Outside diff range and nitpick comments (1)
app/errors/nonce_mismatch.go (1)
Line range hint
35-45: Consider making the sequence number parsing more robustThe current implementation makes several assumptions that could make it fragile:
- It assumes the first number is always the expected sequence
- It discards the actual sequence number which could be useful for debugging
- The error message format might vary across different SDK versions
Consider enhancing the implementation to:
- Verify the numbers appear after "expected" and "got" keywords
- Return both expected and actual sequence numbers
- Add version-specific message handling
func ParseExpectedSequence(str string) (uint64, error) { if !strings.Contains(str, "incorrect account sequence") { return 0, fmt.Errorf("unexpected wrong sequence error: %s", str) } numbers := regexpInt.FindAllString(str, -1) if len(numbers) != 2 { return 0, fmt.Errorf("expected two numbers in string, got %d", len(numbers)) } - // the first number is the expected sequence number - return strconv.ParseUint(numbers[0], 10, 64) + // Find the position of "expected" and "got" to ensure correct number mapping + expectedIdx := strings.Index(str, "expected") + gotIdx := strings.Index(str, "got") + + if expectedIdx == -1 || gotIdx == -1 || expectedIdx > gotIdx { + return 0, fmt.Errorf("malformed sequence error message: %s", str) + } + + expected, err := strconv.ParseUint(numbers[0], 10, 64) + if err != nil { + return 0, fmt.Errorf("failed to parse expected sequence: %w", err) + } + + // Store actual sequence in error message for better debugging + actual, err := strconv.ParseUint(numbers[1], 10, 64) + if err != nil { + return 0, fmt.Errorf("failed to parse actual sequence: %w", err) + } + + return expected, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/errors/nonce_mismatch.go(1 hunks)
🔇 Additional comments (2)
app/errors/nonce_mismatch.go (2)
35-37: LGTM! The change correctly addresses the error message format.
The switch from HasPrefix to Contains is appropriate given the actual error message format shown in the PR description.
Line range hint 41-44: Verify the regex implementation
The code relies on a global regexpInt variable that's not visible in the provided context. Let's verify its implementation to ensure it correctly handles the sequence numbers.
✅ Verification successful
The regex implementation is correctly defined and used
The regexpInt variable is properly defined in app/errors/insufficient_gas_price.go with the pattern [0-9]+ to match one or more digits. This implementation is appropriate for extracting sequence numbers from error messages, as it:
- Correctly matches consecutive digits
- Is used consistently across both files
- Handles the expected format of having two numbers in the error message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the regexpInt definition
rg -A 2 'regexpInt.*='
# Look for any regex-related imports
rg '^import.*regexp'
Length of output: 211
Script:
#!/bin/bash
# Let's get more context around the regex usage in both files
rg -B 2 -A 5 'regexpInt' app/errors/
# Check if there are any other regex patterns defined
rg 'regexp\.MustCompile' app/errors/
# Get the full content of both files to understand the context
cat app/errors/nonce_mismatch.go
cat app/errors/insufficient_gas_price.go
Length of output: 6068
InoMurko
changed the title
|
hmm the existing error message parsing seems fragile but why would it not work? If the error is: Before we change the behavior here, I think we need unit test coverage for the expected error messages. |
The error starts with: 'broadcast tx error: ' |
Is that just when the error gets bubbled up to celestia-node? I think that at the point where this error parsing is happening, the error message is just: |
This is how the flow goes. celestia-app/pkg/user/tx_client.go Line 261 in 2ca0feb celestia-app/pkg/user/tx_client.go Line 299 in 2ca0feb celestia-app/pkg/user/tx_client.go Lines 388 to 392 in 2ca0feb broadcast tx error: celestia-app/pkg/user/tx_client.go Line 69 in 2ca0feb Given that this piece of code (ParseExpectedSequence, ParseNonceMismatch) is not used and given that a test still exists that did not fail |
|
@rootulp let me know if a unit test is needed and if you have any particular flow you'd want to test, given above info! |
This PR changes the behavior of Do you have concrete steps on how I can repro the issue you're observing? It would be helpful for me to repro if you created a bug report issue. Then I can try to review + fix today. |
Haha, good one! This piece of code in PR I tried to use at this point in the code Our fork has added handling nonces in a similar way that celestia-node handles gas. It checks the network message returned and adjusts the gas. We handle nonces at the same place. Currently it looks like this: Hopefully that makes more sense now! |
We can only support on official releases from @celestiaorg here, FWIW. |
Understood. The purpose here is to close the gap. |
|
I think it would make sense to contribute those changes to celestia-node so that others can use it and so it can be upstreamed to prevent you from running a fork. have you made a PR? |
Not yet, ran them over @renaynay (Rene Lubov) and was waiting for feedback. |
|
Update: I wonder if celestiaorg/celestia-core#1553 resolves the underlying issue and this PR is no longer necessary 🤔 |
The actual error log that would bubble up is actually (using this in celestia-node)
So seeking a prefix for
account sequence mismatchwould not work.