Skip to content

refactor: expose unmodifiable collections from PreHandleResult - #26862

Merged
ibankov merged 4 commits into
hiero-ledger:mainfrom
ruslanvelkov-beep:prehandle-result-unmodifiable-collections
Sep 18, 2026
Merged

ibankov merged 4 commits into
hiero-ledger:mainfrom
ruslanvelkov-beep:prehandle-result-unmodifiable-collections

Conversation

@ruslanvelkov-beep

Copy link
Copy Markdown
Contributor

What

Wrap the collections exposed by PreHandleResultrequiredKeys, optionalKeys, hollowAccounts, verificationResults — as unmodifiable views in the record's compact constructor, so a consumer cannot mutate them after construction.

innerResults is intentionally left mutable: atomic-batch pre-handle appends inner results to it after the PreHandleResult is constructed (PreHandleWorkflow#preHandleTransaction).

Why

SpotBugs reports EI_EXPOSE_REP/EI_EXPOSE_REP2 on these fields — the record stored and returned the references directly. In practice the key/account sets already arrive unmodifiable (built via PreHandleContextImpl), but verificationResults is a plain HashMap returned by SignatureVerifierImpl.verify, so getVerificationResults() handed out a mutable map. Wrapping the fields makes the exposure defensively safe regardless of how the result is constructed.

Testing

  • PreHandleResultTest.getVerificationResultsRejectsMutation — new; fails before the change (the HashMap accepts a mutation), passes after.
  • PreHandleContextImplTest.exposedKeyAndAccountSetsAreUnmodifiable — asserts the exposed key/account sets reject mutation.
  • ./gradlew :app:test for PreHandleResultTest, PreHandleContextImplTest, PreHandleWorkflowImplTest (atomic-batch + reuse), DefaultKeyVerifierTest, DispatchValidatorTest — all green.

Signed-off-by: Ruslan Velkov <ruslan.velkov@limechain.tech>
@ruslanvelkov-beep
ruslanvelkov-beep requested a review from a team as a code owner August 14, 2026 14:43
@ruslanvelkov-beep ruslanvelkov-beep self-assigned this Aug 14, 2026
@lfdt-bot

lfdt-bot commented Aug 14, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@joshmarinacci joshmarinacci left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this have any performance impact?

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #26862      +/-   ##
============================================
+ Coverage     71.25%   71.26%   +0.01%     
- Complexity    11906    11907       +1     
============================================
  Files          2596     2596              
  Lines        109552   109558       +6     
  Branches      12364    12368       +4     
============================================
+ Hits          78061    78081      +20     
+ Misses        27447    27440       -7     
+ Partials       4044     4037       -7     
Files with missing lines Coverage Δ Complexity Δ
.../node/app/workflows/prehandle/PreHandleResult.java 100.00% <100.00%> (ø) 0.00 <0.00> (ø)

... and 8 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ruslanvelkov-beep

Copy link
Copy Markdown
Contributor Author

Will this have any performance impact?

No meaningful impact — these are Collections.unmodifiable* views, not defensive copies. Each is wrapped once in the constructor (O(1), no element copying)

…e order

Signed-off-by: Ruslan Velkov <ruslan.velkov@limechain.tech>
@ruslanvelkov-beep

Copy link
Copy Markdown
Contributor Author

Follow-up now that this switched to defensive copies per review: it's no longer O(1) — it's O(n) in the number of signing keys / hollow accounts. But n is single-digit per transaction, and the same pre-handle path already runs verifySignatures(...) (Ed25519/ECDSA) on that transaction, which dominates by orders of magnitude. So still no meaningful impact — the copy is lost in the noise of the crypto that's already mandatory there.

@testlens-app

testlens-app Bot commented Sep 18, 2026

Copy link
Copy Markdown

🚨 All tests passed but jobs failed 🚨

Failed Jobs without Test Failures

600: [FLOW] PR Checks / MATS / Snyk Scan / Snyk Checks
701: [FLOW] Auto Unapprove PR / Auto Unapprove PR

🏷️ Commit: 8ec9a69
▶️ Tests: 48014 executed
🟡 Checks: 16/24 completed


Learn more about TestLens at testlens.app/docs.

@ibankov
ibankov merged commit 95a8057 into hiero-ledger:main Sep 18, 2026
77 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants