Preview conformance fixes #333
Conversation
Signed-off-by: KtorZ <[email protected]>
Signed-off-by: KtorZ <[email protected]>
WalkthroughAlright mate, here’s the lowdown: The serialization of IPv6 addresses in the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| s.serialize_field( | ||
| "ipv6", | ||
| &format!("{}", std::net::Ipv6Addr::from(bytes)), | ||
| )?; |
There was a problem hiding this comment.
Not sure if it's the best choice long-term for sharing those snapshots but... IPv6 are currently encoded according to RFC 5952: http://tools.ietf.org/html/rfc5952 😅 ... we should probably just stick with hex-encoded bytes for the tests even if they're really cute.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru/tests/summary.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
crates/amaru-kernel/src/lib.rs (1)
Learnt from: stevana
PR: pragma-org/amaru#236
File: simulation/amaru-sim/src/simulator/generate.rs:141-145
Timestamp: 2025-06-03T06:31:57.736Z
Learning: In the amaru project, the team prefers to use as_bytes() instead of hex::decode() for converting hash and header strings to bytes in simulation/amaru-sim/src/simulator/generate.rs, even though they appear to be hex-encoded strings.
crates/amaru/tests/summary.rs (4)
Learnt from: rkuhn
PR: pragma-org/amaru#263
File: crates/amaru-consensus/src/consensus/store.rs:220-223
Timestamp: 2025-06-14T16:38:35.449Z
Learning: In `NetworkName::Preprod.into()` when converting to `&EraHistory`, the From implementation returns a static reference to a constant value, not a temporary. This makes it safe to return directly from functions expecting `&EraHistory` without storing it in a struct field.
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Learnt from: jeluard
PR: pragma-org/amaru#69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Snapshots (preprod, 1, 10.1.4)
- GitHub Check: Coverage
🔇 Additional comments (1)
crates/amaru/tests/summary.rs (1)
141-158: Nice Refactor—Just Double-Check Those Epoch BoundariesThis match expression is spot on—each network variant is crystal clear, and the
unimplemented!()stubs for Mainnet and Testnet are way better than a sneaky default. It reads smoother than a Tarantino monologue!Before we deck this out, let’s make sure those hard-fork epochs actually line up with the Nov 6, 2024 v10 jump you found:
• Epoch::from(180) (Preprod) — does that correspond to the 00:00 UTC Nov 6, 2024 boundary for Preprod’s v9→v10?
• Epoch::from(741) (Preview) — is that the same instant for Preview’s fork?If those numbers map to the Conway-era switch at 00:00 UTC Nov 6, 2024, you’re golden—otherwise, tweak them so the tests level up correctly. 🎮
| let bytes: [u8; 16] = [ | ||
| ipv6[3], ipv6[2], ipv6[1], ipv6[0], // 1st fragment | ||
| ipv6[7], ipv6[6], ipv6[5], ipv6[4], // 2nd fragment | ||
| ipv6[11], ipv6[10], ipv6[9], ipv6[8], // 3rd fragment | ||
| ipv6[15], ipv6[14], ipv6[13], ipv6[12], // 4th fragment | ||
| ]; | ||
| s.serialize_field( | ||
| "ipv6", | ||
| &format!("{}", std::net::Ipv6Addr::from(bytes)), | ||
| )?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fair dinkum, that's a substantial change to IPv6 serialization logic!
This refactor switches from raw byte serialization to a formatted string representation with some gnarly byte reordering. The logic reverses each 4-byte fragment before creating the Ipv6Addr - looks like endianness correction or network byte order handling, which is solid as a rock.
However, this change is significant enough that it could affect protocol compatibility. The byte shuffling pattern is quite specific and needs to align with whatever spec you're conforming to.
🏁 Script executed:
#!/bin/bash
# Description: Search for other IPv6 serialization patterns in the codebase to ensure consistency
# Search for other IPv6 handling patterns
rg -A 5 -B 5 "ipv6.*serialize|Ipv6Addr" --type rust
# Look for any tests that might cover this serialization
rg -A 10 -B 5 "SingleHostAddr.*ipv6|ipv6.*SingleHostAddr" --type rustLength of output: 2399
Ensure correct IPv6 byte order and add unit tests
This new serialization does a four-byte reverse dance before feeding into Ipv6Addr::from, which is neat but a touch fiddly. Let’s be sure we’re not sending some hobbiton-sized mis-endian packet:
- Confirm this chunked reverse (LE→BE per u32) actually matches the protocol spec for your stored format.
- Add focused unit tests for a few canonical addresses (e.g.
::1,fe80::1, a random full-address) to catch any mix-ups. - Consider simplifying by interpreting the 16-byte array as a
u128and usingu128::from_be_bytes(…)withIpv6Addr::from—less manual shuffling, fewer gremlins.
Let’s lock down that endianness so no one ends up racing across the outback trying to debug a backwards IPv6!
🤖 Prompt for AI Agents
In crates/amaru-kernel/src/lib.rs around lines 446 to 455, the current IPv6 byte
reordering before serialization is complex and may cause endianness errors.
Verify that the four-byte reversed order matches the protocol specification for
the stored format. To fix, add unit tests for known IPv6 addresses like ::1,
fe80::1, and a full random address to ensure correct serialization.
Additionally, simplify the code by converting the 16-byte array directly into a
u128 using u128::from_be_bytes and then create the Ipv6Addr from that,
eliminating manual byte shuffling and reducing potential errors.
Codecov ReportAttention: Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Bug Fixes
Refactor