docs(validator): clarify ValidateTransactionBatch.Valid is always true#769
Merged
ordishs merged 2 commits intoMay 12, 2026
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: This PR accurately clarifies documentation to prevent future misreadings of the Verification:
Documentation accuracy: All documentation changes match the actual implementation. No discrepancies found. Previously fixed:
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-12 17:00 UTC |
The batch endpoint's Valid flag is intentionally always-true at the wire level; callers must inspect per-item Errors. Tighten the .proto comment and add an inline comment in Server.go to make the contract explicit and prevent future "fixes" from misreading the field's semantics. refs #4567
515e5c7 to
2c4091e
Compare
Make explicit that returning nil prevents the errgroup context from being cancelled, which would interrupt the other in-flight transactions in the batch.
| metaData[idx] = validatorResponse.Metadata | ||
| errReasons[idx] = errors.Wrap(err) | ||
|
|
||
| // Never return an error because we don't the other transaction to stop. |
Contributor
There was a problem hiding this comment.
[Minor] Typo in comment: "we don't the other transaction" should be "we don't want the other transactions" (or "we don't want other transactions").
✅ Fixed - Now reads: "Never return an error because we don't want to cancel the context for other transactions in the batch."
freemans13
approved these changes
May 12, 2026
icellan
approved these changes
May 12, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs #4567
Summary
Supersedes the original fix attempt in this PR. After review, the original behaviour of
ValidateTransactionBatch(settingValid: trueunconditionally) was determined to be documented intent, not a bug. The function's godoc explicitly stated:The audit's HIGH severity finding rested on hypothetical clients ignoring the documented contract. Rather than break the contract (which would silently change the wire-field semantics for every existing consumer), this PR tightens the documentation to prevent future "fixes" from reintroducing the same misreading.
Changes
services/validator/Server.go— inline comment aboveValid: truestating the field is always-true by design and retained for wire compatibility.services/validator/validator_api/validator_api.proto— tighten the field comment from "Overall batch validation status" to "Always true; callers must inspect per-item errors below."Test plan
TestServerValidateTransactionBatch/partial failures) continue to assertValid==truefor partial-failure batches, confirming the documented contract is preserved.Recommendation for #4567
Audit finding #4567 should be reclassified from HIGH "server bug" to LOW "API field is footgun-shaped, mitigated by clearer documentation."