refactor: frontend of trace_decoder

[0xaatif]

• Part of tracking: SMT in trace decoder #275

The previous code was extremely difficult to reason about, and was blocking prior attempts at the above issue.
The best thing was a rewrite, followed by SMT (backend) support (to be completed in a later PR).

For an explanation of the terminology, see this:

zk_evm/trace_decoder/src/lib.rs

Lines 68 to 78 in 76669ee

	/// The broad overview is as follows:
	///
	/// 1. Ethereum nodes emit a bunch of binary [`wire::Instruction`]s, which are
	/// parsed in [`wire`].
	/// 2. They are passed to one of two "frontends", depending on the node
	/// - [`hermez_cdk_erigon`], which contains an [`smt_trie`].
	/// - [`zero_jerigon`], which contains an [`mpt_trie`].
	/// 3. The frontend ([`hermez_cdk_erigon::Frontend`] or
	/// [`zero_jerigon::Frontend`]) is passed to the "backend", which lowers to
	/// [`evm_arithmetization::GenerationInputs`].
	const _DEVELOPER_DOCS: () = ();

Change summary

• Changes the public API of trace_decoder to be a single fn entrypoint, and a bunch of structs at the root.
  I'd prefer this PR didn't bikeshed names - we can't finalize an API until we see what the SMT backend requires.
• Rewrite the binary parsing in wire.
• Rewrite the binary frontend in zero_jerigon
• Ports the old MPT test cases, giving us a high degree of confidence in the changes to the frontend.
• Introduces (test only) frontend for TBC SMT support in hermez_cdk_erigon
• This PR also changed structs that are deserialized from RPC nodes, so @temaniarpit27 did live testing, report here

[Nashtare]

Fairly light review for now but two points:

• a lot of noise is coming from just removing the imports and calling trace_decoder::Foo::bar() instead. This adds needless overhead and makes code harder to parse IMO. (As far as I saw, 90% of zero-bin changes are just this)
• testing: the previous decoder was testing some specific test payloads, which this PR completely removes (including doctest). Current coverage is even lower than before (37.8% -> 29.8% for trace_decoder only) so may deserve more work

[Nashtare]

@temaniarpit27 As you are in charge of testing, could we have (in addition to whole, regular blocks), tests for these "edge-cases":

• an empty block
• an empty block with withdrawals
• a block containing a single transaction
• a block containing a single transaction with withdrawals

[0xaatif]

• testing: the previous decoder was testing some specific test payloads, which this PR completely removes (including doctest). Current coverage is even lower than before (37.8% -> 29.8% for trace_decoder only) so may deserve more work

@Nashtare could you help me understand your objection here?
AIUI the library contained no doctests previously, only unnecessary code snippets, where I've moved comments to the appropriate places in code, or deleted, which I think is the right thing to do. (why paste a copy of e.g the zero-bin callsite, to just get out of date??)

$ git rev-parse --short HEAD
baecefcb
$ cargo test --package trace_decoder --doc
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.11s
   Doc-tests trace_decoder

running 3 tests
test trace_decoder/src/lib.rs - (line 24) ... ignored
test trace_decoder/src/lib.rs - (line 6) ... ignored
test trace_decoder/src/lib.rs - (line 76) ... ignored

It contained 6 MPT payloads:

zk_evm/trace_decoder/src/compact/complex_test_payloads.rs

Lines 17 to 30 in baecefc

	pub(crate) const TEST_PAYLOAD_1: TestProtocolInputAndRoot = TestProtocolInputAndRoot { byte_str: "01055821033601462093b5945d1676df093446790fd31b20e7b12a2e8e5e09d068109616b0084a021e19e0c9bab240000005582103468288056310c82aa4c01a7e12a10f8111a0560e72b700555479031b86c357d0084101031a697e814758281972fcd13bc9707dbcd2f195986b05463d7b78426508445a0405582103b70e80538acdabd6137353b0f9d8d149f4dba91e8be2e7946e409bfdbe685b900841010558210389802d6ed1a28b049e9d4fe5334c5902fd9bc00c42821c82f82ee2da10be90800841010558200256274a27dd7524955417c11ecd917251cc7c4c8310f4c7e4bd3c304d3d9a79084a021e19e0c9bab2400000055820023ab0970b73895b8c9959bae685c3a19f45eb5ad89d42b52a340ec4ac204d190841010219102005582103876da518a393dbd067dc72abfa08d475ed6447fca96d92ec3f9e7eba503ca6100841010558210352688a8f926c816ca1e079067caba944f158e764817b83fc43594370ca9cf62008410105582103690b239ba3aaf993e443ae14aeffc44cf8d9931a79baed9fa141d0e4506e131008410102196573", root_str: "6a0673c691edfa4c4528323986bb43c579316f436ff6f8b4ac70854bbd95340b" };
	pub(crate) const TEST_PAYLOAD_2: TestProtocolInputAndRoot = TestProtocolInputAndRoot { byte_str: "01055821033601462093b5945d1676df093446790fd31b20e7b12a2e8e5e09d068109616b0084a021e19e0c9bab240000005582103468288056310c82aa4c01a7e12a10f8111a0560e72b700555479031b86c357d0084101031a697e814758281972fcd13bc9707dbcd2f195986b05463d7b78426508445a0405582103b70e80538acdabd6137353b0f9d8d149f4dba91e8be2e7946e409bfdbe685b900841010558210389802d6ed1a28b049e9d4fe5334c5902fd9bc00c42821c82f82ee2da10be90800841010558200256274a27dd7524955417c11ecd917251cc7c4c8310f4c7e4bd3c304d3d9a790c014a021e0c000250c782fa00055820023ab0970b73895b8c9959bae685c3a19f45eb5ad89d42b52a340ec4ac204d1908410102191020055820021eec2b84f0ba344fd4b4d2f022469febe7a772c4789acfc119eb558ab1da3d08480de0b6b3a76400000558200276da518a393dbd067dc72abfa08d475ed6447fca96d92ec3f9e7eba503ca61084101021901200558210352688a8f926c816ca1e079067caba944f158e764817b83fc43594370ca9cf62008410105582103690b239ba3aaf993e443ae14aeffc44cf8d9931a79baed9fa141d0e4506e131008410102196573", root_str: "e779761e7f0cf4bb2b5e5a2ebac65406d3a7516d46798040803488825a01c19c" };
	pub(crate) const TEST_PAYLOAD_3: TestProtocolInputAndRoot = TestProtocolInputAndRoot { byte_str: "01055821033601462093b5945d1676df093446790fd31b20e7b12a2e8e5e09d068109616b0084a021e19e0c9bab240000005582103468288056310c82aa4c01a7e12a10f8111a0560e72b700555479031b86c357d0084101031a697e814758281972fcd13bc9707dbcd2f195986b05463d7b78426508445a0405582103b70e80538acdabd6137353b0f9d8d149f4dba91e8be2e7946e409bfdbe685b900841010558210389802d6ed1a28b049e9d4fe5334c5902fd9bc00c42821c82f82ee2da10be90800841010558200256274a27dd7524955417c11ecd917251cc7c4c8310f4c7e4bd3c304d3d9a790c024a021e0a9cae36fa8e4788055820023ab0970b73895b8c9959bae685c3a19f45eb5ad89d42b52a340ec4ac204d1908410102191020055820021eec2b84f0ba344fd4b4d2f022469febe7a772c4789acfc119eb558ab1da3d08480f43fc2c04ee00000558200276da518a393dbd067dc72abfa08d475ed6447fca96d92ec3f9e7eba503ca61084101021901200558210352688a8f926c816ca1e079067caba944f158e764817b83fc43594370ca9cf62008410105582103690b239ba3aaf993e443ae14aeffc44cf8d9931a79baed9fa141d0e4506e131008410102196573", root_str: "6978d65a3f2fc887408cc28dbb796836ff991af73c21ea74d03a11f6cdeb119c" };
	pub(crate) const TEST_PAYLOAD_4: TestProtocolInputAndRoot = TestProtocolInputAndRoot { byte_str: "0103a6885b3731702da62e8e4a8f584ac46a7f6822f4e2ba50fba902f67b1588d23b005821028015657e298d35290e69628be03d91f74d613caf3afdbe09138cfa415efe2f5044deadbeef0558210218b289936a0874cccee65712c88cdaa0a305b004d3fda2942b2b2dc54f14f6110b443b9aca0004", root_str: "69a5a7a8f99161a35e8b64975d8c6af10db4eee7bd956418839d8ff763aaf00c" };
	pub(crate) const TEST_PAYLOAD_5: TestProtocolInputAndRoot = TestProtocolInputAndRoot {
	byte_str: include_str!("large_test_payloads/test_payload_5.txt"),
	root_str: "2b5a703bdec53099c42d7575f8cd6db85d6f2226a04e98e966fcaef87868869b",
	};
	pub(crate) const TEST_PAYLOAD_6: TestProtocolInputAndRoot = TestProtocolInputAndRoot {

All of which I have migrated:
https://github.com/0xPolygonZero/zk_evm/blob/76669ee601331bde8b6e516643baa854081fdeea/trace_decoder/src/test_cases/zero_jerigon.json

There is only one test case that has been removed:

zk_evm/trace_decoder/src/compact/compact_prestate_processing.rs

Line 1448 in baecefc

const SIMPLE_PAYLOAD_STR: &str = "01004110443132333400411044313233340218300042035044313233350218180158200000000000000000000000000000000000000000000000000000000000000012";

Which I don't trust because of this:

zk_evm/trace_decoder/src/wire.rs

Lines 241 to 244 in 76669ee

	// U4::new(*only).ok_or(DecodeKeyError::ExcessNibbleBits)?
	// TODO(0xaatif): I don't like this line - I'm adding it because
	// it's required by the simplest test vector
	U4::new(*only).unwrap_or_default()

The other point you mentioned is coverage - code coverage of the backend should be the same, because that's not changed,
Could you point me to the coverage drop for the frontend? Can it not be attributed to the drop in code size?

[Nashtare]

My bad for doctest, I thought these were running and not ignored.
As for coverage, this is what I got for trace_decoder:

develop:

|| trace_decoder/src/compact/compact_prestate_processing.rs: 334/488
|| trace_decoder/src/compact/compact_to_partial_trie.rs: 66/82
|| trace_decoder/src/decoding.rs: 0/322
|| trace_decoder/src/deserializers.rs: 0/23
|| trace_decoder/src/processed_block_trace.rs: 0/158
|| trace_decoder/src/trace_protocol.rs: 0/4
|| trace_decoder/src/utils.rs: 15/22

this_branch:

|| trace_decoder/src/decoding.rs: 0/327
|| trace_decoder/src/lib.rs: 8/70
|| trace_decoder/src/processed_block_trace.rs: 0/82
|| trace_decoder/src/wire.rs: 81/101
|| trace_decoder/src/zero_jerigon.rs: 137/179

[0xaatif]

@Nashtare
I think we agree that:

• WIBNI to get coverage up, and perhaps @temaniarpit27's testing will

But I hope I've shown that, since the test cases are the same the drop in %coverage is an apples to oranges comparison, so shouldn't block this review.
In fact, it's not surprising, since the previous code had many trivial indirections which would have bloated covered line count for the same logical path.

[0xaatif]

@temaniarpit27 As you are in charge of testing, could we have (in addition to whole, regular blocks), tests for these "edge-cases"

@Nashtare, I think this PR has the following main risks:

1. Breaking deserialization, because in the BlockData/OtherBlockData tree, I've removed some indirections, enum variants, and changed deserializers.
2. Breaking frontend transformations.

(2) is partially addressed by the existing unit tests, which we've already discussed.
I've asked Arpit to do integration testing to further de-risk the above.
Since prove_stdio.sh is already tested in CI, the main thing to de-risk is prove_rpc.sh, so that's where testing will focus.

I welcome additional test cases, but they shouldn't block this refactor: PR, and in turn progress on the SMT work - they belong to a future test: PR.

Do you agree?

[Nashtare]

Overall I'm ok with merging as is but:

• with additions of some TODOs left for the future (cf some comments)
• with clarifications on the different BUG mentions for other contributors, which aren't really clear when just looking at the surrounding code.

[0xaatif]

yikes @ CI, fixed in #357

[muursh]

I think I'm broadly good with this

[0xaatif]

@muursh / @Nashtare what will it take to get this merged? I'm starting to get bogged down by keeping up with develop

[muursh]

I looked at an earlier version and had no major concerns. If Robin has looked and is happy then I'd say you're free to merge.

[Nashtare]

Yeah I'm broadly ok to merge (based on previous version I reviewed) + latest review by Marko.