Repository navigation
Incorrect field mappings in GRPC GetStatsInfo and GetNodeInfo #6926
Description
Activity
@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?
Two safeguards could help prevent similar mapping errors:
- Use distinct sentinel values in mapping tests
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.- Add Error Prone static checks
Consider detecting:
- Repeated calls to the same protobuf builder setter within one conversion method.
- Direct mappings where
setXxx()is populated from a mismatchedgetYyy()orisYyy().
These checks would catch both issues here: the duplicate
setTcpOutTraffic()call andsetNeedSyncFromPeer(isSyncFlag()).@jakamobiii Yes, this would be a better test case.
For example, if the test data is:
syncFlag = false needSyncFromPeer = falsethe incorrect implementation:
setNeedSyncFromPeer(peerInfo.isSyncFlag())would still produce the expected result because both values happen to be the same.
With:
syncFlag = false needSyncFromPeer = truethe 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.
@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.
@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.
@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:
POJO ↓ protobuf conversion ↓ assert each field independentlyAs 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.
@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:
- Both findings are valid gRPC response‑mapping defects.
- The underlying traffic counters and peer synchronization state appear to be correct. The HTTP endpoints, P2P synchronization behavior, and consensus are not affected.
- No protobuf schema change is involved, although monitoring consumers may observe corrected values after upgrading. It may therefore be worth mentioning this behavior change in the release notes.
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.
@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.
@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.@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.
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 GRPCMonitor.GetStatsInfo (Protocol.MetricsInfo).The HTTP
/monitor/getstatsinfoendpoint is not affected. MetricsServlet serializes the POJO populated byNetMetricManager.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 GRPCWallet.GetNodeInfo.The HTTP
/wallet/getnodeinfoand/monitor/getnodeinfoendpoints are not affected.GetNodeInfoServletserializes the POJO directly, andNodeInfoServiceassignsneedSyncFromPeercorrectly.Root Cause
1. NetMetricManager.getNetProtoInfo()
In the UDP block, the target field was not updated after copying the TCP block:
The second assignment overwrites the first one. Consequently,
tcpOutTrafficreports the UDP inbound traffic value, whileudpInTrafficremains 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 forneedSyncFromPeer:The POJO already carries the correct value:
NodeInfoServiceassignsneedSyncFromPeerfromPeerConnection.isNeedSyncFromPeer(). The value is therefore lost only during the POJO → protobuf conversion.For an active peer,
syncFlagis normally false, whileneedSyncFromPeercan independently be true when the local node needs to synchronize from that peer. Because the protobuf conversion usesisSyncFlag()instead ofisNeedSyncFromPeer(), the GRPC response fails to expose this state.Reproduction
Defect 1 — GRPC Monitor.GetStatsInfo
grpcurl -plaintext 127.0.0.1:50051 protocol.Monitor/GetStatsInfoCompare the result with the HTTP endpoint backed by the same underlying counters:
curl -s http://127.0.0.1:8090/monitor/getstatsinfoThe 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
Here,
tcpOutTrafficactually contains the UDP inbound traffic value, whileudpInTrafficis not populated.GRPC GetStatsInfo — after fix
Defect 2 — GRPC Wallet.GetNodeInfo
grpcurl -plaintext -emit-defaults 127.0.0.1:50051 protocol.Wallet/GetNodeInfoCompare with:
curl -s http://127.0.0.1:8090/wallet/getnodeinfoFor each entry in
peerInfoList, the GRPC response currently setsneedSyncFromPeerto the same value assyncFlag, 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 fixGRPC
GetNodeInfo— after fixIn this case, the peer is actively connected and
needSyncFromPeeris true. The current GRPC conversion incorrectly reports false because it usessyncFlagas the source field.Impact
Monitor.GetStatsInfoandWallet.GetNodeInfoare inaccurate and may mislead monitoring, capacity planning, and troubleshooting.tcpOutTrafficreports the UDP inbound value instead of the actual TCP outbound traffic.needSyncFromPeerfield can incorrectly remain false, making the actual per-peer synchronization state unobservable through this API.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, orneedSyncFromPeershould be reviewed before upgrading. In particular, consumers that implicitly relied onudpInTrafficbeing 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.NetInfoandProtocol.NodeInfo.PeerInfoalready 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()
2. Fix the needSyncFromPeer mapping in NodeInfo.transferToProtoEntity()
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
syncFlagandneedSyncFromPeerand 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.