Skip to content

fix(fabricx): stop retaining RWSets and tx statuses in the vault - #1618

Merged
SaidAltury-ibm merged 2 commits into
hyperledger-labs:mainfrom
SaidAltury-ibm:SA_1599
Jul 30, 2026
Merged

fix(fabricx): stop retaining RWSets and tx statuses in the vault#1618
SaidAltury-ibm merged 2 commits into
hyperledger-labs:mainfrom
SaidAltury-ibm:SA_1599

Conversation

@SaidAltury-ibm

@SaidAltury-ibm SaidAltury-ibm commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

closes #1599
The FabricX vault held two in-memory maps (rwsets, txStatuses) that grew
unboundedly: nothing on the FabricX path ever pruned or read them. FabricX has
no local commit pipeline — commit/validation happens in the external
fabric-x-committer, and finality is delivered by the gRPC notifier — so the
vault methods that populated or read those maps (CommitTX/DiscardTx/
SetDiscarded/Match/RWSExists) are never invoked in the FabricX wiring. Endorse-
only nodes were pinning ~1.5 GB of RWSets at rest as a result (PR #1206).

Changes:

  • Remove the rwsets and txStatuses maps, the txStatusInfo struct, and the mutex.
    NewRWSet/NewRWSetFromBytes no longer retain the RWSet; the wrapper owns it.
  • The five commit-pipeline interface methods now panic instead of returning a
    value. They exist only to satisfy fdriver.Vault; reaching one means the vault
    was wired into a generic committer by mistake, which is a programming error,
    not a runtime condition. Panicking fails loudly and cannot be silently
    swallowed — RWSExists in particular has no error channel, so its old false
    return was an actively misleading answer.
  • Status now delegates to the batched Statuses so both report a tx unknown to
    the committer as Unknown ("not final yet") rather than as an error — the
    condition a caller polling an in-flight tx will hit.
  • Bytes() passes namespace-version info to the marshaller per call instead of
    through a shared mutable field, fixing a data race between concurrent Bytes()
    calls. The marshaller is now stateless; the racy field and its exported
    wrapper are removed. Covered by a new -race test.

@SaidAltury-ibm
SaidAltury-ibm marked this pull request as ready for review July 27, 2026 20:27
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

Hey @SaidAltury-ibm 👋 thanks for the PR!
I'm your friendly PR Helper Bot 🤖 and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.

This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!
Here's the latest:


PR Checks

DCO Sign-off -- All commits have valid sign-offs. Nice work!


GPG Signature -- All commits have verified GPG signatures. Locked and loaded!


Merge Conflicts -- No merge conflicts detected. Smooth sailing!


Issue Link -- Linked to #1599 (assigned to you).


🎉 All checks passed! Your PR is ready for review. Great job!

@github-actions github-actions Bot added status: needs revision The pull request requires changes from the author before it can be reviewed or merged status: needs review The pull request is ready for maintainer review and removed status: needs revision The pull request requires changes from the author before it can be reviewed or merged labels Jul 27, 2026
@SaidAltury-ibm SaidAltury-ibm added this to the 26Q3 milestone Jul 27, 2026

@mbrandenburger mbrandenburger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @SaidAltury-ibm - this is a solid PR. Please see my comments below.

Comment thread platform/fabricx/core/vault/vault.go
Comment thread platform/fabricx/core/vault/vault.go Outdated
Comment thread platform/fabricx/core/vault/vault.go Outdated
Comment thread platform/fabricx/core/vault/vault.go Outdated
Comment thread platform/fabricx/core/vault/vault.go
Signed-off-by: Said Altury <Said.Altury@ibm.com>

@mbrandenburger mbrandenburger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @SaidAltury-ibm - greate updates on this PR. I think this PR is almost ready to be merged. Please see my minor comments below. Thank you

Comment thread platform/fabricx/core/vault/vault_test.go Outdated
Comment thread platform/fabricx/core/vault/vault.go Outdated
Signed-off-by: Said Altury <Said.Altury@ibm.com>
@mergify

mergify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@SaidAltury-ibm
SaidAltury-ibm merged commit a9f40a2 into hyperledger-labs:main Jul 30, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review The pull request is ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate: RWSet retained and grow unbounded on endorsement-only nodes

2 participants