sysvar-instructions: check data length before storing current instruction idx

[mjain-jump]

There are no checks for the instructions sysvar data length before trying to subtract / store data into the account. This does not cause issues in production, but causes issues with Firedancer's fuzzing efforts as this code triggers a panic if the sysvar account is not correctly initialized by the fuzzer when testing UB.

[buffalojoec]

I really don't think it's a good idea to silently do nothing in the case of a bad data length. I would say this could cause a consensus mismatch on data.len() < 2, but the original implementation would cause a panic.

Maybe it's better to just make it fallible with an InstructionError?

[mjain-jump]

I really don't think it's a good idea to silently do nothing in the case of a bad data length. I would say this could cause a consensus mismatch on data.len() < 2, but the original implementation would cause a panic.

Maybe it's better to just make it fallible with an InstructionError?

In production it's never possible for the serialized size to be < 2 based on the sysvar serialization logic. I don't think this would cause a consensus break but I'm open to returning an instruction error instead of panicking, just would prefer not to crash the client here.

[buffalojoec]

I really don't think it's a good idea to silently do nothing in the case of a bad data length. I would say this could cause a consensus mismatch on data.len() < 2, but the original implementation would cause a panic.
Maybe it's better to just make it fallible with an InstructionError?

In production it's never possible for the serialized size to be < 2 based on the sysvar serialization logic. I don't think this would cause a consensus break but I'm open to returning an instruction error instead of panicking, just would prefer not to crash the client here.

Yeah, I think the InstructionError is the better move, since on the Agave side, we can just bubble the error up through InvokeContext::push.

[mjain-jump]

I really don't think it's a good idea to silently do nothing in the case of a bad data length. I would say this could cause a consensus mismatch on data.len() < 2, but the original implementation would cause a panic.
Maybe it's better to just make it fallible with an InstructionError?

In production it's never possible for the serialized size to be < 2 based on the sysvar serialization logic. I don't think this would cause a consensus break but I'm open to returning an instruction error instead of panicking, just would prefer not to crash the client here.

Yeah, I think the InstructionError is the better move, since on the Agave side, we can just bubble the error up through InvokeContext::push.

Cool, just pushed a fix. Let me know if the error code looks alright.

[buffalojoec]

Thanks!