discards packets which are not (fully) populated

[behzadnouri]

Problem

Not discarding packets which are not (fully) populated is bug prone or might waste resources.

Summary of Changes

Discard packets which are not (fully) populated.

[steviez]

Idea makes sense, what do you think about moving line inside Packet::populate_packet so that we get this behavior across the board, not just from this one PacketBatch constructor ?

[behzadnouri]

I would rather not do that.
If there is a failure, Packet::populate_packet is returning the error to the caller. The caller may choose to either:

1. discard the packet (like in this commit).
2. or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.

The problem with the current code is that the caller is effectively ignoring the error.

[steviez]

or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.

Sure, but if the caller is making a conscious decision to "recycle" the packet, it doesn't seem unreasonable to call .set_discard(false) to clean up this "dirtied" packet

[steviez]

I'm fine with this as-is; I'm probably lacking some context for the situation you have in mind.

On a related note, I'm currently working on a change to clean this up:

agave/sdk/packet/src/lib.rs

Lines 210 to 213 in 966c3c8

	#[allow(clippy::uninit_assumed_init)]
	impl Default for Packet {
	fn default() -> Self {
	let buffer = std::mem::MaybeUninit::<[u8; PACKET_DATA_SIZE]>::uninit();

As part of that work, I'll likely be reviewing the various places packets are created & populated, so we can revisit incase I develop a stronger opinion on this

[steviez]

or, reuse the packet and write the next payload to it. In which case it shouldn't be marked discard.

Sure, but if the caller is making a conscious decision to "recycle" the packet, it doesn't seem unreasonable to call .set_discard(false) to clean up this "dirtied" packet