Incorrect field mappings in GRPC GetStatsInfo and GetNodeInfo

[xxo1shine]

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 to tcpOutTraffic instead of udpInTraffic. As a result, tcpOutTraffic is overwritten with the UDP inbound value, while udpInTraffic is never populated.

• NodeInfo.transferToProtoEntity() populates needSyncFromPeer from peerInfo.isSyncFlag() instead of peerInfo.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.GetStatsInfo and Wallet.GetNodeInfo are inaccurate and may mislead monitoring, capacity planning, and troubleshooting.
• UDP inbound traffic is not reported through the affected GRPC field, while tcpOutTraffic reports the UDP inbound value instead of the actual TCP outbound traffic.
• For connected peers, the GRPC needSyncFromPeer field 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 PeerConnection state 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:

• tcpOutTraffic will change from the previously reported UDP inbound value to the actual TCP outbound traffic.
• udpInTraffic will change from being unset/default to reporting the actual UDP inbound traffic.
• needSyncFromPeer will 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.

[jakamobiii]

@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]

Two safeguards could help prevent similar mapping errors:

1. Use distinct sentinel values in mapping tests

Avoid using the same default values such as 0 or false for related fields. For example, assign 11, 22, 33, and 44 to the four traffic directions, and use syncFlag=false with needSyncFromPeer=true. Assert every mapped field individually so that overwrites, swaps, and omissions cause the test to fail.

2. 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 mismatched getYyy() or isYyy().

These checks would catch both issues here: the duplicate setTcpOutTraffic() call and setNeedSyncFromPeer(isSyncFlag()).

[xxo1shine]

@jakamobiii Yes, this would be a better test case.

For example, if the test data is:

syncFlag = false
needSyncFromPeer = false

the incorrect implementation:

setNeedSyncFromPeer(peerInfo.isSyncFlag())

would still produce the expected result because both values happen to be the same.

With:

syncFlag = false
needSyncFromPeer = true

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]

@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]

@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]

@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 independently

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]

@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 syncFlag and needSyncFromPeer, 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]

@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.