Fix comparison of payload addresses, handle empty address

[turbolent]

Payloads may have an owner which is empty (global registers). Payload grouping incorrectly returned multiple groups for empty owners. This was caused by the assumption that addresses in payloads' encoded keys all have the same length.

Properly decode addresses from payloads using PayloadToAddress. This is inefficient, but fixes the incorrect grouping. We might want to only decode the owner key part (0) and avoid decoding the key key part (1).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 21.42857% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 55.79%. Comparing base (55f862b) to head (9adca6c).

Files	Patch %	Lines
...execution-state-extract/execution_state_extract.go	0.00%	7 Missing :warning:
cmd/util/ledger/util/payload_grouping.go	42.85%	2 Missing and 2 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5942      +/-   ##
==========================================
- Coverage   55.79%   55.79%   -0.01%     
==========================================
  Files        1128     1127       -1     
  Lines       88996    88959      -37     
==========================================
- Hits        49656    49633      -23     
+ Misses      34598    34582      -16     
- Partials     4742     4744       +2     

Flag	Coverage Δ
unittests	55.79% <21.42%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[janezpodhostnik]

Is the fact that the grouping returns multiple groups for empty owners a problem anywhere? The empty address group(s) don't get fed into the account migrators anyway if I'm not mistaken.

Can we accept that there will be multiple groups for the empty address, and instead sort of join those together after the grouping?

PayloadToAddress is probably going to be quite a big hit here. If its needed, it might be better to to just compute the addresses beforehand.

[turbolent]

Is the fact that the grouping returns multiple groups for empty owners a problem anywhere? The empty address group(s) don't get fed into the account migrators anyway if I'm not mistaken.

Currently / so far, all account groups generated by the grouping where processed, including the ones with a zero address / empty owner. Previously this did not cause problems, but in #5897 we started to index account groups by address, and the duplicates are a problem.

However, as you mentioned, given that the zero address / empty owner registers are not actually for a real account, and no Cadence values are stored in them (right?), we can probably just skip the groups with a zero address? I'll try to just skip them and see.

PayloadToAddress is probably going to be quite a big hit here.

Yeah, it definitely does more than we need. I'd still prefer using a correct address extraction function, the encodedKeyAddressPrefixLength approach seems brittle in general, given that encoding of payload keys is implemented elsewhere and may change at any time, silently breaking this assumption.

If its needed, it might be better to to just compute the addresses beforehand. Can you elaborate? Would be great to optimize this, just not quite sure what you mean.

[fxamacker]

@turbolent I will open a new PR to add new function for Payload to return address.

Maybe we can use that function here and other places so we don't need to parse payload key more than necessary.

[janezpodhostnik]

and no Cadence values are stored in them (right?)

yes, they are all values handled by the FVM. And not cadence values

[turbolent]

I made handling the duplicate owner case more lenient in 94be933: instead of failing, it now merges registers together, and logs a warning. This isn't strictly needed, but even works with the old sort function in the account grouping code that used encodedKeyAddressPrefixLength.

[fxamacker]

@turbolent this PR looks good to me. Do you want to updated this PR to use Payload.Address() in PR #5948?