[Nakamoto] Potential incorrect NakamotoCoinbase encoding

[janniks]

Describe the bug
The code serializing the NakamotoCoinbase might be using unintended code paths.

The current next behavior for the NakamotoCoinbase encodes the vrf_proof as a length-prefixed byte vector. I believe it's intended to use a u8, 80 (always 80 length).

The code should be using consensus_serialize directly. Instead it's using write_next on a vector, which obfuscates what's happening. (This can be fine in many cases, but in this case it hides that LP serialize is used, when the VRFProof actually has a consensus_serialize.

Steps To Reproduce
Hard for me to show in the Rust codebase, but the bytes used for the proof are more than needed. (due to LP)

Code

stacks-core/stackslib/src/chainstate/stacks/transaction.rs

Lines 279 to 297 in 53c168d

	(None, Some(vrf_proof)) => {
	// nakamoto coinbase
	// encode principal as (optional principal)
	write_next(fd, &(TransactionPayloadID::NakamotoCoinbase as u8))?;
	write_next(fd, buf)?;
	write_next(fd, &Value::none())?;
	write_next(fd, &vrf_proof.to_bytes().to_vec())?;
	}
	(Some(recipient), Some(vrf_proof)) => {
	write_next(fd, &(TransactionPayloadID::NakamotoCoinbase as u8))?;
	write_next(fd, buf)?;
	write_next(
	fd,
	&Value::some(Value::Principal(recipient.clone())).expect(
	"FATAL: failed to encode recipient principal as `optional`",
	),
	)?;
	write_next(fd, &vrf_proof.to_bytes().to_vec())?;
	}

stacks-core/stacks-common/src/types/chainstate.rs

Lines 304 to 313 in 53c168d

	fn consensus_deserialize<R: Read>(fd: &mut R) -> Result<VRFProof, CodecError> {
	let mut bytes = [0u8; VRF_PROOF_ENCODED_SIZE as usize];
	fd.read_exact(&mut bytes).map_err(CodecError::ReadError)?;
	let res = VRFProof::from_slice(&bytes).ok_or(CodecError::DeserializeError(
	"Failed to parse VRF proof".to_string(),
	))?;
	Ok(res)
	}
	}

stacks-core/stacks-common/src/codec/mod.rs

Lines 186 to 202 in 53c168d

	impl<T> StacksMessageCodec for Vec<T>
	where
	T: StacksMessageCodec + Sized,
	{
	fn consensus_serialize<W: Write>(&self, fd: &mut W) -> Result<(), Error> {
	let len = self.len() as u32;
	write_next(fd, &len)?;
	for item in self {
	write_next(fd, item)?;
	}
	Ok(())
	}
	fn consensus_deserialize<R: Read>(fd: &mut R) -> Result<Vec<T>, Error> {
	read_next_at_most::<R, T>(fd, u32::MAX)
	}
	}

My recommendation here would be to try to use a top-level consensus_serialize as much as possible, reducing write_next calls / data transformations in the serialize cases.

---

Anyway, I could be wrong and this could be intended. If so, it might be worth adding a comment. But likely this could use a change, before it's consensus critical. 🍀

[jbencin]

Seems odd that we're using .to_bytes().to_vec() to serialize a VRFProof when it has consensus_deserialize() implemented. Made a small PR (#4121) fixing that, do you think that will fix your issue?

[zone117x]

The other tx types have their binary format specified in SIPs, which is helpful for several other projects that encode/decode these. Maybe we should get the new Nakamoto related txs documented as well?

[janniks]

@jbencin Nice thanks, I think that should do it yeah 👍🏻

Good call @zone117x

[saralab]

This was merged as part of #4121

[blockstack-devops]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.