?targets= must be even digits

[ldeffenb]

Summary

While attempting to test global pinning by specifying ?targets=xxxxx on a /chunks request, the bee node did not even attempt to trigger a recovery.

Steps to reproduce

Run a bee node with global-pinning-enable: true and attempt to retrieve a chunk with ?targets=xxxxx specifying an odd number of digits. The node will not attempt to do any recovery and will return chunk not found. If you specify an even number of digits, the node will attempt to initiate a recovery (see #2354) and return chunk recovery initiated. retry after sometime.

Expected behavior

I expected the recovery to occur regardless of the number of digits of hex specified since this is simply specifying the leading bits of the desired target kademlia neighborhood.

Actual behavior

The application did not attempt a recovery with an odd number of hex digits in the ?targets= value.

I added the following debug code to chunkGetHandler which showed that an odd number of digits will end up failing the hex parse.

            var pTargets pss.Targets
            for _, prefix := range prefixes {
                var target pss.Target
                target, err := hex.DecodeString(prefix)
                if err != nil {
                    s.logger.Debugf("chunk: targets %s prefix %s failed with %v", targets, prefix, err)
                    continue
                }
                pTargets = append(pTargets, target)
            }
            if len(pTargets) <= 0 {
                s.logger.Debugf("chunk: targets %s extracted <=0 pss.Targets", targets);
            } else {
                s.logger.Debugf("chunk: successfully parsed targets %s, got %d", targets, len(pTargets))
            }

which logged the following when an odd-length target was specified:

chunk: targets f99db prefix f99db failed with encoding/hex: odd length hex string

This is a copy of the code used in sctx.go's GetTargets which ends up causing the context to appear to not contain any target specification causing netstore to not initiate a recovery attempt.

https://github.com/ethersphere/bee/blob/6f3a38292e5e0fa3cd4856bf7dc9f5ba27c6c293/pkg/sctx/sctx.go#L69

https://github.com/ethersphere/bee/blob/6f3a38292e5e0fa3cd4856bf7dc9f5ba27c6c293/pkg/netstore/netstore.go#L54

I would expect an error to be returned for invalid (odd length) target specifications or the actual bits from an odd length target to actually be used.

[acud]

a valid hex value of 1 byte is always 2 characters long. i'm not sure that introducing a way to encode "half" bytes through a custom encoding is something we'd like to introduce as it is a bit of a hack and once new implementations of swarm arise in different platforms and langauges this will definitely become an issue and would surface as a hack.

a concrete suggestion would be to somehow including a full target address while allowing the exact PO of mining to be selected. or some other, non-hex encoding. not entirely sure. the UX so far has been dreadful around global pinning and this is just part of it. ideas welcome

[acud]

tagging @janos and @zelig for further feedback on this topic

[janos]

I agree that with @acud that validation propagation to the user as bad request response is the best, as this is the most correct way to indicate that the encoding of the submitted data is not correct.

[repo-ranger[bot]]

⚠️ This has been marked to be closed in 7 days.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This issue was closed because it has been stalled for 5 days with no activity.