feat: block bodies#1057
Conversation
# Conflicts: # crates/actors/src/mempool_service/facade.rs # crates/p2p/src/chain_sync.rs # crates/p2p/src/gossip_client.rs # crates/p2p/src/gossip_data_handler.rs # crates/p2p/src/server.rs # crates/p2p/src/tests/block_pool/mod.rs # crates/p2p/src/tests/util.rs
# Conflicts: # crates/p2p/src/gossip_client.rs # crates/p2p/src/gossip_data_handler.rs # crates/types/src/gossip.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/types/src/version.rs (2)
271-288: Doc comment still references old type name.The doc comment on line 271 mentions "AcceptedResponse" but the struct has been renamed to
HandshakeResponse. Consider updating the comment for consistency.📝 Suggested fix
-/// Example serialized JSON AcceptedResponse: +/// Example serialized JSON HandshakeResponse:
389-407: Consider updating variable name for consistency.The test uses variable name
version_request(line 391) which still reflects the old type name. Consider renaming tohandshake_requestfor consistency with the type rename.📝 Suggested fix
#[test] fn should_sign_and_verify_signature() { - let mut version_request = HandshakeRequest::default(); + let mut handshake_request = HandshakeRequest::default(); let testing_config = NodeConfig::testing(); let config = Config::new(testing_config); let signer = config.irys_signer(); - signer.sign_p2p_handshake(&mut version_request).unwrap(); + signer.sign_p2p_handshake(&mut handshake_request).unwrap(); assert!( - version_request.verify_signature(), + handshake_request.verify_signature(), "Signature should be valid" ); - version_request.signature = IrysSignature::default(); + handshake_request.signature = IrysSignature::default(); assert!( - !version_request.verify_signature(), + !handshake_request.verify_signature(), "Signature should be invalid after reset" ); }
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/types/src/version.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1057
File: crates/p2p/src/gossip_client.rs:143-170
Timestamp: 2026-01-05T16:21:34.820Z
Learning: In crates/p2p/src/gossip_client.rs, methods like pull_block_body_from_v1_peer intentionally convert all errors (Err(_)) to Ok(GossipResponse::Accepted(None)) because these methods must never return errors by design. This protocol will be replaced with direct socket communication later, so the current approach of treating network failures uniformly as "data not found" is acceptable.
📚 Learning: 2026-01-05T16:21:34.820Z
Learnt from: antouhou
Repo: Irys-xyz/irys PR: 1057
File: crates/p2p/src/gossip_client.rs:143-170
Timestamp: 2026-01-05T16:21:34.820Z
Learning: In crates/p2p/src/gossip_client.rs, methods like pull_block_body_from_v1_peer intentionally convert all errors (Err(_)) to Ok(GossipResponse::Accepted(None)) because these methods must never return errors by design. This protocol will be replaced with direct socket communication later, so the current approach of treating network failures uniformly as "data not found" is acceptable.
Applied to files:
crates/types/src/version.rs
🧬 Code graph analysis (1)
crates/types/src/version.rs (1)
crates/types/src/peer_list.rs (4)
default(90-106)default(121-126)new(29-31)new(449-451)
⏰ 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). (4)
- GitHub Check: cargo doc
- GitHub Check: cargo test
- GitHub Check: cargo clippy
- GitHub Check: cargo check
🔇 Additional comments (4)
crates/types/src/version.rs (4)
15-22: LGTM!The rename from
AcceptedResponsetoHandshakeResponsein theAcceptedvariant is consistent with the broader handshake terminology refactor throughout this PR.
24-62: LGTM!The
ProtocolVersionexpansion is well-structured:
HashandArbitraryderives enable use in hash-based collections and property testing.- The
#[repr(u32)]ensures stable ABI for the enum discriminants.current()beingconst fnallows compile-time evaluation.supported_versions_u32()correctly uses the repr cast.The
From<u32>fallback behavior was already discussed in prior review comments and validated by the author.
149-206: LGTM!The
HandshakeRequestimplementation is solid:
- Signing includes
protocol_version(line 186), binding the handshake to a specific protocol version.- The
encode_for_signingcorrectly excludes the signature itself, preventing circular dependencies.- Timestamp truncation from
u128tou64is safe for practical timescales.The rename from
VersionRequestaligns with the handshake terminology used throughout the PR.
297-310: LGTM!The
HandshakeResponsedefault implementation is consistent withHandshakeRequest, both defaulting to the current protocol version (V2).
| // Read protocol_version (4 bytes) if available. If it's not available, it means that it's a | ||
| // v1 record, so we default to ProtocolVersion::V1. | ||
| let mut protocol_version = ProtocolVersion::V1; | ||
| if buf.len() >= total_consumed + 4 { |
There was a problem hiding this comment.
do we have fuzz testing for this compact impl? we might want it at some point
There was a problem hiding this comment.
Not right now, I don't think so
Describe the changes
This PR adds block bodies to consolidate transaction processing for block validation
Related Issue(s)
Checklist
Additional Context
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.