Repository navigation
Incorrect field mappings in GRPC GetStatsInfo and GetNodeInfo #6926
Description
Summary
Two GRPC response fields are populated from incorrect source fields due to copy-and-paste mistakes in the POJO → protobuf conversion layer:
-
NetMetricManager.getNetProtoInfo()assigns UDP inbound traffic totcpOutTrafficinstead ofudpInTraffic. As a result,tcpOutTrafficis overwritten with the UDP inbound value, while udpInTraffic is never populated. -
NodeInfo.transferToProtoEntity()populates needSyncFromPeer from peerInfo.isSyncFlag() instead ofpeerInfo.isNeedSyncFromPeer().
Both fields are read-only status/statistics data. Neither defect affects node networking, synchronization, or consensus behavior.
Affected API Surface
Both defects occur in the protobuf conversion path and are exposed only through the affected GRPC APIs.
Defect 1 — tcpOutTraffic / udpInTraffic
The defect is in NetMetricManager.getNetProtoInfo() in the framework module and is exposed through GRPC Monitor.GetStatsInfo (Protocol.MetricsInfo).
The HTTP /monitor/getstatsinfo endpoint is not affected. MetricsServlet serializes the POJO populated by NetMetricManager.setNetInfo(), which maps all four traffic directions correctly.
Defect 2 — needSyncFromPeer
The defect is in NodeInfo.transferToProtoEntity() in the common module and is exposed through GRPC Wallet.GetNodeInfo.
The HTTP /wallet/getnodeinfo and /monitor/getnodeinfo endpoints are not affected. GetNodeInfoServlet serializes the POJO directly, and NodeInfoService assigns needSyncFromPeer correctly.
Root Cause
1. NetMetricManager.getNetProtoInfo()
In the UDP block, the target field was not updated after copying the TCP block:
// tcp
RateInfo tcpOutTraffic = net.getTcpOutTraffic();
Protocol.MetricsInfo.RateInfo tcpOUTrafficInfo = tcpOutTraffic.toProtoEntity();
netInfo.setTcpOutTraffic(tcpOUTrafficInfo);
// udp
RateInfo udpInTraffic = net.getUdpInTraffic();
Protocol.MetricsInfo.RateInfo udpInTrafficInfo = udpInTraffic.toProtoEntity();
netInfo.setTcpOutTraffic(udpInTrafficInfo); // <-- should be setUdpInTraffic
The second assignment overwrites the first one. Consequently, tcpOutTraffic reports the UDP inbound traffic value, while udpInTraffic remains unset and is therefore absent from the response or represented by its default value, depending on the client/output format.
2. NodeInfo.transferToProtoEntity()
isSyncFlag() was copied into the assignment for needSyncFromPeer:
peerInfoBuilder.setSyncFlag(peerInfo.isSyncFlag());
peerInfoBuilder.setHeadBlockTimeWeBothHave(peerInfo.getHeadBlockTimeWeBothHave());
peerInfoBuilder.setNeedSyncFromPeer(peerInfo.isSyncFlag()); // <-- should be isNeedSyncFromPeer()
peerInfoBuilder.setNeedSyncFromUs(peerInfo.isNeedSyncFromUs());
The POJO already carries the correct value: NodeInfoService assigns needSyncFromPeer from PeerConnection.isNeedSyncFromPeer(). The value is therefore lost only during the POJO → protobuf conversion.
For an active peer, syncFlag is normally false, while needSyncFromPeer can independently be true when the local node needs to synchronize from that peer. Because the protobuf conversion uses isSyncFlag() instead of isNeedSyncFromPeer(), the GRPC response fails to expose this state.
Reproduction
Defect 1 — GRPC Monitor.GetStatsInfo
grpcurl -plaintext 127.0.0.1:50051 protocol.Monitor/GetStatsInfo
Compare the result with the HTTP endpoint backed by the same underlying counters:
curl -s http://127.0.0.1:8090/monitor/getstatsinfo
The HTTP endpoint already reports the correct field mapping, making the discrepancy directly observable on the same node.
Illustrative response fragment (values are indicative only):
GRPC GetStatsInfo — before fix
{
"net": {
"tcpInTraffic": { "count": 120000, "meanRate": 1000.0 },
"tcpOutTraffic": { "count": 3000, "meanRate": 25.0 },
"udpOutTraffic": { "count": 2800, "meanRate": 23.0 }
}
}
Here, tcpOutTraffic actually contains the UDP inbound traffic value, while udpInTraffic is not populated.
GRPC GetStatsInfo — after fix
{
"net": {
"tcpInTraffic": { "count": 120000, "meanRate": 1000.0 },
"tcpOutTraffic": { "count": 98000, "meanRate": 820.0 },
"udpInTraffic": { "count": 3000, "meanRate": 25.0 },
"udpOutTraffic": { "count": 2800, "meanRate": 23.0 }
}
}
Defect 2 — GRPC Wallet.GetNodeInfo
grpcurl -plaintext -emit-defaults 127.0.0.1:50051 protocol.Wallet/GetNodeInfo
Compare with:
curl -s http://127.0.0.1:8090/wallet/getnodeinfo
For each entry in peerInfoList, the GRPC response currently sets needSyncFromPeer to the same value as syncFlag, while the HTTP response reports the two fields independently.
For example, consider an active peer that the local node currently needs to synchronize from:
GRPC GetNodeInfo — before fix
{
"syncFlag": false,
"needSyncFromPeer": false,
"needSyncFromUs": false
}
GRPC GetNodeInfo — after fix
{
"syncFlag": false,
"needSyncFromPeer": true,
"needSyncFromUs": false
}
In this case, the peer is actively connected and needSyncFromPeer is true. The current GRPC conversion incorrectly reports false because it uses syncFlag as the source field.
Impact
- Statistics returned by GRPC
Monitor.GetStatsInfoandWallet.GetNodeInfoare inaccurate and may mislead monitoring, capacity planning, and troubleshooting. - UDP inbound traffic is not reported through the affected GRPC field, while
tcpOutTrafficreports the UDP inbound value instead of the actual TCP outbound traffic. - For connected peers, the GRPC
needSyncFromPeerfield can incorrectly remain false, making the actual per-peer synchronization state unobservable through this API. - The same node can report different values over HTTP and GRPC for the same underlying counters/state, which can be confusing for consumers using both APIs.
- These are read-only reporting defects. Traffic accounting, peer synchronization, and consensus behavior are unaffected because the underlying counters and
PeerConnectionstate are correct.
Backwards Compatibility
The fix changes values returned by two existing GRPC responses. Operators consuming these fields may observe a step change after upgrading:
tcpOutTrafficwill change from the previously reported UDP inbound value to the actual TCP outbound traffic.udpInTrafficwill change from being unset/default to reporting the actual UDP inbound traffic.needSyncFromPeerwill stop mirroring syncFlag and will report the actual per-peer synchronization state.
Dashboards or alert rules that rely on historical values of tcpOutTraffic, udpInTraffic, or needSyncFromPeer should be reviewed before upgrading. In particular, consumers that implicitly relied on udpInTraffic being unset or zero may need to update their assumptions.
Consumers using the HTTP endpoints require no changes because those values are already correct.
No protobuf schema change is involved. Protocol.MetricsInfo.NetInfo and Protocol.NodeInfo.PeerInfo already declare all affected fields, so the generated client interfaces remain unchanged and no client recompilation is required.
Suggested Fix
1. Fix the UDP inbound traffic mapping in NetMetricManager.getNetProtoInfo()
- netInfo.setTcpOutTraffic(udpInTrafficInfo);
+ netInfo.setUdpInTraffic(udpInTrafficInfo);
2. Fix the needSyncFromPeer mapping in NodeInfo.transferToProtoEntity()
- peerInfoBuilder.setNeedSyncFromPeer(peerInfo.isSyncFlag());
+ peerInfoBuilder.setNeedSyncFromPeer(peerInfo.isNeedSyncFromPeer());
3. Add unit tests for the POJO → protobuf conversion
Add unit tests that verify each source field is mapped to the corresponding protobuf field.
For the traffic conversion, build a NetInfo object with distinct values for all four traffic directions and assert each target field independently.
For the peer information conversion, use different values for syncFlag and needSyncFromPeer and verify that both values are preserved independently.
Distinct test values are important: tests that use the same value for multiple source fields would not have caught either of these mapping defects.
Activity
jakamobiii commentedon Aug 25, 2026
@xxo1shine Could we add a test case where syncFlag=false and needSyncFromPeer=true?
The root cause of this bug is that needSyncFromPeer incorrectly reuses the value of syncFlag. If both fields have the same value in the test data, the test could still pass even with the incorrect mapping.
Could we explicitly use different values for the two fields, such as syncFlag=false and needSyncFromPeer=true, to ensure that this regression test actually catches the bug?
halibobo1205 commentedon Aug 25, 2026
Two safeguards could help prevent similar mapping errors:
Avoid using the same default values such as
0orfalsefor related fields. For example, assign11,22,33, and44to the four traffic directions, and usesyncFlag=falsewithneedSyncFromPeer=true. Assert every mapped field individually so that overwrites, swaps, and omissions cause the test to fail.Consider detecting:
setXxx()is populated from a mismatchedgetYyy()orisYyy().These checks would catch both issues here: the duplicate
setTcpOutTraffic()call andsetNeedSyncFromPeer(isSyncFlag()).xxo1shine commentedon Aug 25, 2026
@jakamobiii Yes, this would be a better test case.
For example, if the test data is:
the incorrect implementation:
setNeedSyncFromPeer(peerInfo.isSyncFlag())would still produce the expected result because both values happen to be the same.
With:
the incorrect implementation would produce false, while the correct implementation should produce true. This directly catches the mapping error.
Conclusion: This is a meaningful regression test and should be added.
xxo1shine commentedon Aug 25, 2026
@halibobo1205 Thanks for the suggestion. I agree that using distinct sentinel values is important for mapping tests. We'll use different values for each source field and assert every mapped field independently, so that overwrites, swaps, and omissions are properly covered.
jakamobiii commentedon Aug 25, 2026
@xxo1shine Since the HTTP endpoint already returns the correct values while the gRPC conversion contains the incorrect mappings, should we consider adding a test to verify that the corresponding HTTP and gRPC representations remain consistent?
This could help prevent the two API paths from diverging again in the future.
xxo1shine commentedon Aug 25, 2026
@jakamobiii This could be useful, but it is probably not necessary for this fix.
The two APIs use different serialization/conversion paths, so an end-to-end HTTP + gRPC consistency test could add more test complexity than value for these simple field-mapping bugs.
Unit tests directly covering the POJO → protobuf conversion are more focused and should be sufficient:
As long as the tests use distinct values for the source fields, they should catch both of these specific defects.
Conclusion: A reasonable optional suggestion, but I would not consider it a required change.
waynercheung commentedon Aug 27, 2026
@xxo1shine
Thanks for the detailed report and for following up with #6930. I reviewed the affected conversion paths and the proposed changes.
From my review:
The two production changes in #6930 look correct and appropriately scoped. The regression tests also use distinguishable values for the related fields, including opposite combinations for
syncFlagandneedSyncFromPeer, so they should catch the original copy‑and‑paste errors.I agree that an end‑to‑end HTTP‑versus‑gRPC consistency test would be optional and should not block this fix. The proposed Error Prone checks could be considered separately: detecting repeated protobuf builder setters seems like a useful follow‑up, while generic
setXxx(getYyy())name matching may require careful evaluation to avoid false positives.Given that the issue affects read‑only observability data rather than networking or consensus behavior, the current fix appears suitable for the normal review process.
xxo1shine commentedon Aug 28, 2026
@waynercheung, I agree with your assessment. This change only corrects field mappings in the gRPC response conversion and does not affect the underlying traffic counters, peer synchronization logic, or consensus behavior.
The regression tests in #6930 use distinct values for the related fields and verify each mapping independently, so they cover both copy-and-paste errors. An end-to-end HTTP-versus-gRPC consistency test is not required for this fix.
We will note in the release notes that these gRPC monitoring fields will report their correct values after upgrading. The proposed Error Prone checks are a separate general improvement and are outside the scope of this change.
waynercheung commentedon Sep 3, 2026
@xxo1shine Thanks for the clarification. Keeping the general Error Prone work separate makes sense.
I noticed that the follow-up review in #6930 identified two additional cases. In particular, correcting
syncFlaginNodeInfoServicemay affect both HTTP and gRPC responses, so it would be helpful to clarify its intended semantics and update the PR description, regression tests, and release-note scope before the issue is closed.xxo1shine commentedon Sep 3, 2026
@waynercheung, #6926 focuses on the two originally reported POJO-to-protobuf field-mapping errors, both of which are fully addressed by #6930 through the corresponding code fixes and regression tests.
The NodeInfoService.syncFlag and LatencyDetailInfo.witness cases were identified during a broader follow-up review. They involve different conversion layers and API surfaces and are outside the original scope of #6926 and #6930, so they will not be addressed in this PR. If further investigation confirms that changes are needed, they will be evaluated and tracked separately.
Since #6930 resolves all issues originally reported in #6926, it can retain Fixes #6926.