mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-28 15:58:07 +00:00
394ed2cbc0
Review of42a72863bbfound that several of the reasons it recorded were wrong, and one of its own changes was half-applied. A wrong rationale in a contract file is worse than none, because the next reader treats it as evidence. The histogram parity claim falsified itself. That commit's group description says histograms are listed by all three Prometheus series, but dns_resolve and overlay_dial were given only _bucket and _sum. Both gain _count, so the four new histograms now match the claim and the rpc_method_us convention. Two notes still said the histogram is asserted "by its _bucket and _count series"; both now say all three. The reason given for excluding serve_refused_total was wrong. It said every node holds the same history so getLedger()/getTxSet() succeed. PeerSetImpl::addPeers uses hasItem(peer) only as a sort SCORE and then adds peers in score order up to its limit, so peers that lack the item are asked anyway and would be refused. The real reason nothing is refused is that nothing asks: an inbound TMGetLedger originates only from the ledger.acquire and txset.acquire paths, both optional. That ties this counter to those spans, which the note now records. The reason for excluding peer_disconnect_total rested on the run window being 270 s, shorter than maxDivergedTime. The window is over 300 s once the 60 s propagation wait, node startup and up to 45 s of metric polling are counted, so that argument does not hold. The counter also sits after the socket-already- closed early return, which only de-duplicates repeat closes, and covers 13 reason values rather than the three cited. It stays excluded on the ground that a healthy cluster produces no disconnect cause and a mutual-dial race can produce one non-deterministically, which is flaky either way. ledger_jump_total was described as unable to fire. Its counter is unconditional and checkLastClosedLedger runs every round on every node, so a node that falls behind can follow a chain tip it did not build on -- and the burst phases exist to create exactly that load. Reworded as not deliberately provoked, with the decisive test named: grep the nodes' debug.log for "JUMP last closed ledger". The claim that the group had never asserted anything was false, and the true history is worth keeping. The five-argument call was CORRECT when it landed on 2026-07-24: the function then took report as its fifth parameter and recorded the result itself. phase-10 added the deadline and semaphore on 2026-08-14, updating its own caller, and never saw this one. The merge that first contained both,7c70e142e9, was CLEAN because the two edits sit in different regions, and it produced a caller that no longer matches its callee. So the group asserted for about four weeks and then broke silently at a clean merge -- a semantic conflict git cannot see and, in Python, no compile step rejects. Recorded as _gate_history_note, because the standing rule it implies is that a clean merge is not evidence that a cross-branch call site still matches. _b5_rotation_note had three defects of its own. Deleting "Two independent reasons." left "First, ... Second, ..." dangling; it counted four signals where there are three; and its second half still described the dynamic_cast as making registerRotationStateGauge return early, which contradicts the correction in its first half and would tell a reader no instrument exists. The cast and early return are inside the observe callback; the instrument is created eagerly. Two further corrections. The comment explaining the fix said four later phases were lost; CI passes --skip-loki, so three are. And the claim about phase-10's unaccounted-metric pass overstated it: it is warning-only, cannot fail CI, and also accepts an accounted_patterns regex list. Recorded a merge hazard where the resolver will see it. phase-10 replaces the group flatten with _metric_check_targets, which selects every dict group and has no equivalent of SKIPPED_METRIC_GROUPS, so resolving that merge in phase-10's favour makes validate_metrics walk sync_diagnostics while its own validator still does -- every metric polled and reported twice. The runbook contradicted the CI requirement this work introduces. It carried a "Gap: no ledger.* span carries a ledger hash" block stating the attribute is never set by any call site and that filtering on span.ledger_hash returns nothing, while makeLedgerTraceSpan sets it unconditionally on ledger.validate and ledger.store and InboundLedger sets it on ledger.acquire and its three phase children. ledger.build is the one real exception. The block, the ledger-span attribute table and the sentence listing what init() sets are all corrected. Verification: both JSON files parse; the four histograms each carry _bucket, _count and _sum; 61 asserted names with no duplicates and no overlap with the 22 excluded; span counters still 48 and 74; the crash repro records 61 checks with no TypeError; check_otel_naming.py exits 0; pre-commit passes on all three files (prettier re-padded only the runbook table rows touched here). NOT compiled -- no C++ changed.