p2p: carry received counters over GetPeerRegistry - #1717
Conversation
|
🤖 Claude Code Review Status: Complete Current Review: No issues found. Verified the fix against the code:
Nicely scoped: minimal diff, and |
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-09-11 09:11 UTC |
ctnguyen
left a comment
ctnguyen
left a comment
There was a problem hiding this comment.
Review at commit ed85310
This is a tidy fix for a genuinely dead code path. I confirmed the premise in the tree: ReportValidBlock really does write the counter (services/p2p/handle_catchup_metrics.go:399 calls RecordBlockReceived), peerTierCache.refresh really does gate tierMiner on BlocksReceived > 0 (services/asset/httpimpl/peer_auth_middleware.go:116-127), and before this PR the value had no wire field to travel on — so the exemption was unreachable over gRPC. Adding the field rather than weakening the predicate is the right call. I also checked the regenerated p2p_api.pb.go byte-for-byte rather than taking it on trust: the rawDesc message length goes \xe7\b (1127) to \xf2\t (1266), a delta of 139, which is exactly the 41 + 45 + 53 bytes the three new FieldDescriptorProto entries occupy; the tag bytes \x1e/\x1f/\x20, the \x03 int64 type and the R-prefixed json names all line up. The descriptor is therefore byte-consistent with what protoc-gen-go emits for these three fields, so a reader can take the generated file at face value instead of re-deriving it. The Interface.go doc correction is a nice touch — replacing a "complete information" promise that was never true with an explicit list of what does not cross the wire is more useful than the fix itself in the long run. Two non-blocking notes below, both about durability rather than correctness of this change.
Non-blocking issues
ChiR1 — The new reflection guard checks that a wire field exists, not that it is mapped
Problem: TestServer_PeerRegistryInfo_CarriesEveryPeerInfoField (services/p2p/server_handler_test.go:117-158) only asserts that a p2p_api.PeerRegistryInfo struct field named like each exported p2p.PeerInfo field exists. It never exercises peerInfoToP2PProto or convertFromAPIPeerInfo. The bug this PR fixes had two halves — no proto field, and no converter mapping — and the guard closes only the first. Add Foo int64 to p2p.PeerInfo, add foo = 33 to the proto, forget both converters, and the guard still passes while every gRPC consumer reads zero: the identical failure mode. The converter half is currently covered only by the hand-maintained assertion list in TestServer_PeerInfoToP2PProto_RoundTripFields, which asserts 18 of the 30 fields peerInfoToP2PProto sets, omitting twelve (BytesReceived, LastBlockTime, LastMessageTime, the three Interaction* counters, the three LastInteraction* timestamps, ReputationScore, MaliciousCount, LastCatchupErrorTime) and, despite its name, never runs the client-side converter at all. Separately, wireFields is built from all struct fields including the unexported state, unknownFields and sizeCache, so a future p2p.PeerInfo.State would satisfy the guard against the protobuf bookkeeping field rather than against a real wire field.
Why it matters: The PR body presents this test as turning "the class of bug behind this issue into a test failure". As written it catches the schema half only, so the regression it is meant to prevent can still land silently. I verified both converters do map all 30 wire fields today (services/p2p/Server.go:3115-3146, services/p2p/Client.go:851-882), so this is about the guard's future value, not a live defect.
Fix: Skip unexported wire fields, and make the guard value-based so it covers the converters end to end:
for i := 0; i < wt.NumField(); i++ {
if !wt.Field(i).IsExported() {
continue
}
wireFields[strings.ToLower(wt.Field(i).Name)] = true
}
// ... after the existing name check, assert the value actually survives both hops.
src := &blockchain.PeerInfo{ /* every non-allowlisted field set to a distinct non-zero value */ }
got, err := convertFromAPIPeerInfo(peerInfoToP2PProto(src))
require.NoError(t, err)
gv := reflect.ValueOf(*got)
for i := 0; i < dt.NumField(); i++ {
f := dt.Field(i)
if !f.IsExported() {
continue
}
if _, skip := notOnWire[f.Name]; skip {
continue
}
require.False(t, gv.Field(i).IsZero(),
"p2p.PeerInfo.%s is zero after peerInfoToP2PProto/convertFromAPIPeerInfo; add the mapping or list it in notOnWire", f.Name)
}That also retires the hand-maintained assertion list, since the new check subsumes it.
ChiR2 — Retired field numbers 12 and 13 are still not reserved
Problem: PeerRegistryInfo jumps from last_message_time = 11 to interaction_attempts = 14 (services/p2p/p2p_api/p2p_api.proto:262-265) with no reserved declaration. git log -L on the message shows the gap is not decorative: bool url_responsive = 12 and int64 last_url_check = 13 existed and were deleted in d07f154 ("Remove URLResponsive checks, all handled with reputation score"). Nothing marks them as spent, so a later change can reuse 12 or 13 for an unrelated type and a mixed-version reader will misparse the varint.
Why it matters: The repo already treats this as a convention — services/validator/validator_api/validator_api.proto:72-73, services/blockassembly/blockassembly_api/blockassembly_api.proto:158-159 and services/blockchain/blockchain_api/blockchain_api.proto:660-661 all carry reserved plus the retired name, and this very file spells out the reasoning in prose at line 336 for an RPC name that protobuf cannot reserve. This PR is the natural place to close it: it is already doing field-number housekeeping in exactly this message, and appending at 30 is otherwise correct.
Fix: Two lines in the message body:
// Removed in "Remove URLResponsive checks": url_responsive (bool) and
// last_url_check (int64). Do not reuse - a mixed-version reader would
// misinterpret a new field at these numbers.
reserved 12, 13;
reserved "url_responsive", "last_url_check";Then make gen to refresh p2p_api.pb.go.
Recap
| ID | Description | Required | Criticality |
|---|---|---|---|
| ChiR1 | Guard checks names not mappings | 35% | |
| ChiR2 | Field numbers 12 and 13 unreserved | 25% |
2 participants
Closes bitcoin-sv/teranode#4760.
Problem
p2p_api.PeerRegistryInfocarried none of the interaction-type counters (blocks_received,subtrees_received,transactions_received,catchup_blocks), sopeerInfoToP2PProtoandconvertFromAPIPeerInfosilently dropped them even thoughblockchain.PeerInfopopulates them andp2p.PeerInfodocuments them as part of the public contract. The asset service'speerTierCache.refreshclassifiestierMineronly whenBlocksReceived > 0, and the gRPC client is the onlyClientIimplementation, so in a real microservice deployment no allowlisted miner was ever promoted and all were capped at the ordinary per-peer rate limit.All cited locations confirmed still valid on
upstream/main.Fix
blocks_received,subtrees_received,transactions_received) toPeerRegistryInfo, mirroring the names already used inblockchain_api.proto, and regeneratep2p_api.pb.go.catchup_blocksis deliberately not added: nothing in the repo writesCatchupBlocks, so a wire field would be permanently zero.peerInfoToP2PProto(Server.go) and map them back inconvertFromAPIPeerInfo(Client.go).docs/references/protobuf_docs/p2pProto.md), which was also missing the existingcatchup_*rows, and correct theGetPeerRegistrydoc inInterface.go, which promised "complete information" while several fields never cross the wire.No change to the
tierMinerpredicate: the field is the right signal, it was just never transmitted.Rolling upgrade: a new asset service against an old p2p service still reads
BlocksReceived == 0, so miners stay attierPeeruntil p2p is upgraded. Fails closed (stricter limit), same as today.Residual
Pre-existing, not introduced here:
convertFromAPIPeerInfomaps a0unix timestamp totime.Unix(0, 0), which is notIsZero(). No gRPC consumer relies onIsZero()today (sync_coordinatoruses the in-process registry client). Worth a separate follow-up.Tests
TestServer_PeerInfoToP2PProto_RoundTripFields: now asserts the three counters.TestServer_GetPeerRegistry_ReceivedCountersSurviveWire(new): registryRecordBlockReceived/RecordSubtreeReceived/RecordTransactionReceived->Server.GetPeerRegistry->proto.Marshal/Unmarshal->convertFromAPIPeerInfo, asserting the counters survive the wire message.TestServer_PeerRegistryInfo_CarriesEveryPeerInfoField(new): reflection guard that every exportedp2p.PeerInfofield has aPeerRegistryInfocounterpart unless listed in an explicit, documented not-on-wire allowlist. This turns the class of bug behind this issue into a test failure.TestSimpleClientGetPeerRegistry: gRPC client maps the three proto fields ontop2p.PeerInfo.TestPeerTierCache_Refresh_ClassifiesByBlocksReceived(new, asset):refreshpromotes onlyBlocksReceived > 0with reputation at or above the threshold totierMiner.services/asset/httpimpltests could not be executed locally (pre-existing GoBDK cgo link failure on this machine); they type-check and run in CI.🤖 Generated with Claude Code