feat: add built-in portfolio viewer UI at /portfolio

[Marchhill]

Changes

• Adds a minimal, self-contained portfolio viewer UI served by the node at the /portfolio path of the JSON-RPC HTTP endpoint (no external assets — all data comes from the node's own JSON-RPC for privacy). Packaged as a new embedded plugin, Nethermind.PortfolioViewer.Plugin, disabled by default (opt in with --PortfolioViewer.Enabled true).
• Unified multi-chain viewing (Ethereum, Gnosis, Sepolia, Hoodi, Chiado): the plugin probes localhost for sibling Nethermind nodes (/portfolio-nodes, proxied JSON-RPC via /portfolio-rpc/{port}), continuously re-discovering them.
• NFT tracking and display.
• Automatic token/NFT detection. Recommended to run with the log index enabled (--LogIndex.Enabled true) for usable detection performance, and with the LogIndex JSON-RPC module exposed so the UI can show index-build progress. Without the index it still works but falls back to a slower per-block bloom scan.
• ENS name resolution (name.eth)
• Subscribe to notifications about account activity.
• Fiat conversion and token values via on-chain Chainlink price feeds, as well as Uniswap pools.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

• Unit tests in StartupTests cover serving the embedded page on plain HTTP ports and 404 on authenticated/unknown ports.
• The in-page keccak-256/namehash was verified against EIP-137 vectors and cross-checked against Python's SHA3-256 for the multi-block absorb path.
• UI exercised end-to-end in a browser against a mocked JSON-RPC node (ENS pinning, token tracking with metadata prefill, balance rendering, persistence across reloads).

Documentation

Requires documentation update

• Yes
• No

New PortfolioViewer.Enabled config option (disabled by default) and the /portfolio page should be mentioned in the docs. The docs should also recommend running with the log index enabled (--LogIndex.Enabled true) and the LogIndex JSON-RPC module exposed for good detection performance.

Requires explanation in Release Notes

• Yes
• No

Nethermind now ships a built-in portfolio viewer at the /portfolio path of the JSON-RPC HTTP endpoint (native + ERC-20 balances, NFTs, ENS support), using only local node data. Disabled by default; enable with --PortfolioViewer.Enabled true.

For good auto-detection performance it is recommended to also enable the log index (--LogIndex.Enabled true) and expose the LogIndex JSON-RPC module.

[asdacap]

Wrap in a plugin?

[claude]

Claude finished @Marchhill's task in 2m 19s —— View job

---

Re-review: Portfolio Viewer plugin (#12360)

I re-checked the code against the five substantive findings from the previous review (2 High, 3 Medium) plus the Low/naming items. All are resolved, and the fixes are covered by tests. No new blocking issues.

• Verify prior High findings
• Verify prior Medium findings
• Fresh pass over backend correctness/security/perf

Prior findings — status

#	Prior finding	Status
1 (High)	UnpinAllAsync wiped the entire Kubo pinset	✅ Fixed — PinnedCidStore now tracks only CIDs this plugin pinned; UnpinAllAsync iterates pins.Snapshot() and pin/rms exactly those (PortfolioViewerConfigurer.cs:247-260, PinnedCidStore.cs).
2 (High)	Documented disable flag didn't exist	✅ Fixed — interface renamed to IPortfolioViewerConfig → category PortfolioViewer, so keys are PortfolioViewer.Enabled / PortfolioViewer.SiblingProbePorts, matching the PR body, release notes, and README. Plugin now consistently named PortfolioViewer throughout.
3 (Medium)	No CSRF/origin protection on side-effecting endpoints	✅ Fixed — IsCrossOrigin guard (:110-158) rejects state-changing requests (proxy, detect POST/DELETE, pin, unpin-all) carrying a foreign Origin with 403; covered by SideEffecting_CrossOriginRequest_Rejected / _SameOriginRequest_Allowed.
4 (Medium)	Unvalidated rel in IPFS gateway proxy	✅ Fixed — IsSafeIpfsRef (:161-164) enforces a charset and rejects ./.. segments; applied to both the gateway rel and the pin CID. Covered by Ipfs_InvalidRef_Returns400.
5 (Medium)	Full-file rewrite on every Put	✅ Fixed — DetectionCache now coalesces writes with a 2s leading/trailing throttle (:135-165).
6–8 (Low)	README path/key, naming	✅ Fixed — README documents /portfolio and PortfolioViewer.*; naming unified.

Fresh pass — observations (non-blocking)

• DetectionCache.RequestSave runs the leading-edge Save() (file I/O) while holding _saveGate (DetectionCache.cs:143-144), briefly blocking other Put callers. Bounded to once per 2s window, so acceptable — noting only.
• IsCrossOrigin treats an absent Origin as same-origin (:156). Correct for browser CSRF (browsers attach Origin on cross-origin state-changing requests); non-browser clients like curl are intentionally out of scope. Good.
• Proxy targets are constrained to discovered siblings — ProxyAsync/ProxyDetectAsync only fire for IsKnownSibling(port) on 127.0.0.1, and sibling ports come from the configured probe list, so there's no arbitrary-port SSRF. Good.
• Fire-and-forget tasks (PinAsync, UnpinAllAsync, ScheduleTrailingFlushAsync) are documented as intentional and each handles its own exceptions — no async void, no swallowed faults.
• Detection scanner runs via IBackgroundTaskScheduler at BelowNormal priority with adaptive chunking, cancellation, and a MaxContracts cap — no consensus/hot-path impact, unchanged from the prior review's assessment.

Recommendation

Mergeable. All previously-raised High and Medium items have been fixed and are backed by tests. Remaining notes are minor and non-blocking.
· branch feature/balance-viewer

[benaadams]

Reviewed the current head while accounting for prior feedback. The earlier plugin-structure, configuration naming, IPFS path validation, cache-write throttling, and cancellation-persistence concerns appear resolved; the remaining issues are called out inline.

[benaadams]

This serves arbitrary IPFS bytes with their upstream Content-Type under the portfolio/JSON-RPC origin. A malicious HTML or SVG CID can therefore execute with same-origin access to portfolio storage and RPC; IPFS path gateways explicitly lack origin isolation. Please use a separate/subdomain origin, or sandbox responses and allowlist safe media types with nosniff; GETs should also reject cross-site fetches so foreign pages cannot drive the local Kubo gateway. See IPFS gateway guidance.

[benaadams]

A successful pin/add does not prove that this plugin created the pin: Kubo also succeeds when a CID is already pinned. Recording every success means “unpin all” can remove a pin the operator created independently. Check the exact pin state before adding and record only newly created pins, or avoid automatically removing ownership-ambiguous pins.

[benaadams]

pins.Clear() runs even when pin/rm fails or returns an error, and it can erase a pin added concurrently after Snapshot(). That permanently loses the ability to retry cleanup. Remove each CID from the store only after a successful unpin, retain failures, and serialize pin/unpin operations or use an atomic drain protocol.

[benaadams]

This unauthenticated endpoint lets the caller schedule a full-history scan for any chain ID and address. The supplied chain ID is only used in _active/cache keys while the scanner always queries the local chain, so arbitrary IDs bypass deduplication and can keep the background queue full. Derive or validate the local chain ID and bound/rate-limit unique account scans.

[benaadams]

A numeric head alone cannot establish that this cache is current. After a same-height or deeper reorg, replacement blocks at or below entry.Head are never scanned, so newly introduced token contracts can be missed indefinitely. Persist and revalidate the canonical block hash, or rescan an overlap/finality window on every update.

[benaadams]

Repository coding rules avoid LINQ where straightforward loops work. Please replace these Take/allocation pipelines with an explicit bounded copy; the same rule applies to the new All/Any/Select usage in PortfolioViewerConfigurer and the LINQ pipelines in SiblingNodeRegistry.

[benaadams]

The new public config/plugin contracts need XML documentation under the repository rules. Please document the public interfaces and members and use <inheritdoc/> on their implementations; this also applies to the undocumented IPinnedCidStore/IDetectionCache methods and public plugin/module types.

[benaadams]

The test rules require NUnit 4’s using (Assert.EnterMultipleScope()) rather than legacy Assert.Multiple; please update all six occurrences in this file. Independent result assertions in DetectionScannerTests and PortfolioViewerMiddlewareTests should likewise use a multiple scope.

[benaadams]

This blanket catch hides failed cleanup and can leave temporary state that contaminates later test runs. Let teardown fail when deletion fails, or log a narrowly caught expected exception.

[benaadams]

This comment says external URIs are never fetched, but the current implementation performs opt-in off-chain metadata and artwork requests. Please update it to describe the actual privacy behavior.

[benaadams]

@claude confirm or refute the feedback from me

[claude]

Claude finished @benaadams's task in 3m 58s —— View job

---

Adjudicating @benaadams' review feedback

• Read backend files (PortfolioViewerConfigurer, DetectionScanner, DetectionCache, PinnedCidStore, SiblingNodeRegistry)
• Read frontend regions (portfolio.html metadata cache / refresh gating / privacy comment)
• Read tests + repo rules (test-infrastructure.md, coding-style.md, robustness.md)
• Adjudicate each of the 13 inline findings

Verdict: 11 of 13 confirmed as written; 2 confirmed with a qualification. I found nothing to refute outright. Details below, with the extra evidence I turned up.

---

Confirmed

1. IPFS proxy serves untrusted bytes with upstream Content-Type under the JSON-RPC origin — PortfolioViewerConfigurer.cs:194
Confirmed, and this is the most serious item in the set. context.Response.ContentType = resp.Content.Headers.ContentType.ToString() forwards whatever Kubo reports; there is no X-Content-Type-Options: nosniff, no CSP, and no Content-Disposition. An NFT whose tokenURI resolves to an HTML or SVG CID renders as script in the same origin as /portfolio, which holds the watched-address list in localStorage, the IndexedDB metadata store, and same-origin POST / access to unauthenticated JSON-RPC (including whatever modules the operator enabled). An <iframe src="http://localhost:8545/portfolio-ipfs/<cid>"> from any page reaches it — the attacker can't read the frame, but the script inside it runs with that origin's privileges and can exfiltrate. Delivery doesn't even need social engineering: an unsolicited NFT transfer is enough to get the CID into pinIpfs/resolveArtUri.

Your second half also holds: isIpfs is deliberately excluded from isSideEffecting at :112, so the GET path has no origin check. A foreign page can drive the local Kubo node into fetching arbitrary network content (<img>/no-cors fetch), and response timing leaks whether a CID is already local.

Fix this →

2. A successful pin/add does not prove this plugin created the pin — :232
Confirmed. Kubo's pin/add is idempotent: pinning an already-pinned CID returns 200 with the CID in Pins, so if (resp.IsSuccessStatusCode) pins.Add(cid) records operator-created pins that happen to coincide with viewed art. Unpin-all then removes them.

Related, and not covered by your comment: UnpinAllAsync finishes with repo/gc (:258), which is repo-wide — it evicts all unpinned blocks on the user's node, not just the ones this plugin dropped. That's a narrower survivor of the original "wipes the pinset" finding, and it should probably be dropped or made opt-in regardless of how #2 is resolved.

3. pins.Clear() runs unconditionally — :257
Confirmed, and slightly worse than described: pin/rm's response status is never inspected at all (using HttpResponseMessage _ = await ...), so a 500 from Kubo is indistinguishable from success. Combined with the swallowed HttpRequestException, the store is cleared even when nothing was unpinned, and the CIDs become permanently unreclaimable. The Snapshot()→Clear() window also drops any concurrent PinAsync addition, leaking an untracked pin.

4. Unauthenticated /portfolio-detect accepts an arbitrary chain ID — :306
Confirmed. post.ChainId is used only for _active/cache keys; DetectionScanner always queries the local logFinder/blockFinder, so distinct IDs are pure key-space inflation that bypasses the _active dedup and pushes work into the background queue. Concrete impact you didn't spell out: DetectionCache is capped at DefaultMaxEntries = 10_000 with LRU-by-UpdatedMs eviction, so a flood evicts real users' scan cursors and forces full re-scans.

Two qualifications, neither of which changes the conclusion: the IsCrossOrigin guard at :113 does cover isDetectPost, so browser drive-by is blocked and the exposure is any local process (or anything remote if the RPC port is bound non-loopback); and since Address.TryParse accepts 2^160 addresses, flooding works with the correct chain ID too — so the bound/rate-limit half of your recommendation is the load-bearing part, chain-ID validation is the cheap correctness fix.

5. A numeric head can't establish cache currency across reorgs — DetectionScanner.cs:61
Confirmed. entry.Head >= head skips, and the forward phase resumes at existing.Head + 1, so an equal-height or deeper reorg permanently skips the replacement blocks. Impact is bounded — this is an additive discovery cache, so the failure is a missed token rather than wrong balances — but it is a real permanent gap. The inverse also holds: contracts discovered in a reorged-out block are never removed.

6. Forward phase doesn't handle ResourceNotFoundException — :95
Confirmed. The forward try catches only OperationCanceledException; LogFinder.cs:64/:70 throw ResourceNotFoundException("Receipt not available for From block …"), which falls through to the outer catch at :148. It doesn't hard-loop within a single run, but the effect is what you describe across polls: _active is cleared, the entry is never marked Complete, the client re-triggers, and the same unavailable range is retried indefinitely — at Warn each time.

One point in your favour that the comment understates: because the forward block at :87 precedes the existing?.Complete check and the downward walk, a permanently-failing forward range blocks the downward history scan entirely, not just the forward gap. The downward path already handles this correctly at :134, which makes the asymmetry look unintentional.

7. NFT metadata cache returns before revalidating tokenURI — portfolio.html:1947
Confirmed. idbGetManyMeta is keyed chainId|collection|id and short-circuits at :1948 before the SEL_TOKEN_URI batch at :1959. The persistence gate at :1996 (mk === 'on-chain' || mk === 'IPFS') conflates CID immutability with mapping immutability — a reveal or dynamic NFT returns a different ipfs:// CID for the same id, and the stale entry is then never displaced. The comment at :1663 ("immutable, so nothing can go stale") states the incorrect invariant explicitly and should be corrected along with the behaviour.

8. Count-based refresh signature misses equal-count ownership changes — portfolio.html:2682
Confirmed on both halves. For ERC-721, sig = erc721Count, so a transfer-out plus transfer-in between refreshes leaves the count unchanged and continue skips re-discovery — held ids stay wrong for the session. For ERC-1155, sig = 'x' is constant, so every refresh after the first is a no-op; the comment at :2679-2680 acknowledges that as a deliberate trade-off, but the ERC-721 case reads as an oversight rather than a trade-off.

9. LINQ where a loop would do — DetectionCache.cs:66
Confirmed against coding-style.md ("No LINQ when a simple for/foreach works — use LINQ only for complex queries"). 13 sites: DetectionCache.cs:66,70; PortfolioViewerConfigurer.cs:163,164,172; SiblingNodeRegistry.cs:58,59,60,73,80,85. Worth noting IsSafeIpfsRef (:163-164) is on the request path and allocates a string[] per call via Split('/') plus a closure per char — a single foreach over the span with a running segment check does the same job allocation-free, so this one is more than style.

11. Assert.EnterMultipleScope() over Assert.Multiple — PortfolioViewerScriptTests.cs:156
Confirmed verbatim against .agents/rules/test-infrastructure.md:69: "wrap them in using (Assert.EnterMultipleScope()) { ... } (the NUnit 4 form; prefer this over the older Assert.Multiple(() => { ... }) lambda)." Six occurrences: :156, 170, 213, 229, 564, 600. The second half of your comment (adding scopes in DetectionScannerTests, 27 bare Assert.That, and PortfolioViewerMiddlewareTests, 18) is a per-group judgement call rather than a rule violation, but the same rule notes the dedupe-into-a-helper preference, which those files would benefit from.

13. Stale privacy comment — portfolio.html:1647
Confirmed. "Only fully on-chain metadata (data: URIs, e.g. Nouns) is rendered — external URIs are never fetched, so no request leaves the node" is contradicted ~320 lines later by fetchOffchain(murl, 3) at :1968 and the pinIpfs calls at :1984. Per coding-style.md, a comment that contradicts the code is worse than none.

---

Confirmed with a qualification

10. XML documentation on public contracts — IPortfolioViewerConfig.cs:8
Confirmed for IPinnedCidStore (PinnedCidStore.cs:14-16), IDetectionCache (DetectionCache.cs:19-22), and the public PortfolioViewerPlugin/PortfolioViewerModule/PortfolioViewerConfig types — all undocumented, and coding-style.md asks for doc comments on all public APIs. Also worth flagging while there: those three are the only non-sealed public types in the plugin.

The qualification is the anchor you chose. IConfig interfaces in this repo document themselves through [ConfigItem(Description=…, DefaultValue=…)] and carry no XML docs — ITxPoolConfig has zero <summary> tags, IJsonRpcConfig likewise. IPortfolioViewerConfig already has Description + DefaultValue on both members, so it matches the established convention; requiring <summary> there specifically would be a new bar rather than an existing rule.

12. Blanket catch in test teardown — PortfolioViewerPinStoreTests.cs:25
Confirmed on the rule, refuted on the stated rationale. robustness.md prohibits swallowing exceptions in a bare catch, and this branch already has commit 8594dad "catch specific exceptions instead of bare catch" — so consistency clearly argues for catch (Exception e) when (e is IOException or UnauthorizedAccessException) plus a TestContext.Out line here too.

But the contamination mechanism doesn't apply: _dir is Path.Combine(Path.GetTempPath(), "bv-pins-" + TestContext.CurrentContext.Test.ID), unique per test, so a leaked directory can't be observed by any other test. And "let teardown fail when deletion fails" would trade a harmless temp-dir leak for CI flakiness on Windows, where a transient handle on the just-written portfolio-viewer-pins.json fails the delete. Narrow the catch and log — don't make it throw.

---

Summary

#	Finding	Verdict
1	IPFS proxy content-type / origin isolation	✅ Confirmed — most serious of the set
2	pin/add success ≠ pin ownership	✅ Confirmed (+ repo-wide repo/gc)
3	Unconditional pins.Clear()	✅ Confirmed (+ status never checked)
4	Unauthenticated scan, arbitrary chain ID	✅ Confirmed (+ LRU cache eviction)
5	Numeric head vs reorg	✅ Confirmed
6	Forward phase misses ResourceNotFoundException	✅ Confirmed (+ blocks downward walk)
7	Metadata cache not revalidated vs tokenURI	✅ Confirmed (+ wrong invariant in comment)
8	Count-based NFT refresh signature	✅ Confirmed
9	LINQ vs loops	✅ Confirmed (13 sites)
10	XML docs on public contracts	⚠️ Confirmed except for IConfig, where ConfigItem is the repo convention
11	Assert.EnterMultipleScope()	✅ Confirmed (6 occurrences)
12	Blanket catch in teardown	⚠️ Rule confirmed; contamination rationale doesn't apply (per-test unique dir)
13	Stale privacy comment	✅ Confirmed

I've only adjudicated here — no code changed. Say the word and I'll implement any subset; #1, #3 and #6 look like the ones worth doing first.
· branch feature/balance-viewer