Fix: Use VRFProof::consensus_serialize()

[jbencin]

Description

Use VRFProof::consensus_serialize() to serialize it rather than .to_bytes.to_vec()

Applicable issues

• fixes [Nakamoto] Potential incorrect NakamotoCoinbase encoding #4115

[jcnelson]

LGTM. Just make sure that the codec tests for the Nakamoto-flavored coinbase continue to work.

[kantai]

Can you add a test with an explicit test vector?
e.g.

let test_vector = "013fab..";
assert_eq!(&to_hex(&payload.serialize_to_vec(), test_vector);

[ASuciuX]

I ran the tests related to the NakamotoCoinbase tx and they are failing:

• https://github.com/jbencin/stacks-blockchain/blob/0b89af19b66ecfc879dc50a4d4b0bb28995073ef/stackslib/src/chainstate/stacks/transaction.rs#L2002
• https://github.com/jbencin/stacks-blockchain/blob/0b89af19b66ecfc879dc50a4d4b0bb28995073ef/stackslib/src/chainstate/stacks/transaction.rs#L2138

From the output, I see the created tx doesn't have this bytes sequence 0, 0, 0, 80 in both tests, while the predefined tx has them.
1.

8, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 9, 
`0, 0, 0, 80`,
146, 117, 223, 103, 166, 140, 135, 69, 192, 255, 151, 180, 130, 1, 238, 109, 180, 71, 247, 201, 59, 35, 174, 36, 205, 194, 64, 15, 82, 253, 176, 138, 26, 106, 199, 236, 113, 191, 156, 156, 118, 233, 110, 228, 103, 94, 191, 246, 6, 37, 175, 40, 113, 133, 1, 4, 123, 253, 135, 184, 16, 194, 210, 19, 155, 115, 194, 59, 214, 157, 230, 99, 96, 149, 58, 100, 44, 42, 51, 10

8, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 10, 6, 1, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 12, 102, 111, 111, 45, 99, 111, 110, 116, 114, 97, 99, 116, 
`0, 0, 0, 80`, 
146, 117, 223, 103, 166, 140, 135, 69, 192, 255, 151, 180, 130, 1, 238, 109, 180, 71, 247, 201, 59, 35, 174, 36, 205, 194, 64, 15, 82, 253, 176, 138, 26, 106, 199, 236, 113, 191, 156, 156, 118, 233, 110, 228, 103, 94, 191, 246, 6, 37, 175, 40, 113, 133, 1, 4, 123, 253, 135, 184, 16, 194, 210, 19, 155, 115, 194, 59, 214, 157, 230, 99, 96, 149, 58, 100, 44, 42, 51, 10

[jbencin]

Can you add a test with an explicit test vector?

There's already 3 tests that serialize/deserialize this struct:

chainstate::stacks::transaction::test::tx_stacks_transaction_codec
chainstate::stacks::transaction::test::tx_stacks_transaction_payload_nakamoto_coinbase 
chainstate::stacks::transaction::test::tx_stacks_transaction_payload_nakamoto_coinbase_alt_recipient

From the output, I see the created tx doesn't have this bytes sequence 0, 0, 0, 80

This number is the length prefix for a vector, and since VRFproof is fixed length (80 bytes), it isn't necessary. I have removed it from the unit tests and from TransactionPayload::consensus_deserialize() now so all tests pass

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ca75e1) 72.02% compared to head (396b34b) 72.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #4121      +/-   ##
==========================================
- Coverage   72.02%   72.01%   -0.01%     
==========================================
  Files         421      421              
  Lines      298095   298095              
==========================================
- Hits       214700   214683      -17     
- Misses      83395    83412      +17     

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