Extend telemetry data#5035
Conversation
Test Results for Commit 336aff9Pull Request 5035: Results Test Case Results
Last updated: 2026-03-13 20:03:20 UTC |
There was a problem hiding this comment.
Pull request overview
Extends the node telemetry payload to a new version by adding database backend, confirmation latency, and bootstrap status fields while preserving signature validity across old/new nodes via versioned sizing and unknown_data handling.
Changes:
- Adds three new telemetry fields and versioned size constants (
size_v1,size), plus adata_size_mechanism to sign/verify the appropriate structured subset. - Populates the new fields in
node::local_telemetry()(backend mapping, average confirmation latency, bootstrap status). - Updates telemetry comparison helpers and adds compatibility tests for v1↔v2 signature validation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| nano/messages/telemetry.hpp | Adds new enums/fields and versioned size constants; introduces serialized_size() and data_size_. |
| nano/messages/telemetry.cpp | Updates telemetry payload size handling, (de)serialization of new fields, signature behavior, and adds backend mapping helper. |
| nano/node/node.cpp | Populates the new telemetry fields from node runtime state. |
| nano/test_common/telemetry.cpp | Extends test telemetry comparisons to include the new fields (with one field intentionally skipped). |
| nano/core_test/telemetry.cpp | Adds tests covering backward/forward compatibility across telemetry payload versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nano/messages/telemetry.cpp
Outdated
| if (payload_length_a >= size) | ||
| { | ||
| read (stream_a, database_backend); | ||
| read (stream_a, confirmation_latency_ms); | ||
| boost::endian::big_to_native_inplace (confirmation_latency_ms); | ||
| read (stream_a, bootstrap_status); | ||
| } | ||
| data_size_ = std::min (payload_length_a, static_cast<uint16_t> (latest_size)); |
There was a problem hiding this comment.
data_size_ is set to min(payload_length_a, latest_size), but serialization of the new v2 fields is gated by data_size_ > size_v1. For payload lengths 151–155 (between v1 and v2), this makes serialized_size() report 151–155 while serialize() will still emit the full v2 fields (because the condition is true), leading to inconsistent header size vs actual bytes (and generally inconsistent re-serialization). Consider making data_size_ take on only known structured sizes (e.g., size_v1 or size) and gating v2 field serialization on data_size_ >= size (or equivalent), so partial payload lengths can be rejected without producing mismatched sizes when re-serializing.
nano/test_common/telemetry.cpp
Outdated
| ASSERT_GT (data_a.timestamp, std::chrono::system_clock::now () - std::chrono::seconds (100)); | ||
| ASSERT_EQ (data_a.active_difficulty, data_b.active_difficulty); | ||
| ASSERT_EQ (data_a.database_backend, data_b.database_backend); | ||
| ASSERT_EQ (data_a.confirmation_latency_ms, data_b.confirmation_latency_ms); |
There was a problem hiding this comment.
confirmation_latency_ms is derived from recently_cemented and can change between telemetry snapshots in the same way bootstrap_status can. Comparing it for exact equality in compare_telemetry_data_impl risks introducing test flakiness when compare_telemetry_data compares a received snapshot against a freshly generated node.local_telemetry(). Consider either skipping this field in the comparison (like bootstrap_status) or comparing it with a tolerance / only when both snapshots are known to be taken at the same time.
| ASSERT_EQ (data_a.confirmation_latency_ms, data_b.confirmation_latency_ms); | |
| // confirmation_latency_ms is not compared because it can change between telemetry snapshots |
nano/core_test/telemetry.cpp
Outdated
| // Old node (150-byte payload, size_v1) -> New node: signature must validate, new fields default to 0 | ||
| TEST (telemetry, backward_compat_v1_to_new) | ||
| { | ||
| nano::keypair node_id; | ||
| nano::messages::telemetry_data data; | ||
| data.node_id = node_id.pub; | ||
| data.major_version = 20; |
There was a problem hiding this comment.
The PR description calls out “partial payload protection” for 151–155 byte payloads, but the added compatibility tests cover only exactly v1 (150) and full v2 (156). Consider adding a unit test that deserializes a 151–155 byte payload and asserts the new fields are not consumed and (importantly) signature validation fails / the message is rejected as intended.
8012d56 to
d8c51fb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| auto maker_l = json.get<std::string> ("maker"); | ||
| maker = telemetry_maker_from_string (maker_l); |
|
|
||
| uint16_t telemetry_data::serialized_size () const | ||
| { | ||
| return size_for_version (version) + static_cast<uint16_t> (unknown_data.size ()); |
d8c51fb to
b475ad4
Compare
…values in serialized_json Replace raw uint8_t with enum types for maker, database_backend, and bootstrap_status fields. Add to_string functions for JSON serialization.
b475ad4 to
336aff9
Compare
Extends the telemetry message with three new fields: database_backend (uint8), confirmation_latency_ms (uint32), and bootstrap_status (uint8), adding 6 bytes to the payload (150 → 156 bytes)
Introduces versioned size constants (size_v1 = 150, size = 156)
Forward compatibility: An old node receives a 156-byte payload but only knows about 150 bytes of structured fields. The extra 6 bytes are read into unknown_data. Signature validation succeeds because serialize_without_signature appends unknown_data to the verification bytes, matching what the new node signed. No code changes are needed on old nodes
Backward compatibility: A new node receives a 150-byte payload. The deserialize check payload_length_a >= size (156) is false, so the three new fields remain at their zero-initialized defaults. data_size_ is set to 150 (the payload length), so serialize_without_signature skips the new fields, producing the same bytes the old node signed. Signature validation succeeds.
Partial payload protection: If a payload length falls between 151–155 bytes (more than v1 but less than the full v2 size), the new fields are not read