Repository navigation
fix: isolate native BLS binding errors by OS thread - #7825
PastaPastaPasta wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (14)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. WalkthroughThe native Go and Rust C bindings now store BLS error messages per OS thread. The Go bindings lock the goroutine to its current OS thread during C binding calls. Comments describe the thread scope and pointer validity of Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant G1ElementFromBytes
participant GoRuntime
participant CBinding
participant gErrMsg
participant errFromC
G1ElementFromBytes->>GoRuntime: LockOSThread
G1ElementFromBytes->>CBinding: Convert bytes
CBinding->>gErrMsg: Store error on current OS thread
CBinding-->>G1ElementFromBytes: Return conversion result
G1ElementFromBytes->>errFromC: Copy thread-local error
G1ElementFromBytes->>GoRuntime: UnlockOSThread
Merge Risk: ⚪ Minimal · up to This change makes native BLS error messages per-thread, so concurrent failures no longer overwrite each other's messages. No actionable merge-blocking risk is identified in the supplied changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit df45987) · triage: normal |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification at df45987 found no actionable in-scope defects: both native bridges consistently use thread-local error storage, all 12 Go error-copying entrypoints retain balanced OS-thread pinning, and Rust copies errors synchronously. The license-header nitpick is dropped because existing tests in the same directory already use the headerless pattern. No builds or tests were run; the supplied CI snapshot reports successful container builds and formatting checks, with both manifest jobs still queued.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change requires reasoning about native thread-local error lifetimes and Go OS-thread pinning across 12 entrypoints, but does not alter cryptographic algorithms, signatures, or key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
|
Moved this library-only change to dashpay/bls-signatures#135, following the vendor-ownership review on #7813 (review). All 14 changed files match the reviewed patch byte-for-byte. The upstream native suite (1,417 assertions), actual Go tests/vet, Rust sys (3 cases), and safe Rust suite (23 cases) pass; independent transfer review approved. Closing this direct vendor-edit PR as superseded. Dash should receive the accepted upstream library change through its normal vendor import. The upstream PR is linked and watched here. 🤖 Posted autonomously by Codex on behalf of pasta. |
thepastaclaw
removed
the
pastaclaw:approved
Issue being fixed or feature implemented
Concurrent consumers of the bundled Go or Rust BLS bindings can overwrite a process-global native error string while another call copies its borrowed error pointer. This creates shared-storage races and can report another thread's error. Dash Core calls the native C++ API directly; daemon impact has not been established.
What was done?
Give each native OS thread its own last-error string in both bridges. Pin all 12 Go error-returning entrypoints from the native operation through the owned error copy, with balanced deferred unlocking. Rust already calls and copies synchronously on one thread. Document the borrowed getter lifetime and add ordinary, channel-serialized ownership regressions plus concurrent public Go correctness checks.
How Has This Been Tested?
On Apple Silicon/macOS, using the existing Dash depends prefix and one build job:
make -C src/dashbls -j1and fullruntest: 1,417 assertions across 17 cases passed.go test -p 1 -count=1 ./..., andgo vet -p 1 ./...passed, linked against this worktree's native archives.make -j1, Corebls_tests, whitespace lint, Go/Rust formatting and diff checks passed.The Go stress table covers four entrypoints; all 12 pinning sites were inspected independently. Rust's ownership regression exercises the sys API, while the unchanged safe copier was inspected and its existing suite passed. No TSan result, reliable crash, memory disclosure, deployed consumer reachability, or daemon exploit is claimed.
Breaking Changes
Exported C function signatures remain unchanged. Raw C consumers must retrieve and copy an error on the native thread that produced it, before another failure on that thread or thread exit. Direct C++ consumers of
gErrMsgmust use the matchingthread_localdeclaration and recompile the bridge objects together.Checklist:
This pull request was created by Codex.