[Proposal] Comprehensive type encoding and decoding tests

[shoenseiwaso]

Overview

This is a proposal to add comprehensive type encoding and decoding tests to go-ethereum, including all of the Pack* and Unpack* functions in accounts/abi and accounts/abi/bind. The tests would also test how the values are sent to/from smart contracts and therefore also check the full consensus mechanism as it relates to go-ethereum.

Background

Several recent bugs have exposed inconsistencies with how data types are encoded/decoded from the ABI (bugs fixed in Solidity 0.5.10 release, bugs fixed in Solidity 0.5.11 release, #20269), as well as how geth produces valid blocks (regressions fixed in go-ethereum 1.9.5 release). Comprehensive testing would help catch these during the development cycle.

Approach

Part 1: unify, fix and DRY

1. Refactor the disparate encoding/decoding packing/unpacking tests in pack_test.go, unpack_test.go and topics_test.go to use a unified set of test cases. It is highly likely this will expose additional issues that should be addressed.
2. Unify the encoding/decoding code across the Pack* and Unpack* functions in accounts/abi and accounts/abi/bind. The most complete and DRY of these looks to be in accounts/abi. Some of the changes will probably have to be percolated up from the private functions to, for example, rely on a unified primary key across abi.Type.Type, abi.Type.T and abi.Type.Kind. Some of the helper functions (toGoType(), etc.) may need to be further pulled apart or adjusted to handle the difference between decoding "standard" ABI-encoded bytes and how indexed events (i.e., transaction logs) are encoded.

Part 2: comprehensive end-to-end tests

1. Add dockerized solc to the Travis CI configuration so that we can compile contracts as part of Go tests.
2. Build on the extensive tests in bind_test.go and dynamically generate getter/setter smart contracts for all base types and select dynamic (composite) types, including both indexed and non-indexed events (where appropriate).
   1. If we want to be efficient, it would be a single smart contract compiled by the test and deployed to an in-memory SimulatedBackend with sufficiently high gas limit (100 million?) to handle it in one gulp.
   2. Better, though, would be to have the smart contract divided into pieces (automatically) and run through full geth PoA to test block composition.
3. After deploying the smart contract to an in-memory blockchain, it would then proceed to call each setter and getter, and subscribe to the events, comparing what was sent to the blockchain to what was received.

Implementation notes

Resolving this issue in the way described above is a fairly large amount of work, even for someone with detailed knowledge of Go, go-ethereum and Solidity. It could be ~2 months of full time engineering effort.

[karalabe]

I think this would be a very very welcome work. The abi package is one of the last standing legacy pieces of code that we inherited from the PoC phases of Ethereum. We kind of hacked it ever since, but it should really just be rewritten from 0 altogether. I would be very happy to have someone redo the whole thing (we did get it assigned out once, but it didn't work out in the end).

Re the 1.9.5 fix, that was not caused by abi decoding or anything like it, it was a regression introduced by some optimizations we've been working on behind the scenes to prepare for a larger new feature. Of course, would have been awesome to have a test that caches it. Regarding that though, possibly a fuzzer based contiguous approach is the best, but that's a different story altogether.

[shoenseiwaso]

Thank you for the feedback. Noted regarding 1.9.5, changed bugs => regressions. Thanks for the clarification on context.

[adamschmideg]

@MariusVanDerWijden is working on it

[shoenseiwaso]

@MariusVanDerWijden is working on it

Great!!! @MariusVanDerWijden, not sure how familiar you are with this part of the code, but if you need some pointers or hints, let us know if there's anything our team can do to help.

[MariusVanDerWijden]

@shoenseiwaso Thank you very much for the offer! I'm currently all set, but if you have any suggestions, please feel free to open additional issues. I already unified the test cases for packing and unpacking, and refactored the three different types of type checking into one.

[MariusVanDerWijden]

I think this issue can be closed now. Following improvements have been made to the abi package:

• Unified three types of type checking into one
• Created Encoding and Decoding tests for for all base types
• Encoding and Decoding tests for struct and array types
• Added a fuzzer to test packing/unpacking for all possible combinations of contract parameters and base types

If you have some more suggestions how to improve the abi package, please open a new issue