Validate txid length when parsing#1115
Conversation
Add a dedicated helper that rejects any chainhash string unless it is exactly 64 hex characters long, instead of relying on btcd's lenient parser semantics. Cover the helper with focused tests for valid, short, odd-length, empty, and non-hex inputs so later call sites can use it as a full-txid validation primitive.
Reject short skipped transaction IDs before passing them into the batcher.
Replace the local outpoint parser with wire.NewOutPointFromString. This switches outpoint validation to btcd's canonical parser, which rejects truncated txids and invalid output indexes.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of transaction ID parsing and database interaction across the application. By replacing permissive parsing functions with strict validation, it ensures data integrity when handling transaction hashes. Additionally, it enhances error handling in batch processing and clarifies the API behavior regarding pending withdrawals, ensuring a more predictable and stable system state. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces strict transaction ID parsing across the codebase using a new utility, chainhashutil.NewHashFromStrExact, which ensures that hex strings are exactly 64 characters long to prevent truncated hashes from being accepted. Additionally, the PR updates the static address withdrawal system to support pending states, allowing the transaction ID to be null or nil and documenting that associated fields will remain zero until confirmation. Database retrieval logic in several stores was refactored to handle these nullable fields and improve recovery robustness. Feedback was provided to ensure consistency in the withdrawal store by explicitly checking for empty strings when reading transaction IDs from the database.
| if err != nil { | ||
| return nil, err | ||
| var txID *chainhash.Hash | ||
| if w.WithdrawalTxID.Valid { |
There was a problem hiding this comment.
For consistency and robustness, this check should also include a validation that the string is not empty, similar to the changes made in staticaddr/loopin/sql_store.go and sweepbatcher/store.go. The PR description mentions that SQL code is fragile and empty strings should be treated as NULL in critical parts. If an empty string exists in the database, chainhashutil.NewHashFromStrExact will return an error due to incorrect length, potentially breaking the recovery or listing of withdrawals.
| if w.WithdrawalTxID.Valid { | |
| if w.WithdrawalTxID.Valid && w.WithdrawalTxID.String != "" { |
Use strict txid parsing when reading swap updates and static address loop-ins from SQL rows. Empty swap update txids still mean "absent", but any non-empty truncated value now fails loudly. Add store-level tests that inject truncated txids through the query layer and assert that the fetch paths reject them.
Return errors from batch row conversion instead of silently accepting malformed batch txids while appending nil batches.
Represent withdrawal txids as optional in the domain model and keep pending withdrawals visible through GetAllWithdrawals. Pending rows now surface nil and zero values in Go and empty and zero defaults over RPC, while malformed non-NULL txids still fail loudly. Also sync godoc's with how it actually behaves: returns pending withdraws in addition to finalized ones.
starius
force-pushed
the
max-hash-string-size
branch
from
bfa83b9 to
8ac8e38
Compare
2 participants
chainhash.NewHashFromStr and chainhash.Decode accept partial txid, under 64 characters (even ""), padding its result with zeros. We should not use it directly to parse txid, otherwise we can accept truncated data accidentally. This PR replaces it with other functions (e.g.
wire.NewOutPointFromStringwhich is strict) or with the newly added strict wrapperNewHashFromStrExact.SQL related code is fragile, so I decided to treat an empty string the same as NULL in some critical parts just in case such records exist in the DB. I'm not 100% certain about SQL commits, suggestions are welcome!
Also I found an issue in sweepbatcher:
convertBatchRowswallowed errors and there is an obvious mistake when calling it. Fixed this as well.Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes