feat: add ovm BlockFileCodec

[lean-apple]

Closes #12184.

[emhane]

First of all I moved the file into ret-optimism-cli, alongside the ovm receipt file codec.

I left you a hint about how to approach this. You can import the Header type from alloy, as is done for the original block. However, you must copy the BlockBody type, just as I have done with Block type. Also the TransactionSigned type must be copied, AND for the TransactionSigned type, we must also copy its impl body

reth/crates/primitives/src/transaction/mod.rs

Lines 1430 to 1459 in 21d911a

	impl Decodable for TransactionSigned {
	/// This `Decodable` implementation only supports decoding rlp encoded transactions as it's used
	/// by p2p.
	///
	/// The p2p encoding format always includes an RLP header, although the type RLP header depends
	/// on whether or not the transaction is a legacy transaction.
	///
	/// If the transaction is a legacy transaction, it is just encoded as a RLP list:
	/// `rlp(tx-data)`.
	///
	/// If the transaction is a typed transaction, it is encoded as a RLP string:
	/// `rlp(tx-type || rlp(tx-data))`
	///
	/// This can be used for decoding all signed transactions in p2p `BlockBodies` responses.
	///
	/// This cannot be used for decoding EIP-4844 transactions in p2p `PooledTransactions`, since
	/// the EIP-4844 variant of [`TransactionSigned`] does not include the blob sidecar.
	///
	/// For a method suitable for decoding pooled transactions, see [`PooledTransactionsElement`].
	///
	/// CAUTION: Due to a quirk in [`Header::decode`], this method will succeed even if a typed
	/// transaction is encoded in this format, and does not start with a RLP header:
	/// `tx-type || rlp(tx-data)`.
	///
	/// This is because [`Header::decode`] does not advance the buffer, and returns a length-1
	/// string header if the first byte is less than `0xf7`.
	fn decode(buf: &mut &[u8]) -> alloy_rlp::Result<Self> {
	Self::network_decode(buf).map_err(Into::into)
	}
	}

and all other of its methods that are called via that.

Let me know if it doesn't make sense, then I can finish it off.

[lean-apple]

First of all I moved the file into ret-optimism-cli, alongside the ovm receipt file codec.

I left you a hint about how to approach this. You can import the Header type from alloy, as is done for the original block. However, you must copy the BlockBody type, just as I have done with Block type. Also the TransactionSigned type must be copied, AND for the TransactionSigned type, we must also copy its impl body

reth/crates/primitives/src/transaction/mod.rs

Lines 1430 to 1459 in 21d911a

	impl Decodable for TransactionSigned {
	/// This `Decodable` implementation only supports decoding rlp encoded transactions as it's used
	/// by p2p.
	///
	/// The p2p encoding format always includes an RLP header, although the type RLP header depends
	/// on whether or not the transaction is a legacy transaction.
	///
	/// If the transaction is a legacy transaction, it is just encoded as a RLP list:
	/// `rlp(tx-data)`.
	///
	/// If the transaction is a typed transaction, it is encoded as a RLP string:
	/// `rlp(tx-type || rlp(tx-data))`
	///
	/// This can be used for decoding all signed transactions in p2p `BlockBodies` responses.
	///
	/// This cannot be used for decoding EIP-4844 transactions in p2p `PooledTransactions`, since
	/// the EIP-4844 variant of [`TransactionSigned`] does not include the blob sidecar.
	///
	/// For a method suitable for decoding pooled transactions, see [`PooledTransactionsElement`].
	///
	/// CAUTION: Due to a quirk in [`Header::decode`], this method will succeed even if a typed
	/// transaction is encoded in this format, and does not start with a RLP header:
	/// `tx-type || rlp(tx-data)`.
	///
	/// This is because [`Header::decode`] does not advance the buffer, and returns a length-1
	/// string header if the first byte is less than `0xf7`.
	fn decode(buf: &mut &[u8]) -> alloy_rlp::Result<Self> {
	Self::network_decode(buf).map_err(Into::into)
	}
	}

and all other of its methods that are called via that.

Let me know if it doesn't make sense, then I can finish it off.

Thanks for the hints, it makes more sense, continuing on it now.

[lean-apple]

First of all I moved the file into ret-optimism-cli, alongside the ovm receipt file codec.

I left you a hint about how to approach this. You can import the Header type from alloy, as is done for the original block. However, you must copy the BlockBody type, just as I have done with Block type. Also the TransactionSigned type must be copied, AND for the TransactionSigned type, we must also copy its impl body

reth/crates/primitives/src/transaction/mod.rs

Lines 1430 to 1459 in 21d911a

	impl Decodable for TransactionSigned {
	/// This `Decodable` implementation only supports decoding rlp encoded transactions as it's used
	/// by p2p.
	///
	/// The p2p encoding format always includes an RLP header, although the type RLP header depends
	/// on whether or not the transaction is a legacy transaction.
	///
	/// If the transaction is a legacy transaction, it is just encoded as a RLP list:
	/// `rlp(tx-data)`.
	///
	/// If the transaction is a typed transaction, it is encoded as a RLP string:
	/// `rlp(tx-type || rlp(tx-data))`
	///
	/// This can be used for decoding all signed transactions in p2p `BlockBodies` responses.
	///
	/// This cannot be used for decoding EIP-4844 transactions in p2p `PooledTransactions`, since
	/// the EIP-4844 variant of [`TransactionSigned`] does not include the blob sidecar.
	///
	/// For a method suitable for decoding pooled transactions, see [`PooledTransactionsElement`].
	///
	/// CAUTION: Due to a quirk in [`Header::decode`], this method will succeed even if a typed
	/// transaction is encoded in this format, and does not start with a RLP header:
	/// `tx-type || rlp(tx-data)`.
	///
	/// This is because [`Header::decode`] does not advance the buffer, and returns a length-1
	/// string header if the first byte is less than `0xf7`.
	fn decode(buf: &mut &[u8]) -> alloy_rlp::Result<Self> {
	Self::network_decode(buf).map_err(Into::into)
	}
	}

and all other of its methods that are called via that.

Let me know if it doesn't make sense, then I can finish it off.

Thanks for letting me the time to go through this despite it's a blocker for other tasks @emhane @mattsse @JosepBove

I have still a couple of questions and observations :

• if I let the ovm codec file into the optimism-cli and want to use then in the reth-downloaders, I got some cycle dependency so atm I let it in the reth-downloaders

• I move the logic for the feature specification that we want to remove there directly into the TransactionSigned::decode_rlp_legacy_transaction_tuple, are there more changes expected to adapt this TransactionSigned to OVM ? Do we plan to have a special Signature type for optimism ?

• Also I notice to make compatible with the optimism feature for the file client module, we have to use this kind of ugly imports for each case :

#[cfg(not(feature = "optimism"))]
use super::file_codec::BlockFileCodec;
#[cfg(not(feature = "optimism"))]
use reth_primitives::BlockBody;
#[cfg(feature = "optimism")]
use super::ovm_file_codec::OvmBlockFileCodec as BlockFileCodec;
#[cfg(feature = "optimism")]
use super::ovm_file_codec::BlockBody;

... and while fixing the test test_chunk_download_headers_from_file in the module file_client, I noticed we have many conditional imports for different BlockBody types in this whole crate. Instead of managing multiple BlockBody imports through feature flags, we could have a single BlockBody type that uses Vec<Box<dyn TransactionSignedTrait>>, where the trait handles the signature differences between standard and optimism transactions through separate implementations, what do you think @emhane ?

[emhane]

• if I let the ovm codec file into the optimism-cli and want to use then in the reth-downloaders, I got some cycle dependency so atm I let it in the reth-downloaders

this issue is part of a bigger whole, eventually we will have a separate import block command for ovm, and make the block FileClient generic over codec. check out how it was done for receipts. however, this will be addressed in succeeding issues, this issue is simply for implementing the codec, which already is a pretty big scope by itself as you noticed.

• I move the logic for the feature specification that we want to remove there directly into the TransactionSigned::decode_rlp_legacy_transaction_tuple, are there more changes expected to adapt this TransactionSigned to OVM ? Do we plan to have a special Signature type for optimism ?

this pr should add unit tests to prove the decoding works as expected. any bugs that may have come into existence from the refactor you describe, should be caught by these tests. it's ok for this specific issue to copy paste the needed code into reth-optimism-cli and here it's ok if it's a lot. as for signature - there will not be a separate op signature type. see review of #11567 for context.

[lean-apple]

• if I let the ovm codec file into the optimism-cli and want to use then in the reth-downloaders, I got some cycle dependency so atm I let it in the reth-downloaders

this issue is part of a bigger whole, eventually we will have a separate import block command for ovm, and make the block FileClient generic over codec. check out how it was done for receipts. however, this will be addressed in succeeding issues, this issue is simply for implementing the codec, which already is a pretty big scope by itself as you noticed.

• I move the logic for the feature specification that we want to remove there directly into the TransactionSigned::decode_rlp_legacy_transaction_tuple, are there more changes expected to adapt this TransactionSigned to OVM ? Do we plan to have a special Signature type for optimism ?

this pr should add unit tests to prove the decoding works as expected. any bugs that may have come into existence from the refactor you describe, should be caught by these tests. it's ok for this specific issue to copy paste the needed code into reth-optimism-cli and here it's ok if it's a lot. as for signature - there will not be a separate op signature type. see review of #11567 for context.

Ok got it, I will remove the integration of the codec - for now, and add units tests for the decoding.

[emhane]

on the right track

[emhane]

only methods called indirectly by this impl, need to be included, i.e. the methods that are called in the function body of Self::network_decode, need to stay. the other methods can be removed.

[emhane]

also certainly not needed since the codec is for decoding blocks from a file of rlp encoded blocks, and will not be used for encoding (in foreseeable future)

[emhane]

need any help to wrap this up @lean-apple ?

[emhane]

great job!

[lean-apple]

great job!

Thanks, sorry for the late commits, I was a bit absorbed by the Devcon, I've added a last test for eip 1559 type.