fix: plug five silent asset-safety bugs across selector, storage, and htlc#1522
Conversation
Aman-Cool
changed the title
|
Hey @adecaro, Found these while tracing the commit pipeline for the first PR. Figured I'd fix them in one shot since they're all in the same area. The auditor The Rest are smaller but real; the lock leak compounds over time, the HTLC clock thing is a latent endorsement bomb near deadlines, and the identity key scoping only bites in multi-TMS setups which are increasingly common in production. Would appreciate a second pair of eyes especially on the auditor recovery path and the SQL idempotency change. |
|
Hi @Aman-Cool , great effort indeed. A few comments, I would remove from this PR the changes to manager.go and transactions.go. The rest is totally fine. If it is okay for you, I'll merge the rest once you remove the changes I pointed to. |
Aman-Cool
force-pushed
the
fix/multi-store-safety
branch
from
d778723 to
be0fed4
Compare
|
@adecaro, Thanks a lot for the thorough review, really appreciate it; you're absolutely right on both points and I should've caught that myself before opening this. Pulled out the The auditor recovery and the split-brain atomicity fix are tracked separately in #1507 ; would love your eyes on that when you get a chance, especially since you clearly have good context on the commit pipeline. |
adecaro
force-pushed
the
fix/multi-store-safety
branch
from
5b901eb to
1766f65
Compare
adecaro
left a comment
adecaro
left a comment
There was a problem hiding this comment.
LGTM. Great effort. Thanks.
Aman-Cool
force-pushed
the
fix/multi-store-safety
branch
from
1766f65 to
b38b311
Compare
- selector/sherdlock: call UnlockAll before returning SelectorInsufficientFunds so advisory locks don't leak when a wallet has genuinely no funds - htlc/signer: replace hard-coded time.Now() in Verifier.Verify with injectable ClockFunc so callers can pass the Fabric block timestamp for deterministic deadline enforcement across endorsers with clock skew - kvs/identitydb: add tmsID to StoreIdentityData, StoreSignerInfo, and all corresponding read paths to prevent cross-TMS identity data collisions when multiple TMS instances share one KVS backend Signed-off-by: Aman-Cool <aman017102007@gmail.com>
- Add blank line before return in selector.go for nlreturn compliance - Replace context.Background() with t.Context() in test files - Log unlock errors instead of embedding in error message Signed-off-by: Aman-Cool <aman017102007@gmail.com>
adecaro
force-pushed
the
fix/multi-store-safety
branch
from
b38b311 to
a2cd0b0
Compare
|
Hi @Aman-Cool , please, update the description of the PR to reflect the actual changes. Thanks a lot 🙏 |
adecaro
changed the title
|
great, thanks a lot @Aman-Cool 🙏 |
… htlc (hyperledger-labs#1522) Signed-off-by: Aman-Cool <aman017102007@gmail.com>
2 participants
Been going through some edge cases in the token pipeline and found a few things worth fixing.
Selector lock leak:
selectInternalwas returningSelectorInsufficientFundson the genuine no-funds path without callingUnlockAllfirst. Any tokens locked during that selection attempt just stay locked until something else cleans them up, which it won't.HTLC wall-clock non-determinism:
Verifier.Verifywas callingtime.Now()directly to evaluate the deadline. Endorsing peers with any clock skew can disagree on whether the deadline has passed, causing endorsement failures near the boundary. Made the clock injectable via aClockFuncfield so callers with access to the block timestamp can pass it in, defaulting totime.Now()when not set.IdentityDB namespace collision:
StoreIdentityData,GetAuditInfo,GetTokenInfo,StoreSignerInfo,GetExistingSignerInfo, andGetSignerInfowere all building KVS composite keys without including theTMSID. In any deployment with multiple TMS instances sharing a single KVS backend, identity and signer data silently collides across TMS instances. Added theTMSIDsegment to every key.All three are reachable through normal SDK usage, not just edge case theory.