accounts/abi/bind: handle UnpackLog with zero topics

[aaronbuchwald]

This PR adds error handling for the case that UnpackLog or UnpackLogIntoMap is called with a log that has zero topics.

Also moves errors from fmt.Errorf(...) with no formatted arguments to use errors.New and declare the errors at the top of the file.

[panicalways]

Is there a case where log.Topics is zero length?

[holiman]

LGTM

[holiman]

Odd, both windows-builders failed on this:

?   	github.com/ethereum/go-ethereum/core/state/pruner	[no test files]
--- FAIL: TestDiffLayerExternalInvalidationPartialFlatten (0.00s)
    snapshot_test.go:231: stale reference returned account: 0x0 (err: <nil>)
    snapshot_test.go:234: stale reference returned storage slot:  (err: <nil>)
FAIL
FAIL	github.com/ethereum/go-ethereum/core/state/snapshot	4.284s

I wonder if we have some breakage on master.

[s1na]

I guess it doesn't hurt to have this extra check but why did you decide to add it?

[aaronbuchwald]

I guess it doesn't hurt to have this extra check but why did you decide to add it?

We had an audit done of a fork if this repo and the auditors raised this issue.

I was split on whether to explain why this wasn't an issue (which it is not in the way that it's used here) or to add the suggested check.

I decided that adding the check would make the correctness of this code much more clear at very minimal cost and since it's used as a library as well, that seemed like a good tradeoff.

[aaronbuchwald]

Is there a case where log.Topics is zero length?

Is there a case where log.Topics is zero length?

Yes there can be a log with length 0 through the LOG0 opcode.

They are not very common since Solidity smart contracts typically use an identifier of the event as the first topic and any indexed parameters as the rest of the topics in the log: https://docs.soliditylang.org/en/v0.8.19/abi-spec.html#events, but you can create them by using an anonymous event or manually create an event with zero topics by doing something lower level.

[s1na]

This PR is strictly an improvement because abigen would crash for LOG0 otherwise. But I feel uneasy about returning an error for this case because technically it should be possible to unpack the data field of a LOG0.

[aaronbuchwald]

This PR is strictly an improvement because abigen would crash for LOG0 otherwise. But I feel uneasy about returning an error for this case because technically it should be possible to unpack the data field of a LOG0.

Ah nice, I agree it would be great to add support for handling anonymous logs. Thanks for updating and reviewing!

Tbh because UnpackLog takes in an event parameter as well, I thought it was a reasonable behavior to assume that the caller guarantees that this is not an anonymous log since the most common way of using this would be in a situation where the caller has extracted the event parameter from the topic at index 0 already in order to call this function or to already have some other guarantee that it knows what event it should be using to unpack this log.

The reason I think this is a good change is to make it more obviously correct and gracefully handle this slightly unexpected case.

[s1na]

Tbh because UnpackLog takes in an event parameter as well, I thought it was a reasonable behavior to assume that the caller guarantees that this is not an anonymous log since the most common way of using this would be in a situation where the caller has extracted the event parameter from the topic at index 0 already in order to call this function or to already have some other guarantee that it knows what event it should be using to unpack this log.

I didn't know this as well and learned yesterday how anonymous events work. They do have a name and that's what the event parameter of UnpackLog is. It's not the hash of signature. So you could well use UnpackLog for an anonymous event.

I have locally added some more tests, I will polish and push by this evening. It would be also nice to have a testcase which generates code for a contract with anonymous events.

[s1na]

Abigen doesn't generate a type for anonymous events because it's not possible to filter them specifically. It is still possible to search by contract address. So there's a case to be made for allowing unpacking of anonymous events, however they seem to be used veeeery seldom. So I will revert my changes and approve the PR how you had it in the beginning.

[s1na]

LGTM