Refactor TxDecoder

[emlautarom1]

Partially solves #7313

Changes

• Refactor TxDecoder to dispatch (de/en)coding to appropriate sub (de/en)coders.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Tested against the existing test suite with a single breaking change

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

Remarks

The existing API for the TxDecoder remains untouched and only one (nonsensical) test was removed. The new design picks from a list of decoders the appropriate one based on the transaction type, throwing when the type is not recognized.

This is a first step in a list of PRs, and its intentionally kept "small" to avoid having a single large refactoring PR.

[LukaszRozmej]

I think we went from one extreme to the other. From sharing everything in one class to extreme code duplication in multiple classes. Can we share the code that is the same for different decoders either by base class or some static methods?

[emlautarom1]

This was the only instance where the hash was evaluated eagerly. Instead of distorting the API to conform to this single use case, we force the hash evaluation immediately.

[emlautarom1]

I think we went from one extreme to the other. From sharing everything in one class to extreme code duplication in multiple classes. Can we share the code that is the same for different decoders either by base class or some static methods?

I tried using some form of template pattern where different transaction types can override the "default" behavior. In my opinion, this resulted in heavily coupled code with a lot of implicit assumptions since the "base class" methods resulted in a mix of abstract and virtual that always throw (ex. LegacyTxDecoder needs a special Encode and EncodeSignature, forcing the template class to either have a virtual method that always throws, or an abstract method with additional parameters only for this case).

I think there are two main sources of duplication:

• The existence of RlpStream and ref Rlp.ValueDecoderContext
• The requirement to compute sequence lengths before encoding.

For the second case some higher order functions could help at the expense of performance. I will create a separate PR showing this approach and we can discuss if it is worth or not.

[LukaszRozmej]

RlpStream and ValueDecoderContext code might be able to be unified after: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/ref-struct-interfaces

[emlautarom1]

During this refactor we explored an alternative design to the current RLP encoding/decoding API that is more declarative and results in less duplication:

https://github.com/NethermindEth/nethermind/blob/b9becf741a61fbeb9ce24bb72e9e9603dadbd9cb/src/Nethermind/Nethermind.Serialization.Rlp/MyTxDecoder/AccessListTxDecoder.cs

Essentially, we describe how to traverse a data structure (in this case a Transaction) to a Reader/Writer which is responsible for:

• During decoding, decodes sequences and the values it contains.
• During encoding, computes the length of the values and writes the appropriate sequences (including length prefixes)

Unfortunately I could not figure out a way to describe the traversal once and use it for both decoding and encoding, yet we are able to remove quite a bit of duplicated code. We might want to revisit this approach in the future.