fix(ibc-core): reject packets with no timeouts in send_packet_validate

[rnbguy]

Closes: #1198

Description

send_packet_validate should reject packets that have neither timeout height nor timestamp.

Reference to ICS04 spec.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests.
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[rnbguy]

Looks like these two tests (without any timeout) are expected to pass.

ibc-rs/ibc-testkit/tests/core/router.rs

Lines 347 to 359 in 16666c2

	// Timeout packets
	Test {
	name: "Transfer message no timeout".to_string(),
	msg: msg_transfer_no_timeout.into(),
	want_pass: true,
	state_check: None,
	},
	Test {
	name: "Transfer message no timeout nor timestamp".to_string(),
	msg: msg_transfer_no_timeout_or_timestamp.into(),
	want_pass: true,
	state_check: None,
	},

Judging by the impl TryFrom<RawMsgTransfer> for MsgTransfer, this should have failed

ibc-rs/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs

Lines 65 to 67 in 16666c2

	if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() {
	return Err(ContextError::from(PacketError::MissingTimeout))?;
	}

[ancazamfir]

maybe?

Suggested change

	let raw_packet_no_timeout_of_timestamp = dummy_raw_packet(10, 0);
	let raw_packet_no_timeout_timestamp = dummy_raw_packet(10, 0);

[rnbguy]

faf03fa

[ancazamfir]

shouldn't we do a similar check in recv, timeout and timeoutOnClose?
All the on_a vs on_b cause a lot of duplicate code, ideally this check should be done in one place for all messages that include a packet.

[rnbguy]

Very good question! Here's my argument:

• OnRecvPacket: timeout doesn't matter - as this would generate packet_ack anyway.
• OnTimeout/OnTimeoutClose: we added send_packet with timeout height or timeout timestamp (invariant).

Also, I didn't find these checks in IBC spec and in ibc-go.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.45%. Comparing base (30a295c) to head (f2c1a42).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
+ Coverage   64.44%   64.45%   +0.01%     
==========================================
  Files         229      229              
  Lines       22051    22055       +4     
==========================================
+ Hits        14210    14215       +5     
+ Misses       7841     7840       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.