Skip to content

[Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923

Description

@warku123

Summary

java-tron maintains two parallel metrics stacks with independent switches: a legacy Dropwizard-based implementation (org.tron.core.metrics, gated by node.metricsEnable) exposed only through the gRPC MonitorApi.getStatsInfo endpoint and the HTTP /monitor/getstatsinfo servlet, and the Prometheus stack (org.tron.common.prometheus) that has become the standard monitoring path.

This proposal stages the decommissioning of the legacy stack together with MonitorApi over two releases, leaving Prometheus as the single supported monitoring backend. Phase 1 (next release) migrates the functional consumer, adds fetch-behavior counters that are retained long-term, and marks the legacy APIs deprecated while keeping them fully functional. Phase 2 (the agreed major release) removes the legacy implementation, its configuration chain, the Dropwizard dependency, and the protobuf definitions. The proposal also adds a tron:node_info{version, genesis_block_id} info metric so that node information stays easy to observe.

This is the implementation issue for item 7 of the tracking issue #6921.

Problem

Motivation

Prometheus has become the standard monitoring solution for java-tron (see #6590 and the tron-docker metric_monitor reference stack). Keeping a second, non-Prometheus implementation alongside it means duplicated instrumentation and extra maintenance, and it misleads operators: enabling node.metricsEnable without Prometheus produces no scrapeable output at all. The legacy endpoints have no known production consumers.

Current State

  • The legacy stack writes into a Dropwizard MetricRegistry and is served only via MonitorApi.getStatsInfo (gRPC) and /monitor/getstatsinfo (HTTP). Most of its fields already have Prometheus equivalents.
  • The legacy registry is not purely observational: fetch-block peer selection (FetchBlockService.getPeerTop75) consumes the per-peer net.latency.fetch.block.<peerIP> 75th-percentile histogram as functional failover input. These histograms are written via histogramUpdateUnCheck — bypassing the metrics-enable gate — use one unbounded key per peer IP that is never cleaned on disconnect, and mix latency samples from different block-fetch paths, so the choice of metrics backend silently dictates scheduling behavior.
  • A few node-level fields exist only in the legacy payload — notably the node version — and are not currently exposed to Prometheus.

Limitations or Risks

  • The fetch-block peer-selection read must be migrated before the legacy registry can be removed.
  • During Phase 1 the legacy endpoints remain functional, so the legacy instrumentation stays in place until Phase 2.
  • Operators with node.metricsEnable=true who skip the deprecation window must not silently lose monitoring: the Phase 2 release retains a tombstone warning when the key is still present.

Proposed Solution

Proposed Design

The change is staged over two releases.

Phase 1 (next release) — migrate the functional consumer and deprecate the legacy APIs.

Fetch-block peer selection. Fetch-block peer selection switches from the per-IP P75 histogram to a bounded, per-connection estimate of actual block-fetch duration. Each PeerConnection holds an explicit unsampled state plus a volatile long EWMA. While unsampled, reads fall back to the libp2p channel RTT (Channel.getAvgLatency(), the same signal PeerManager.sortPeers already uses); the first real fetched-block measurement replaces the fallback instead of entering the EWMA, and smoothing starts from the second measurement. The steady-state weight is a fixed α = 0.1 ((ewma * 9 + last) / 10) expressed as a named constant with its rationale documented; a warm-up running mean is deliberately not used. Estimates are clamped to fetchBlockTimeout, with an explicit saturation gate that requires a strictly better candidate and an unconditional hard-timeout branch once the wall-clock fetch budget is exhausted. Because the field lives on PeerConnection, it is released with the connection — no disconnect bookkeeping and no unbounded per-IP keys. The per-IP histogram read and write sites are removed. Global fetch-latency observability stays available through the existing unlabeled Prometheus histogram (a per-peer Prometheus label would reintroduce the unbounded-cardinality problem in the TSDB).

Legacy APIs stay functional. The gRPC Monitor service and the HTTP /monitor/getstatsinfo endpoint remain registered and keep serving as before. Both are marked deprecated (option deprecated = true on service Monitor and message MetricsInfo), documented as deprecated in release notes and docs, and accompanied by a startup deprecation warning when node.metricsEnable is present plus a process-once WARN on the first invocation of either deprecated API. A legacy-field-to-Prometheus mapping table, including fields without an equivalent, is published in the PR description, release notes, and the English/Chinese documentation.

Node information. Add a tron:node_info{version, genesis_block_id} info metric (collector base name tron:node, queried as tron:node_info):

  • version preserves the node version previously carried only by the legacy payload and not currently exposed to Prometheus.
  • genesis_block_id is the full genesis block hash — the canonical TRON chain identifier. Mainnet, Nile and private networks each have distinct values, so dashboards and PromQL can tell at a glance which network a node belongs to (e.g. tron:node_info{genesis_block_id="..."}).
  • Node IP is intentionally not exported; the scrape target's instance label already identifies the node.

Fetch-behavior counters (retained long-term). Add three unlabeled, monotonically increasing Prometheus counters:

  • tron:block_fetch_armed — incremented when FetchBlockService arms a fetch tracking.
  • tron:block_fetch_secondary — incremented when a secondary fetch is sent.
  • tron:block_already_known — incremented for a matching outstanding adv request whose exact block ID is already known before processing that response. A block below the current head is not necessarily already known; it may be an unseen fork block, so the counter requires a matching advInvRequest.remove(item) together with the exact block ID already being known. It is a best-effort signal: concurrent arrivals may be missed, and it does not establish secondary-fetch attribution. Exact attribution remains in the experiment harness.

These counters describe fetch behavior that remains after the legacy stack is removed. They are not coupled to the Dropwizard cleanup and are retained beyond Phase 2; removing them later would be a separate observability decision. Normalized rates (secondary-fetch ratio, already-known response rate) are derived in PromQL from the counters rather than precomputed on the node.

Phase 2 (the agreed major release) — remove the legacy stack. Remove MetricsUtil, the legacy metric managers and DTOs, the gRPC MonitorApi registration, the /monitor/getstatsinfo servlet route, all legacy write sites, the node.metricsEnable configuration chain, and the Dropwizard dependency, following the WalletExtension staging precedent from #6921. Remove the protobuf definitions (service Monitor in api.proto, message MetricsInfo in Tron.proto); clients calling the removed endpoints receive UNIMPLEMENTED / 404. When node.metricsEnable is still present, emit a tombstone warning so operators who skip Phase 1 do not silently lose monitoring. The three fetch-behavior counters and tron:node_info remain. /monitor/getnodeinfo is node info, not metrics, and is unaffected.

Key Changes

  • Module: common (new info metric and fetch-behavior counters), framework (migration in Phase 1; removal in Phase 2), build files (Dropwizard dependency removed in Phase 2).
  • Configuration: Phase 1 keeps node.metricsEnable working and warns when it is present; Phase 2 removes the configuration chain and leaves a tombstone warning for a residual node.metricsEnable. node.metrics.prometheus.enable / .port keep their semantics throughout.
  • API: Phase 1 keeps gRPC Monitor.GetStatsInfo and HTTP GET /monitor/getstatsinfo functional but deprecated; adds tron:node_info{version, genesis_block_id} and the three fetch-behavior counters. Phase 2 stops serving the legacy APIs and removes their proto definitions.

Impact

  • Developer Experience: a single metrics stack to instrument and review after Phase 2; the legacy and Prometheus stacks coexist during the Phase 1 deprecation window.
  • Stability: fetch-block peer selection moves from a per-tick histogram snapshot (registry lookup + locked, sorted sample copy) to an O(1) field read of a per-connection EWMA; the unbounded per-IP metric family is gone, and the scheduling signal is decoupled from the metrics backend.
  • Security: the legacy gRPC/HTTP Monitor surface is removed in Phase 2; the Prometheus exporter path is untouched.
  • Operations: node version and network identity remain visible in Prometheus/Grafana; the fetch-behavior counters stay available to detect regressions after the legacy stack is removed.

Compatibility

  • Breaking Change: Phase 2 only. In Phase 1, gRPC Monitor.GetStatsInfo and HTTP GET /monitor/getstatsinfo remain functional; both are marked deprecated in the protos and docs with warnings. In Phase 2 they stop being served (UNIMPLEMENTED / 404) and the protobuf definitions are removed, following principle 2 of Tracking: code refactor and cleanup #6921 and the WalletExtension staging precedent. (Today the gRPC endpoint is only registered when node.metricsEnable=true, which defaults to false, and the HTTP endpoint serves mostly empty metric fields when the switch is off.)
  • Default Behavior Change: Yes — fetch-block peer selection now ranks peers by a decaying average (EWMA) of measured block-fetch durations, clamped to fetchBlockTimeout, instead of the raw 75th percentile of the per-IP histogram. Semantics stay close to the old intent (actual fetch duration) while fixing its defects: mixed request sources, no decay, unbounded keys. The unsampled state falls back to the channel RTT and the first real fetch replaces it, so a newly connected peer is never ranked with a placeholder observation (previously an empty histogram silently ranked a peer as fastest — pinned by unit tests). One intentional behavior change is listed explicitly per principle 1 of Tracking: code refactor and cleanup #6921: previously a candidate whose P75 exceeded fetchBlockTimeout was filtered out; with the clamped EWMA a saturated candidate stays eligible and may receive the secondary request at the hard timeout when no better candidate exists. This is a deliberate liveness improvement and is documented and tested.
  • Migration Required: Yes, but time-boxed — during Phase 1 operators who use the legacy stack should switch flags; the config key keeps working until Phase 2 and its presence triggers a deprecation warning, so monitoring is not lost mid-window.
Before After
node.metricsEnable = true node.metrics.prometheus.enable = true
(legacy port n/a) node.metrics.prometheus.port = 9527 (default)

The key is ignored silently by config parsing in Phase 2; the release notes must call out this migration, and the tombstone warning must fire when the key is still present.

Test Plan

  • New unit tests covering fetch-block peer selection and the EWMA estimator: explicit unsampled fallback, replace-on-first-sample, degradation and recovery convergence with the fixed α = 0.1, clamp, saturation gate, hard-timeout branch, boundary values, and reconnect isolation; tests for tron:node_info and the three fetch-behavior counters, including the precise tron:block_already_known definition.
  • Controlled before/after experiment: warm up the estimator, degrade one peer mid-run (e.g. tc netem), and report how many additional per-peer samples are needed for the ranking to flip; the develop baseline runs the same harness and topology with the counters cherry-picked onto an instrumentation-only branch so both arms expose identical metrics. Report the normalized secondary-fetch rate and already-known response count; primary/secondary attribution comes from the harness. Measured replacement for the earlier one-or-two-sample phrasing: at fetchBlockTimeout = 200 the estimate reaches its clamp within 1-2 in-window samples and the first switch lands on block 2-3 of a degradation window; at fetchBlockTimeout = 500 sub-budget degradation does not saturate and only the comparison path fires (decision traces linked from the PR).
  • Repo CI: build matrix, checkstyle, CodeQL, single-node integration, and the changed-line coverage gate (> 60%).

Rollback

Phase 1: revert the PR commits; the protobuf definitions are unchanged (only marked deprecated), so the previous behavior is restored without protocol or config migration. Phase 2: revert reintroduces the legacy endpoints and the Dropwizard dependency, and requires restoring the removed proto definitions.

References

Additional Notes

  • Do you have ideas regarding implementation? Yes — see Proposed Design (bounded per-connection fetch-duration estimator); the pull request will reference this issue.
  • Are you willing to implement this feature? Yes

Activity

  1. xxo1shine commented on Aug 25, 2026

    @xxo1shine
    Collaborator

    @warku123 The existing /monitor/getnodeinfo interface already provides the node's version and chain_id information. If this endpoint remains available after the legacy metrics are removed, these values will not be lost as a result of removing the legacy metrics.

    In that case, why do we still need to expose the same information through the new tron:node_info{version, chain_id} Prometheus metric? Is there a specific use case that requires these fields to be available as Prometheus metrics?

  2. warku123 commented on Aug 31, 2026

    @warku123
    ContributorAuthor

    @xxo1shine Thanks for the question — you're right that the values are not lost, and /monitor/getnodeinfo remains available. The tron:node_info metric is not meant for API consumers; it targets the monitoring path, where the two serve fundamentally different models:

    • The HTTP API answers interactive, point-in-time queries for humans.
    • Prometheus/Grafana alerting can only evaluate time series collected by scrape. Without a metric, node identity simply cannot participate in PromQL alert rules.

    With the metric in place, common operational checks become plain alert rules:

    # alert when a node is NOT on the expected chain (chain IDs differ per network,
    # e.g. mainnet vs Nile vs private — each has its own genesis block hash)
    absent(tron:node_info_info{chain_id="<expected genesis hash>"})
    
    # overview of which networks the fleet is actually on
    count by (chain_id) (tron:node_info_info)
    
    # alert when a node still runs an old version after an upgrade window
    tron:node_info_info{version!="<expected release>"} == 1
    

    There is also a consistency argument: without the metric, every operator would have to build a custom poller/exporter that converts /monitor/getnodeinfo responses into time series before Grafana could alert on them — which is exactly the kind of duplicated glue code this cleanup track is trying to remove. Exposing static info as an Info metric is a standard pattern (and java-tron already does it for JVM/OS info via the built-in Prometheus collectors).

    For what it's worth, the two fields were requested by node operators for our Grafana dashboards: show the running node version, and use chain_id to quickly tell whether a node is connected to mainnet or a testnet. If one only wants to display the version, a Grafana JSON API datasource querying the HTTP endpoint would technically work, but that is not the standard alerting path and requires per-panel setup.

    So the metric complements the existing API rather than duplicating it: the API serves interactive queries, the metric serves automated monitoring and alerting.

  3. waynercheung commented on Sep 2, 2026

    @waynercheung
    Collaborator

    Thanks for the detailed write-up. I support consolidating on Prometheus, but I think two blockers and a few migration details should be settled before implementation.

    1. Stage the public API removal

    node.metricsEnable has been documented as deprecated since v4.8.2, but that does not explicitly deprecate the Monitor gRPC service or /monitor/getstatsinfo. Monitor still returns data when the flag is enabled, so unregistering it is an observable breaking change.

    To follow principle 2 of #6921, I suggest:

    • next release: mark service Monitor and message MetricsInfo deprecated, emit a startup deprecation warning pointing to node.metrics.prometheus.enable, publish migration guidance, and keep both APIs functional;
    • a future major release: remove the endpoints, proto definitions, legacy implementation, and Dropwizard dependency.

    This differs from #6931: WalletExtension already returns UNIMPLEMENTED, whereas Monitor is functional when enabled.

    2. Keep block-fetch latency as the peer-selection signal

    The per-IP histogram is updated independently of node.metricsEnable in BlockMsgHandler and consumed only by FetchBlockService; it is functional scheduling state, not part of getstatsinfo.

    Please move that state instead of replacing it with libp2p avgLatency. The latter is a connection-lifetime mean of keepalive RTT, sampled only after 20 seconds without outbound traffic, and remains 0 before the first pong. shouldFetchBlock, however, interprets its estimate as block-fetch latency and subtracts elapsed fetch time from it. Substituting RTT therefore changes the signal, sampling, and zero-value behavior; it is neither equivalent nor demonstrably conservative.

    A bounded block-fetch estimator on PeerConnection—for example, a small rolling reservoir with a cached P75—could be updated beside the Prometheus observation. Please cover unsampled peers, reconnect/reset, timeout fallback, and duplicate-request prevention in tests.

    3. Metric and migration details

    • simpleclient Info appends _info; use tron:node as the collector base name (as in the draft) and query tron:node_info.
    • The proposed full value is a genesis block ID, whereas eth_chainId returns only its last four bytes. Prefer genesis_block_id over chain_id.
    • Include a legacy-field-to-Prometheus mapping, including fields without an equivalent, in the PR description and release notes, and land the documentation-en/zh updates before release.

    For reviewability, I suggest separate PRs for (1) block-fetch state migration and tests and (2) deprecation/migration plus tron:node_info. The actual removal can follow in the later major release.

  4. warku123 commented on Sep 4, 2026

    @warku123
    ContributorAuthor

    Thanks for the careful review — both blockers accepted. Responses below.

    1. Staged decommission of the Monitor API — accepted.

    Phase Behavior
    Next release node.metricsEnable keeps working exactly as before: when enabled, the gRPC Monitor service stays registered and /monitor/getstatsinfo keeps serving. Add a prominent startup deprecation warning, mark the config key and both endpoints deprecated in docs and release notes, and publish the legacy field mapping table.
    Following release Remove the legacy stack: the config key (ignored harmlessly by the typesafe config afterwards), the gRPC service (UNIMPLEMENTED), the HTTP endpoint (404), and the Dropwizard dependency.

    This follows principle 2 of #6921 (public APIs deprecated for one release cycle before removal) while committing to an actual removal date instead of an indefinite "future major".

    2. Fetch-block latency signal — accepted.

    Agreed: RTT alone does not capture block-serving throughput, so peer selection should keep a measured fetch-duration signal. #6923 has been updated to propose a bounded, per-connection estimator:

    • a single volatile long per PeerConnection, updated on every fetched block with the measured request-to-block duration: (ewma * 9 + last) / 10, clamped to fetchBlockTimeout;
    • the first sample is seeded from the channel RTT (Channel.getAvgLatency()) so a fresh connection never sits in an unknown/zero state;
    • the state lives and dies with the PeerConnection object — bounded by the active peer set, no disconnect bookkeeping, nothing stored in the metrics registry;
    • the Prometheus histogram stays unlabeled and observational only (a per-peer label would reproduce the unbounded-cardinality problem in the TSDB).

    One factual note for the record: Channel.getAvgLatency() is seeded at handshake completion and updated on keepalive pongs (pings fire only after 20s without outbound traffic), so it is not stuck at zero — but we agree it measures proximity, not serving quality, which is exactly why the estimator above becomes the selection signal.

    Other accepted points

    • Single PR: the bounded fetch-duration estimator + peer-selection migration land together with the legacy stack removal and tron:node_info{version, genesis_block_id} — the estimator replaces the histogram's functional consumer, so it is intrinsic to this cleanup rather than separable. Commits stay logically separated for review.
    • Rename the label chain_id → genesis_block_id.
    • Legacy field mapping table in the PR description and release notes; docs (en/zh) updated alongside.
  5. waynercheung commented on Sep 4, 2026

    @waynercheung
    Collaborator

    @warku123 Thanks, this resolves most of my concerns, and thanks for correcting the handshake detail. I verified that HandshakeService seeds avgLatency at handshake completion, so I retract my earlier statement that it remains zero until the first pong.

    A few points still need alignment before implementation:

    1. Make the two phases consistent. Tracking: code refactor and cleanup #6921 requires removal in a future major release. "Following release" is fine only if that release is the agreed major; otherwise the exception should first be agreed in Tracking: code refactor and cleanup #6921. The current [Feature] Remove the legacy Monitor API and non-Prometheus metrics implementation #6923 description still specifies direct removal, a single removal PR, retained proto definitions, and silent handling of the removed config key. Please update it to reflect:

      • Phase 1 PR: migrate the estimator, remove the per-IP histogram read and write sites, add tron:node_info, add option deprecated = true to Monitor and MetricsInfo, emit deprecation warnings, and publish the mapping/docs while keeping both APIs functional.
      • Phase 2 PR in the target major: remove the HTTP/gRPC implementations, Dropwizard stack, and proto definitions, following the WalletExtension staging documented in Tracking: code refactor and cleanup #6921.

      Because /monitor/getstatsinfo is registered independently of node.metricsEnable, a warning based only on config-key presence may miss HTTP consumers. Please also emit a process-once WARN when either deprecated API is first called. The removal release should retain a tombstone warning when node.metricsEnable is present, so operators who skip Phase 1 do not lose monitoring silently.

    2. Keep EWMA initialization explicit. Using the channel latency estimate as a cold-start fallback is reasonable, but it should not enter the EWMA as if it were an observed block fetch. Keep an explicit unsampled state, use channel latency only as the fallback, let the first real fetch replace it, and apply EWMA from subsequent samples.

      With the current shouldFetchBlock logic, clamping estimates to fetchBlockTimeout also makes the > fetchBlockTimeout branch unreachable while allowing an estimate equal to the timeout through the <= candidate filter. Please either preserve an over-timeout state or update the comparisons explicitly, with boundary tests.

    3. Validate the observable behavior change. The path is not categorically dormant in steady state: normal head+1 block retrieval arms FetchBlockService, whose 50 ms worker may evaluate a secondary fetch while tracking remains active. Please instrument the normalized secondary-fetch rate and duplicate-block response count without peer labels, then report a before/after result from a sufficiently long Nile soak or a controlled delayed-peer experiment. The validation environment can be selected based on the observed event volume, but the test should not be dismissed on the premise that the path is dormant.

    Minor documentation cleanup: describe version as "not currently exposed to Prometheus" rather than "lost", call genesis_block_id the full genesis block ID, and update the earlier PromQL examples to tron:node_info{genesis_block_id="..."}.

    With these updates, I'm +1 on proceeding with Phase 1.

  6. warku123 commented on Sep 8, 2026

    @warku123
    ContributorAuthor

    Thanks — and thanks for verifying the handshake seeding detail on your side.

    Agreed on the overall structure. Going point by point:

    1. Two-phase consistency. We'll update this issue to the Phase 1 / Phase 2 narrative:

    • Phase 1: migrate the fetch-latency estimator and remove the per-IP histogram read/write sites; add tron:node_info{version, genesis_block_id}; mark the Monitor service and MetricsInfo message with option deprecated = true in the protos; emit deprecation warnings — both for the node.metricsEnable config key and as a process-once WARN on the first invocation of either deprecated API (agreed that the HTTP endpoint is not config-gated, so a key-based warning alone would miss HTTP consumers); publish the legacy field mapping table and docs (en/zh). Both APIs remain fully available.
    • Phase 2 (in the agreed major release): remove the HTTP/gRPC implementations, the Dropwizard stack, and the proto definitions, following the WalletExtension precedent from Tracking: code refactor and cleanup #6921; keep a tombstone warning when node.metricsEnable is still present, so operators who skip Phase 1 don't silently lose monitoring.

    The concrete target major release needs an internal alignment round on our side — we'll update this issue (and the #6921 cross-references) as soon as it's confirmed.

    2. EWMA initialization. Agreed — channel RTT is a different quantity and should not enter the series as an observation. We'll make the unsampled state explicit: channel latency serves only as a read fallback while unsampled, the first real fetch replaces the placeholder, and EWMA starts from the second sample. (The current implementation blends the first sample at α = 0.1 as outlier defense; we'll switch it to replace-on-first-sample and adjust the pinned tests accordingly.)

    For the steady-state EWMA weight we plan to keep α = 0.1 (each new sample contributes 10%): at the expected sampling cadence a large degradation flips the min() ordering within one or two samples, and the clamp plus the hard-timeout branch bound the downside, so the conservative weight mainly buys smoothing against per-sample noise (fetched block sizes vary by orders of magnitude). Would you see a reason to weight recent samples more aggressively here?

    Separately — the clamp/reachability issue you flagged is already fixed in the current implementation: shouldFetchBlock now has an explicit hard-timeout branch (unconditional switch once the wall-clock budget is exhausted) plus a latency-saturation gate that requires a strictly better candidate, with boundary tests covering both. Your comment referenced the earlier issue text.

    3. Verification. Agreed that this cannot be skipped. We'll add the two unlabeled metrics (normalized secondary-fetch rate and duplicate-block response count) and report before/after from a controlled experiment with delayed peers; based on the observed event volume we'll assess whether a longer Nile soak adds signal beyond the controlled run.

    Docs nits all accepted: describe version as "not currently exposed to Prometheus" rather than lost; describe genesis_block_id as the full genesis block ID; update the PromQL examples to tron:node_info{genesis_block_id="..."}.

    We'll follow up with the issue description rewrite and a Phase 1 PR reflecting all of the above.

  7. waynercheung commented on Sep 8, 2026

    @waynercheung
    Collaborator

    @warku123 Thanks, we are aligned. Three short points, then I will leave the remaining implementation details to the PR.

    Alpha. I would not increase alpha a priori. With replace-on-first-sample, the first measurement enters unsmoothed, and at 0.1 it can dominate the estimate for several subsequent fetches, so one unusually large block may skew a peer's early ranking.

    If you want warm-up protection, use a running mean for the first ten measured samples, equivalent to alpha_n = max(1.0 / n, 0.1), and then continue with the fixed EWMA. The sample counter can be capped at ten.

    Either approach is acceptable for Phase 1. Please make the weight a named constant, document the rationale, and pin both degradation and recovery convergence in tests.

    Selection logic. Agreed that my earlier comment referred to the previous issue text. Until the PR is opened, the revised issue should remain the source of truth. Please describe the final candidate selection, saturation gate, hard-timeout branch, and tested boundary cases there.

    One intentional behavior change should also be listed explicitly under principle 1 of #6921: previously, a candidate whose P75 exceeded fetchBlockTimeout was filtered out. With the new clamped EWMA, a saturated candidate remains eligible and may receive the secondary request at the hard timeout when no better candidate exists. I think this is a reasonable liveness improvement, but it should be documented and tested rather than left implicit.

    Verification and timing. The controlled experiment works for me. Please let the estimator complete its warm-up, then degrade one peer during the run and report how many additional per-peer samples are required for the ordering to change. Use the same harness and topology for the develop baseline. If the one-or-two-sample claim is retained, please state and demonstrate the latency ranges for which it holds. Exact primary/secondary attribution for the experiment can come from the harness itself.

    Expose the underlying events as unlabeled, monotonically increasing Prometheus counters: fetch trackings armed, secondary fetches sent, and duplicate or already-known block responses. Derive normalized rates in PromQL rather than precomputing them on the node.

    Please define the duplicate-response event precisely in the PR. A matching advInvRequest.remove(item) combined with the block already being present can provide a best-effort "requested response for an already-known block" signal, but it does not by itself prove that the response was caused by a secondary fetch and may miss concurrent arrivals. If exact attribution is required, the secondary origin should be recorded explicitly in bounded request metadata; otherwise the metric should be named and documented as a generic already-known response counter.

    Phase 1 should target the next release so that the API deprecation window starts independently of the Phase 2 schedule. Once the target major is agreed, please record it in #6921 and cross-reference it here.

    Looking forward to the Phase 1 PR.

  8. warku123 commented on Sep 9, 2026

    @warku123
    ContributorAuthor

    Thanks — confirmed on all points. The remaining details will be folded into the Phase 1 branch before the PR opens.

    Counters. The third counter will be added: tron:block_fetch_armed (incremented when FetchBlockService arms a fetch tracking), alongside tron:block_fetch_secondary and tron:block_already_known. Per your precision note, the duplicate counter will be renamed to tron:block_already_known and documented as a best-effort "response for an already-known block" signal — we do not record secondary origin in request metadata, so it covers concurrent arrivals and any low block, not just secondary-fetch responses.

    On lifecycle: these three counters are verification-scoped instrumentation — they ship with Phase 1 so the before/after comparison and the deprecation-window soak both have identical signals, and they are scheduled for removal in Phase 2 once the results are recorded in this issue. The permanent fetch-latency signal is the existing unlabeled tron:block_fetch_latency_seconds histogram, which stays.

    Alpha. α stays fixed at 0.1, expressed as a named constant (no pre-tuning, no warm-up running mean, per your note that both options are acceptable; the simplicity trade-off is stated in the PR). The constant carries the design rationale in a comment, and degradation/recovery convergence tests pin both directions.

    Selection logic. The issue will be rewritten as the source of truth before the PR opens: final candidate selection, saturation gate, hard-timeout branch, and the measured boundary cases. The deliberate behavior change (saturated candidates stay eligible instead of being filtered like P75 > fetchBlockTimeout was) is listed explicitly under principle 1 of #6921, with tests.

    Verification. As specified: warm up the estimator first, then degrade a single peer mid-run (tc netem) and report how many additional per-peer samples it takes for the ranking to flip; the develop baseline runs the same harness and topology, with the counters cherry-picked onto an instrumentation-only branch so both arms expose identical metrics. We will either qualify the "1–2 samples to flip" claim to the latency range where it holds, or replace it with measured numbers. Verification results will be posted back to this issue.

    Timing. Phase 1 targets the next release so the deprecation window starts independently of Phase 2; once the target major is agreed, we will record it in #6921 and cross-reference it here.

  9. waynercheung commented on Sep 9, 2026

    @waynercheung
    Collaborator

    @warku123 Thanks. The fixed alpha and the controlled comparison against an instrumentation-only develop baseline work for me. Two clarifications on the counters before we move the remaining review to the PR:

    1. Please keep these counters beyond Phase 2. They describe fetch behavior that remains after the legacy stack is removed, and operators need them to detect regressions after release. The existing latency histogram cannot recover the secondary-fetch ratio or the already-known response count. Removing them later should be a separate observability decision, rather than part of the Dropwizard cleanup.

    2. Please narrow the definition of tron:block_already_known. A block below the current head is not necessarily already known; it could be an unseen fork block. For the proposed response counter, count a matching outstanding adv request whose exact block ID is already known before processing that response. Document that concurrent arrivals may be missed and that this does not establish secondary-fetch attribution. Exact attribution can remain in the experiment harness.

    With those clarifications reflected in the issue rewrite, I'm +1 on proceeding with the Phase 1 PR. Please post the verification results here before merge so they can inform the final review.

  10. warku123 commented on Sep 15, 2026

    @warku123
    ContributorAuthor

    Verification results from the controlled experiment, as discussed.

    Setup. 4-node private chain (1 witness producer, 1 observer, 2 candidate peers) in docker on a single host. The observer's requests to the producer were degraded with source-IP-filtered tc netem (only the observer→producer direction was delayed; candidates were untouched). Both arms ran the same harness and topology: baseline = develop + the three counters on an instrumentation-only branch; treatment = the Phase 1 branch. Each delay step ran ~45s (≈15 blocks at 3s), ascending order.

    How to read the table. Each cell reports how many of the ~15 fetches armed in that window ended in a secondary-fetch switch — the observer giving up on the (degraded) producer and re-requesting the same block from a healthy candidate. 0/15 means every fetch in the window kept hitting the degraded producer; 15/15 means every armed fetch switched away.

    Secondary-fetch switches per window:

    added delay (observer→producer) develop + P75 Phase 1 EWMA
    0 (control) 0/15 0/15
    10ms 0/15 0/15
    50ms 8/15 0/15
    100ms 0/15 † 1/15
    150ms 0/15 † 15/15
    200ms 0/15 † 15/15
    300ms 0/15 † 15/15
    500ms 15/15 † 15/15
    1000ms 15/15 † 15/15
    2000ms 15/15 † 15/15

    † Baseline points at 100ms and above come from an earlier round with the same harness; the low-tier points (≤50ms) and the treatment arm's 1000/2000ms points come from a later round on an updated branch (the peer-selection logic is identical — only the already_known write point differs between the two treatment builds).

    Observations.

    • In both arms, every switch targeted the same healthy candidate, and each block produced at most one secondary request — no thrashing between candidates.
    • In the treatment arm, switches fired ~25–50ms after arming (as soon as a candidate advertisement was available), well before the hard timeout; the baseline switched at comparable points once its P75 crossed the fetch timeout.
    • Healthy segments — including two producer pauses and one all-direction degraded window with no eligible candidates — produced zero secondary fetches on either arm; the no-candidate clearing path handled the latter without issuing requests.
    • After the 2000ms window was released, ~44 subsequent healthy blocks produced zero switches: the estimator recovered with no residual switching.
    • Chain production and sync were unaffected in every window on both arms.

    The 50ms row is worth calling out. The baseline switched in 8/15 fetches at 50ms in this run, yet switched 0/15 at 100ms in the earlier round — same build, same harness. This is the empty-histogram defect in action: a candidate that has never served a fetch reads P75 = 0.0 (a registry lookup auto-creates an empty histogram), so an unsampled candidate is always ranked "fastest" and the comparison branch degenerates into a race between the 50ms worker tick and the producer's P75, decided by the composition of the exponentially-decaying reservoir (how many low samples are stacked below the new degraded ones). Whether a given run switches is history-dependent. The EWMA arm reads unsampled candidates with the channel-RTT fallback instead of 0, so its noise floor is clean: no switching at 50ms, and consistent switching once degradation actually pushes the estimate past the comparison band.

    Calibration.

    • This is a single ascending run per arm — we report it as an observed divergence band under this topology, not as fixed universal thresholds.
    • The private chain uses fetchBlockTimeout = 200; the mainnet default is 500, so absolute thresholds scale proportionally.
    • Same-host baselines are ~1ms with near-empty blocks, so bandwidth variance (a real factor with full-size blocks) is not exercised; degradation was constant delay rather than jitter or loss.
    • already_known was validated live at the high-delay steps (switches and late duplicate responses matched one-to-one under the exact-ID definition); low-delay windows produced no such responses, as expected.

    In every tested regime the new estimator was at least as good as the old signal, and strictly better in the mid-degradation band; the structural wins (O(1) read, bounded per-connection memory, no metrics-gate bypass, no unbounded per-IP keys) hold regardless of topology.

    Next: the Phase 1 PR will be opened against develop referencing this issue, and its link and CI status will be synced here.

  11. waynercheung commented on Sep 16, 2026

    @waynercheung
    Collaborator

    Thanks for running this and updating the issue. The reported no-thrashing and no-candidate behavior are useful results. My support for opening the Phase 1 PR stands; three points need clarification before these results support the final merge decision.

    1. Please reconcile the 300 ms baseline row. With fetchBlockTimeout = 200, a tracking that remains pending should trigger a secondary request at a subsequent worker tick if an eligible candidate is available (the oldPeerSpendTime >= fetchTimeOut branch in shouldFetchBlock). Eligibility requires more than an advertisement: the candidate must also be idle and pass the baseline P75 filter. Please state the effective timeout and exact commits for each round, and provide enough decision traces to explain the 0/15 result. Repeat the key comparisons under matching configuration and warm-up conditions, including representative cases at the default 500 ms timeout.

    2. Please report the cost and benefit together. Include armed, secondary, and already-known counts per window for both arms, alongside time from the initial request to the first usable block and the extra response traffic. More secondary requests may reduce latency while increasing duplicate transfers; the switch fractions alone do not establish "at least as good in every regime".

    3. Please show the estimator's adaptation directly. The ascending windows share history, so a 15/15 window does not establish how many new samples caused the decision to change. Record the old and candidate estimates, elapsed fetch time, and decision reason during degradation and recovery, and include a case with a controlled, non-negligible candidate RTT so the fallback and the estimate actually compete beyond the same-host setup. Then update or remove the "1-2 samples" statement in the issue accordingly. Similarly, zero switches during the subsequent healthy blocks does not by itself establish that the estimate recovered; prompt arrivals may simply have cleared tracking before a decision was made. The fixed worker interval and other timing inputs also mean that neither proportional threshold scaling nor an unchanged mainnet threshold follows from this experiment.

    Please link the harness, configurations, and raw results in the PR. These targeted checks should give us the evidence needed to assess the latency/overhead tradeoff before merge.

  12. warku123 commented on Sep 18, 2026

    @warku123
    ContributorAuthor

    @waynercheung

    Reply v8 — Round-3 re-run: baseline vs phase1 switch behavior under added latency

    1. Background: scope of this reply

    This reply follows up on the three points from the last review comment (2026-09-16). As requested, the key comparisons were repeated under matching configuration and warm-up conditions, including the default 500 ms timeout: a full re-collection of both builds, with each build collected at two effective fetchBlock.timeout values, 200 ms and 500 ms.

    The repeated runs use the same per-window controls in both builds:

    • jar identity is pinned and verified at launch, with a fetch-block-service / PeerConnection instrumentation check that differs per build;
    • containers are recreated before the run and verified (mounts, jar match, and configuration read-back);
    • each measurement tier takes counter snapshots strictly before and after the netem window, and the netem on/off operations are timestamped;
    • warm-up is gated and primary-peer reachability is confirmed per tier;
    • the timeout configuration is confirmed by in-container HOCON read-back: the t200 runs use fetchBlock.timeout = 200 and the t500 runs use fetchBlock.timeout = 500, so both timeout settings are stated directly;
    • each run writes to its own UTC-stamped run directory and the chain is fail-stop.

    Coverage of the three points in this reply: point 1 is covered — the repeat under matching conditions including the 500 ms default, and the per-window decision mix (the histogram columns in §3 are the aggregated form of the decision traces). Point 2 is partly covered — the per-window armed and secondary counts are in the §3 table (armed(Δ) and secondaryΔ/putΔ). Point 3 — the per-window decision breakdown in section 3 is the direct answer here; nothing further is required to evaluate the replacement.

    • baseline: one run per timeout setting (t200, t500), both completing without error;
    • phase1: one run per timeout setting (t200, t500), both completing without error.

    2. Method

    Each build runs two timeout settings: t200 = fetchBlock.timeout 200 ms, t500 = 500 ms (the default). 14 windows per build, 28 windows total (2 builds × 2 timeouts × 7 tiers). Each window is one tier delay applied to the node-1↔peers path for 45 s while the node fetches blocks; 15 fetch requests are armed inside the window (armed counter POST−PRE = 15 throughout, except two phase1 windows noted below).

    Reduction (script reduce_r3.py):

    • perTier.tsv is parsed by metric name (column order is not stable).
    • The trace window is the netem on/off interval from the metrics snapshot, mapped through the time of day onto the container clock (HH:mm:ss.SSS, UTC), with ±1 s tolerance. A window that would cross UTC midnight is rejected outright.
    • Numerator:
      • baseline build: the window's lines recording a switch to the secondary peer being applied, with the paired annotation asserted equal;
      • phase1 build: the window's lines recording a switch decision with the send flag set.
    • Denominator: the armed-counter POST−PRE delta.
    • Cross-checks (hard BLOCKED, never smoothed): trace window count vs the secondary-counter POST−PRE delta; switch-count consistency; each build's own trace markers must not appear in the other build's traces; armed delta must be 15 for baseline.

    Every tier in both baseline runs passed all cross-checks. One phase1 cell is BLOCKED and is reported as-is (section 3).

    3. Results (28 windows)

    The table has four series — the 4 build × timeout combinations, named in the series column — and 7 rows per series, one per tier: the tier is the additional netem delay applied for that 45 s window. For each row, armed(Δ) is the window's tron:block_fetch_armed_total POST−PRE delta, i.e. the fetch requests armed inside the window; switched is the numerator — the baseline side counts put=true switches to the secondary peer, the phase1 side counts sent=true switch decisions; and ratio is switched ÷ armed(Δ).

    The histogram columns count trace events inside the window by classification, not requests: the baseline side classifies them by the branch= field, the phase1 side by the decision= field; fail occurs only in baseline rows and comparison-fail / saturation-gate only in phase1 rows, while the remaining classification names are shared columns that each row fills from its own taxonomy and leaves 0 otherwise. Because they are event counts, they need not add up to armed(Δ). The last column, secondaryΔ/putΔ, is the tsv tron:block_fetch_secondary_total POST−PRE delta and equals the numerator in every non-BLOCKED cell — the cross-check; the one BLOCKED row is explained below the table.

    series tier(ms) armed(Δ) switched ratio hard-timeout comparison-pass comparison-fail fail saturation-gate no-candidate secondaryΔ/putΔ
    base-t200 100 15 13 13/15 0 13 0 4 0 1 13
    base-t200 150 15 12 12/15 0 12 0 9 0 2 12
    base-t200 200 15 12 12/15 12 0 0 12 0 3 12
    base-t200 300 15 15 15/15 15 0 0 11 0 2 15
    base-t200 500 15 15 15/15 15 0 0 11 0 2 15
    base-t200 1000 15 15 15/15 15 0 0 15 0 1 15
    base-t200 2000 15 15 15/15 15 0 0 11 0 1 15
    base-t500 100 15 12 12/15 0 12 0 6 0 3 12
    base-t500 150 15 12 12/15 0 12 0 9 0 0 12
    base-t500 200 15 12 12/15 0 12 0 11 0 3 12
    base-t500 300 15 12 12/15 0 12 0 17 0 2 12
    base-t500 500 15 12 12/15 12 0 0 29 0 0 12
    base-t500 1000 15 15 15/15 15 0 0 30 0 2 15
    base-t500 2000 15 15 15/15 15 0 0 28 0 3 15
    phase1-t200 100 15 0 0/15 0 0 28 0 0 2 0
    phase1-t200 150 16 11 11/16 0 11 11 0 0 3 11
    phase1-t200 200 — — BLOCKED — — — — — — —
    phase1-t200 300 15 14 14/15 2 9 8 0 3 3 14
    phase1-t200 500 15 15 15/15 1 4 4 0 10 4 15
    phase1-t200 1000 15 15 15/15 1 2 4 0 12 2 15
    phase1-t200 2000 15 14 14/15 1 0 4 0 13 2 14
    phase1-t500 100 15 0 0/15 0 0 29 0 0 2 0
    phase1-t500 150 15 4 4/15 0 4 32 0 0 2 4
    phase1-t500 200 15 12 12/15 0 12 12 0 0 3 12
    phase1-t500 300 15 12 12/15 0 12 12 0 0 2 12
    phase1-t500 500 15 13 13/15 1 12 19 0 0 3 13
    phase1-t500 1000 15 14 14/15 1 6 9 0 7 1 14
    phase1-t500 2000 15 15 15/15 1 2 10 0 12 3 15

    phase1-t200 tier 200: the ±1 s window contains 8 sent=true decisions while the tsv counter delta is 7. The extra decision line sits 37 ms after the NETEM_OFF / POST-counter snapshot timestamp (01:54:00.037 vs TS_UNIX=1789696440), i.e. a decision logged just after the POST snapshot was taken; the corresponding counter increment is not in the POST value. Per the reduction rules this is a cross-check mismatch and the cell is emitted as BLOCKED with the raw counts; it is not smoothed into either number. The remaining 27 cells are consistent.

    Two phase1 windows (t150 and t200 of the same run) show armed delta 16 instead of 15 (boundary armed event recorded on the other side of a snapshot); these windows keep the actual delta as denominator and are flagged as warnings. All baseline windows are 15.

    4. Findings

    Stated strictly within what the 28 windows support:

    1. Baseline saturates from 300 ms in the t200 group and from 1000 ms in the t500 group, reaching 15/15 in those segments.
    2. Baseline records 12–15 hard-timeout events per window at 200–2000 ms (t200 group) and 500–2000 ms (t500 group); phase1 records at most 2 in the same ranges.
    3. At 100 ms the baseline switches 13/15 and 12/15 while the phase1 build records 0/15, with comparison-fail dominated decisions.
    4. The two phase1 runs differ at the 150 ms tier, 11/16 vs 4/15.

    5. Overall conclusion

    The 28-window round-3 data supports replacing the original selection logic with the new computation. Three reasons:

    1. Switch coverage is not reduced. In every latency region where the baseline build saturates, the saturation segments from 300 ms upward in the t200 group and from 1000 ms upward in the t500 group, the phase1 build switches on 14–15 of the 15 armed requests, matching or coming close to the baseline's 15/15.
    2. The main failure mode is eliminated. In those same segments the baseline records 12–15 hard-timeout events per window, waiting out the timeout before switching; the phase1 build switches proactively on the saturation-gate decision, hard-timeout drops to at most 2 per window, and every saturation-gate switch has sent=true. Hard-timeout remains in place as a fallback path and stays active.
    3. Gate behavior is by design, not a defect. In the 100 ms tier the phase1 build records 0/15 switches and its decision records are dominated by comparison-fail, meaning no switch when the candidate estimate is not better; no-candidate events are all sent=false and consume no switch budget. Across all 28 windows both builds kept the chain moving with no stall, and no functional regression was observed. Which build fares better in this tier is an end-to-end question outside this experiment's scope.

    Supplementary characterization is not a prerequisite for the replacement judgment. The single BLOCKED cell and the 150 ms run-to-run variance reported verbatim in section 3 are data-integrity notes and do not change the conclusion above.

    One-line conclusion: within the tested envelope, the new computation preserves the switching capability of the original selection logic and removes its main failure mode; the current data supports the replacement.

  13. waynercheung commented on Sep 21, 2026

    @waynercheung
    Collaborator

    Thanks for the full re-collection. The round-3 results address the earlier 300 ms baseline concern for the current comparison, and should supersede the mixed-round comparison. Explicitly reporting the BLOCKED cell is also helpful.

    Two points remain before drawing the replacement conclusion:

    1. The baseline label does not establish timeout waiting. In instrumentation/trace-counters, hard-timeout covers both oldPeerTop75 > fetchTimeOut and oldPeerSpendTime >= fetchTimeOut, whereas Phase 1 separates the saturation gate from the elapsed timeout. Their counts therefore cannot directly demonstrate that timeout waiting was eliminated. Please confirm the run commits for both instrumentation branches. If they match the inspected instrumentation, the existing spendMs / spendTimeMs fields on successful-switch lines should allow an approximate arming-to-secondary-dispatch comparison for both arms without rerunning the experiment.

    2. Please explain the 100/150 ms decisions from the recorded state. comparison-fail does not necessarily mean the candidate estimate was worse; the comparison uses the estimated remaining time and the 0.5 factor. instrumentation/trace-phase1 already records spendTimeMs, oldEstimateMs and candidateEstimateMs per decision, and updateFetchLatency logs every estimator update. Please extract representative sequences, including window-entry state and whether the candidate was still on its channel-latency fallback. The same update lines should support or qualify the issue's "1-2 samples to clamp" claim; aggregate branch counts cannot establish it.

    The outcome check from the previous review remains: for representative matched windows, including the default 500 ms timeout, please report already-known counts alongside armed/secondary counts, initial-request-to-first-usable-block time, and extra response traffic. Fewer secondary requests may trade latency for lower overhead, but the current table does not establish either outcome. There is no need to expand the full delay matrix.

    Please narrow the performance conclusions to what those measurements support, and link the exact commits, harness, reduce_r3.py, configurations, and raw results in the PR.

    I remain supportive of opening the Phase 1 PR. The remaining evidence can be reviewed there before the merge decision.

  14. removed this from the GreatVoyage-v4.8.3 milestone on Sep 24, 2026
  15. warku123 commented on Sep 25, 2026

    @warku123
    ContributorAuthor

    @waynercheung Both points answered from the same 28 round-3 windows (no rerun), plus the outcome check. Numbers below are extracted by the analysis-r4 scripts over the four canonical run directories; cross-checks passed 30/30 (analysis-r4/verify.md) and 28/28 (per-block reconciliation in outcome-latency.md).

    1. Run identity

    series jar sha256 instrumentation branch commit
    base-t200, base-t500 c6f7f91af8d2ef5905ee3ca3e76b0377c92791e9a4d8553829b9b2c5ddee68c8 instrumentation/trace-counters acefb07179912d8d943bc9ed4ab76fcae96f20ff
    phase1-t200, phase1-t500 ac84cb6411c2e7af52f113c8c221ca9235e35784c93d5ba7d6404520dfdb0d8f instrumentation/trace-phase1 2a49bdb60808aef3ef1d044a14f4d61158c7eee6

    Jar SHA-256s are taken from each run's JAR_IDENTITY log line. Caveat stated plainly: the jar↔commit correspondence is by build convention — chain7-baseline.sh / chain7-phase1.sh build the named jar from the checked-out branch and hand it to the node; the jars were not retained after the runs, so the mapping is a convention mapping, not a re-hash. Both instrumentation branches were verified clean and equal to their remote SHAs at analysis time.

    2. Point 1 — timeout waiting, read from the spend fields

    You are right that the counts alone cannot establish it. The spendMs (baseline) / spendTimeMs (phase1) fields on successful-switch lines resolve it, and they cut in both directions:

    • Baseline hard-timeout switches do not predominantly wait out the timeout: median spendMs is 21–48 ms across every tier — those are the early oldPeerTop75 > fetchTimeOut triggers (the first of the two conditions you named), not the elapsed one. Elapsed-timeout waiting appears only in the tail: p90 ≈ 218–227 ms (t200 group, tiers ≥ 300) and ≈ 513–531 ms (t500 group, tiers 1000/2000), i.e. at the configured timeout.
    • Phase1 hard-timeout switches shrink to 8 events across both runs combined, and all 8 sit at full length (233–250 ms in t200; 500–513 ms in t500). The switch load moves to saturation-gate (median 8–41 ms) and comparison-pass (median 22–53 ms).

    Pooled per-tier medians (both runs per arm):

    tier (ms) base hard-timeout med p1 hard-timeout med p1 saturation-gate med
    300 33.0 236.5 8.0
    500 35.0 366.5 31.5
    1000 46.0 377.0 38.0
    2000 37.0 377.0 35.0

    So the corrected statement is: median arming→secondary-dispatch is similar in both arms (tens of ms); what changes is the tail. Baseline pays ≈ full-timeout waits on its p90 tail at high tiers; phase1 reroutes those decisions through the saturation gate / estimator comparison (typically ≤ ~60 ms), leaving the hard timeout as a fallback that fires at most twice per window and then at full length.

    3. Point 2 — the 100/150 ms decisions, from recorded state

    Window-entry state (phase1-t200, tier 100, first decision lines of the window):

    decision=comparison-fail spendTimeMs=16 oldEstimateMs=4.0 candidate=/172.22.0.13 candidateEstimateMs=52.0 sent=false
    decision=comparison-fail spendTimeMs=66 oldEstimateMs=4.0 candidate=/172.22.0.14 candidateEstimateMs=46.0 sent=false
    updateFetchLatency peer=/172.22.0.11 firstSample=false oldMs=4 sampleMs=105 newMs=14
    

    The old peer enters with a warmed EWMA (4 ms, from pre-window samples); the candidates are still on their fallback values (46–75 ms across these windows; the channel-RTT fallback per the design). One 105 ms sample moves the EWMA to 14 ms — α = 0.1 closes 10% of the gap per sample.

    Representative convergence (phase1-t200, tier 150, 01:51:12–01:51:21): three updateFetchLatency lines interleave with comparison-fail decisions — 5.0 → (s=152) 19 → (s=153) 32 → (s=152) 44 — and the next fetch opens with:

    decision=comparison-pass spendTimeMs=13 oldEstimateMs=44.0 candidate=/172.22.0.14 candidateEstimateMs=8.0 sent=true
    

    phase1-t500 tier 150 behaves the same at a higher plateau: 86 → 92 → 98 → 103 across three samples, then comparison-pass spendTimeMs=6 oldEstimateMs=103.0 candidateEstimateMs=41.0.

    The fail/pass pair with identical estimates is the direct answer to "comparison-fail does not necessarily mean the candidate estimate was worse":

    01:51:18.169 decision=comparison-fail spendTimeMs=152 oldEstimateMs=44.0 candidateEstimateMs=8.0 sent=false
    01:51:21.025 decision=comparison-pass spendTimeMs=13  oldEstimateMs=44.0 candidateEstimateMs=8.0 sent=true
    

    Same estimates, different elapsed time — the outcome tracks the estimated-remaining-time comparison you described (with the 0.5 factor), not the raw estimate ordering.

    Absence, attested rather than inferred: saturation-gate / hard-timeout decisions with sent=true occur only at tier ≥ 300 (t200) / ≥ 500 (t500); tiers 100–150 record none, and tier 100 records zero comparison-pass in both runs. When the hard timeout does fire, the old estimate is far below the cap (t200 tier 300: spend 235 ms, oldEstimate 6; t500 tier 500: spend 500 ms, oldEstimate 54) — a wall-clock trigger, not estimator saturation. In the latter case the following update shows the α = 0.1 pace explicitly: oldMs=54 sampleMs=504 newMs=99.

    On the "1–2 samples to clamp" claim, the update lines split it:

    • For an unsampled connection it holds by construction — the first measurement replaces the fallback with the clamped sample (firstSample=true oldMs=0 sampleMs=3 newMs=3 is the logged small-scale case; a first sample above the timeout clamps in one step).
    • For a warmed estimator it does not: at α = 0.1, reaching 0.8×tier from a low start needs ≈ 16 samples at the tier value, and the 45 s windows supply only 13–16 samples for the delayed peer (fewer for the candidates). In all 35 run×tier×peer cells the estimate never reaches 0.8×tier inside the window — 18 cells are structurally unreachable (0.8×tier above the run's clamp: t200 tiers ≥ 300, t500 tiers ≥ 1000), and 17 are sample-starved, sometimes by 1 ms (t200 tier100 old peer: 14,22,30,37,43,49,54,58,62,66,69,72,75,77,79 — stops at 79 against a threshold of 80).

    The issue text will be corrected to that wording.

    4. Outcome check — matched windows, default 500 ms timeout

    Counters are window POST−PRE deltas. "First usable" is per-block first arrival, taken from the native Receive block/interval … fetch/delay lines that exist in both builds (matched by message text, since the instrumentation shifts the line number 111↔119). Those lines are one per (block, arrival) — after a switch the same block often arrives twice (block 24556, phase1-t200 tier 500: 503 ms from the delayed peer, 2 ms from the switched peer) — so per-block first arrival is the time to the first usable copy.

    t500 run (the default-timeout arm), tiers 100/150/500:

    tier base known/sec/armed Δ base first usable base write rows p1 known/sec/armed Δ p1 first usable (first / median)
    100 0 / 12 / 15 103 ms 27 0 / 0 / 15 104 / 103.0 ms
    150 0 / 12 / 15 156 ms 27 4 / 4 / 15 154 / 152.0 ms
    500 0 / 12 / 15 503 ms 27 13 / 13 / 15 503 / 3.0 ms

    Reading, stated in both directions:

    • Low tiers (100–150): the arms split the median. Phase1 does not switch early here (tier 100: 0 secondaries; tier 150: first switch only after ~3 estimator samples), so its per-block median equals the applied delay (103.0 / 152.0 ms); baseline's redirects pull the median down to 3–4 ms. Baseline is better on the median at low tiers; first-arrival values are within noise of each other (104 vs 103; 154 vs 156 ms).
    • Mid/high tiers (≥ 300): both arms' medians are 3–5 ms and the difference moves to the tail — base-t500 p90 ≈ 303 / 502 ms at tiers 300/500 vs phase1 184 / 303 ms; phase1's max is 304 / 504 ms (the 504 at tier 500 is the residual full-length fallback; the 304 at tier 300 tracks the applied delay itself).
    • Traffic trade, both directions: baseline's redirect replies show up as 27–30 write rows per window with already-known Δ = 0. Phase1 dispatches its secondaries early (typically ≤ ~60 ms), they arrive before the delayed peer's copy, and the old peer's copy then lands as the duplicate — already-known Δ 4–15 per window at tiers ≥ 150, i.e. roughly one duplicate block response per secondary, bounded at one per secondary per block. At tier 100 phase1 sends no secondaries (0/15) and records zero duplicates. All 28 windows reconcile at 15 ± 2 distinct blocks; neither arm drops blocks.

    The honest outcome statement is therefore: at low tiers baseline wins the median (redirect rescue) while phase1 holds its gate; at mid/high tiers phase1 wins the tail and pays for it with bounded duplicate arrivals, while baseline pays the tail latency and carries zero counted duplicates.

    5. Narrowed conclusions

    1. Switch coverage: unchanged from the round-3 reading.
    2. Timeout waiting: baseline's full-timeout waits are a p90-tail phenomenon, not the median; phase1 removes them except for ≤ 2 events per window, which run at full length as the designed fallback.
    3. The 100/150 ms no-switch behavior is remaining-time-gated by design; the estimator crosses the comparison threshold after ~3 samples at tier 150 in the quoted windows.
    4. Outcome: baseline keeps the better median at low tiers; phase1 improves the tail at mid/high tiers, paid for with bounded duplicate-arrival overhead (§4).
    5. The "1–2 samples to clamp" wording is qualified per §3 and will be corrected in the issue text.

    6. Artifacts

    To be linked in the PR description:

    • Instrumentation commits: acefb0717991 (instrumentation/trace-counters), 2a49bdb60808 (instrumentation/trace-phase1).
    • Harness: chain7-baseline.sh, chain7-phase1.sh, netem tier schedule, node configurations.
    • Reduction: reduce_r3.py, REDUCTION-R3.md.
    • This addendum: analysis-r4/ — commits.md, dispatch-latency.{md,tsv}, sequences.md, outcome-windows.{md,tsv}, outcome-latency.{md,tsv}, verify.md (30/30 + 28/28 checks).
    • Raw logs: the four canonical run directories.
  16. waynercheung commented on Sep 26, 2026

    @waynercheung
    Collaborator

    Thanks for extracting the sequences and correcting the interpretation of the baseline label. The dispatch-time and remaining-time explanations clarify the earlier questions. Two measurement-definition issues still need resolving before relying on the outcome conclusions:

    1. The already-known counters are not comparable. In baseline acefb071, BLOCK_ALREADY_KNOWN is incremented when block.getNum() < headNum. In phase1 2a49bdb6, it requires a matching outstanding adv request and an exact block ID already known before processing. A duplicate at the current head is therefore not counted by the baseline. Please revise the inference that duplicate overhead occurs only in Phase 1, and compare response counts/bytes for the same tracked-block cohort using a common definition. Reconstruct this from retained logs where possible; otherwise align the instrumentation for a targeted comparison.

    2. Please verify the time origin in the outcome reduction. The native fetch field is now minus the responding peer's own request timestamp from advInvRequest, not necessarily the initial request timestamp. A secondary dispatched 40 ms after the initial request and answered in 2 ms gives roughly 42 ms of total waiting, not 2 ms. Please calculate each block's earliest response against one common initial-request timestamp. Taking the minimum peer-local fetch duration would omit the secondary-dispatch delay. Also distinguish receipt from successful processing when calling the result "first usable".

    Please include per-window sample counts and per-block latencies alongside any quantiles. With roughly 15 blocks per window, descriptive p90 differences alone do not establish a repeatable tail-latency improvement.

    One correction to the convergence wording: reaching 0.8 times the injected delay is not the same as reaching the timeout clamp. A warmed estimate can also clamp in one update; for example, old=4 ms, sample=2000 ms and timeout=200 ms. Please report the observed sample-to-clamp sequences separately.

    For #6988, please link the actual analysis-r4 scripts, raw logs and corrected tables. Also document the observed reduction in secondary requests at low degradation, and the role of the unsampled channel-latency fallback, in the behavior-change section. Reserve claims about earlier first arrival or improved tails for the corrected timing results. No broader experiment matrix is needed.

    I support continuing with #6988, with the corrected outcome evidence reviewed before merge.

  17. warku123 commented on Oct 5, 2026

    @warku123
    ContributorAuthor

    Reply — measurement-definition corrections: already-known counts and per-block outcome latency

    @waynercheung Both points are valid. The two arms counted already-known at different points (baseline: block.getNum() < headNum; phase1: outstanding adv request match plus exact id contained before processing), so the earlier cross-arm comparison of that counter was not sound — I withdraw the inference that duplicate-arrival overhead existed only in Phase 1. The latency numbers had the same class of problem: they measured from the responding peer's own request timestamp instead of the common initial request. Both are corrected below, and the outcome latency is now split into first arrival and first usable.

    1. Correction 1 — already-known under a unified definition

    Unified best-effort definition from the logs: for every tracked block (each Set fetchBlockInfo line, i.e. each armed initial request), count same-id BLOCK responses arriving after the first-arriving response in the same window. A duplicate arrival is a necessary condition for already-known, so this is an upper-bound approximation of the phase1 counting point; the phase1 counter is itself documented in source as a lower-bound indicator, and the logs carry no outstanding-request markers, so the two are bounds from opposite sides rather than two measurements of the same quantity.

    Reconstruction vs counters, per window (dup = reconstructed duplicates, known Δ = tron:block_already_known POST−PRE, sec Δ = tron:block_fetch_secondary POST−PRE; n = tracked blocks, 15 or 16, equals the armed delta in all 28 windows):

    tier baseline-t200 baseline-t500 phase1-t200 phase1-t500
    100 13 (0, 13) 12 (0, 12) 0 (0, 0) 0 (0, 0)
    150 12 (0, 12) 12 (0, 12) 11 (10, 11) 4 (4, 4)
    200 12 (0, 12) 12 (0, 12) 7 (7, 7) 12 (12, 12)
    300 15 (0, 15) 12 (0, 12) 14 (14, 14) 12 (12, 12)
    500 15 (0, 15) 12 (0, 12) 15 (15, 15) 13 (13, 13)
    1000 15 (0, 15) 15 (0, 15) 15 (15, 15) 14 (14, 14)
    2000 15 (0, 15) 15 (0, 15) 13 (13, 14) 15 (15, 15)

    Three points. Under the unified definition the baseline arm also produces 12–15 duplicate arrivals per window, the same order as phase1 at the higher tiers — the duplicate traffic itself is not a Phase-1 phenomenon. The baseline counter stays 0 in all 14 windows because a duplicate arriving ~one tier-period after the first copy never satisfies getNum() < headNum; that is a property of the counting point, not a behavior difference. On the phase1 side the reconstruction equals the known delta in 13 of 14 windows; the one gap (tier 150) is a 2 ms containment-visibility race, and the sec-delta exceeds the known delta by one at t200/2000 because that block's secondary response falls outside the log truncation and the POST snapshot. For completeness: block_fetch_secondary counts secondary dispatch sends, and its equality with the reconstruction in all 14 baseline windows is an empirical one-request-one-response consequence of this testnet, not a definitional identity.

    2. Correction 2 — per-block outcome latency from the common initial request

    Each block is measured from its earliest Set fetchBlockInfo epoch (the common initial request; no block had a second Set in any window). First arrival is the first Fetch block success line for that id; first usable is the first Success process block line. Tables report first-usable latency; arrival medians sit typically 15–25 ms below usable medians across the 28 windows. n = 15 per window except two phase1-t200 windows at 16; per-block detail is in the linked tsv.

    First-usable ms (median / p90 / max):

    tier baseline-t200 baseline-t500 phase1-t200 phase1-t500
    100 64 / 112 / 122 54 / 117 / 118 121 / 128 / 217 124 / 129 / 170
    150 64 / 169 / 173 52 / 167 / 185 57 / 173 / 178 170 / 183 / 192
    200 80 / 222 / 224 68 / 223 / 230 170 / 220 / 224 56 / 227 / 238
    300 61 / 240 / 262 75 / 323 / 330 48 / 184 / 261 63 / 224 / 337
    500 60 / 234 / 263 50 / 512 / 520 68 / 119 / 255 64 / 348 / 534
    1000 71 / 246 / 267 67 / 556 / 573 64 / 90 / 274 71 / 81 / 525
    2000 61 / 239 / 268 67 / 535 / 571 56 / 87 / 262 59 / 137 / 535

    Single-run per cell, so direction only — consistent across both runs: at the 100 ms tier phase1 is slower on median (121/124 ms vs 64/54 ms baseline) because the EWMA estimate reacts slowly to the sudden onset of injected delay and comparison failures keep the old peer for the early blocks of the window — the real cost of the conservative gate, not a switch failure. From the 500 ms tier upward the corrected numbers show the intended behavior: phase1 p90 falls to 119/90/87 ms against baseline 234–246/512–556 ms in the same tiers. (Rounded to whole ms here; the linked tsv carries one decimal where present.)

    3. Sample-to-clamp sequences for the EWMA estimate

    The instrumentation logs every estimator update as updateFetchLatency peer=… oldMs=… sampleMs=… newMs=…; seeding is first-sample, blending is alpha 0.1 (new = (old*9 + sample)/10), clamped to fetchBlock.timeout (per run: 200 in t200, 500 in t500, per the config read-back). On your specific point, accepted: the steady-state estimate is not "0.8× the injected delay" — it converges toward the sample value and is bounded by the clamp. The windows show convergence-then-saturation:

    • t200 (clamp 200): steps from the first in-window sample to the clamp are 1 at tier 2000 (one-step: oldMs=6, sampleMs=2003, newMs=200 — the warm-up-seeded ~6 ms estimate jumps straight to the clamp), 3 at 1000, 5 at 500, 11 at 300; tiers 200 and below approach the injected value asymptotically and never reach the clamp in-window (tier 200 tops out at 158).
    • t500 (clamp 500): 3 steps at tier 2000 (206→385→500), 7 at 1000; tier 500 and below do not reach the clamp in-window (tier 500 tops out at 385).
    • Once clamped, newMs stays at the clamp for the rest of every window; no fallback was observed (the injected peer's samples never drop back inside a window).

    One-step clamp from the warm-up estimate is confirmed where the injected delay is at or above roughly ten times the seeded estimate and the clamp is tight; at lower tiers the estimate glides toward the injected delay over ~10 samples — the stickiness behind the tier-100 median above.

    4. Artifacts

    The reduction script analyze_r5.py and its outputs (per-block detail tsv, already-known table, sequences) will be linked in #6988; the first-arrival and tail conclusions in the earlier replies are superseded by the corrected per-block numbers above. I am not requesting a larger experiment matrix — the current 28 windows answer both points within the stated limitations.

  18. waynercheung commented on Oct 6, 2026

    @waynercheung
    Collaborator

    Thanks, this addresses the measurement-definition concerns in principle. The reconstructed responses, common-origin timing, and actual sample-to-clamp sequences are the right corrections. No broader experiment matrix is needed.

    The remaining work can be completed in #6988:

    1. Please publish analyze_r5.py, the per-block results, and the raw logs/configurations needed to reproduce the tables, replacing the current link to the earlier summary. Label the timing origin as tracking-arm time: Set fetchBlockInfo occurs after the initial send call. Keep the reconstructed duplicate-response counts distinct from the runtime already-known counter, flag the truncated response explicitly, and limit overhead claims to response counts unless bytes were also measured.

    2. Please update the issue and the PR's behavior-change section with the observed trade-off. At the 100 ms tier, Phase 1 sends fewer secondary requests but has higher first-usable medians: 121/124 ms versus 64/54 ms. Document the role of estimator adaptation and the unsampled candidate fallback. Keep the timeout groups separate, and describe the higher-tier p90 improvements as observations from these single runs, not a general performance guarantee.

    3. Replace the remaining blanket "1-2 samples" wording with the measured per-tier clamp counts. Please also avoid the "roughly ten times the seeded estimate" rule: a one-update clamp depends on the sample, the previous estimate, and the timeout together.

    The structural benefits remain a sound reason to pursue the change; it does not need to be faster in every regime. Once the artifacts and documentation are available, we can finish the experimental review in #6988 and assess the recorded trade-off alongside the code and tests before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    • Status
      No status

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions