config add-dir regression (segmentation violation)

[michaelfig]

The bottom-up validation has caused a regression in config add-dir. I can verify that this bug doesn't exist on master when #398 is reverted.

This happens when the directory added contains, say, ibc0.json, ibc1.json, path1.json (note that the path configuration is alphabetically later than the chain configurations).

It appears that the bottom-up validation assumes that the chains for all added paths are already configured and running. This doesn't cause problems with two-chainz because the path is in demo.yaml which is read before the chain configurations.

To reproduce:

soil:relayer michael$ ls -l nchainz/config/
total 24
-rw-r--r--  1 michael  staff  167 Feb  2 16:51 ibc0.json
-rw-r--r--  1 michael  staff  167 Feb  2 16:51 ibc1.json
-rw-r--r--  1 michael  staff  228 Feb  2 16:51 path-0.json
soil:relayer michael$ cat nchainz/config/ibc0.json 
{
  "account-prefix": "agoric",
  "gas-adjustment": 1,
  "trusting-period": "336h",
  "chain-id": "ibc0",
  "rpc-addr": "http://localhost:26657",
  "key": "testkey"
}
soil:relayer michael$ cat nchainz/config/ibc1.json 
{
  "account-prefix": "agoric",
  "gas-adjustment": 1,
  "trusting-period": "336h",
  "chain-id": "ibc1",
  "rpc-addr": "http://localhost:26557",
  "key": "testkey"
}
soil:relayer michael$ cat nchainz/config/path-0.json 
{
  "src": {
    "chain-id": "ibc0",
    "port-id": "transfer",
    "order": "unordered"
  },
  "dst": {
    "chain-id": "ibc1",
    "port-id": "transfer",
    "order": "unordered"
  },
  "strategy": {
    "type": "naive"
  }
}
soil:relayer michael$ rly config add-dir nchainz/config/
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x4ebf5d7]

goroutine 1 [running]:
github.com/cosmos/relayer/relayer.(*Chain).GetAddress(0xc000f2c120, 0x0, 0x0, 0x0, 0x0, 0x0)
        /Users/michael/agoric/forks/relayer/relayer/chain.go:417 +0x97
github.com/cosmos/relayer/relayer.Chains.Get(0xc000f20660, 0x2, 0x2, 0xc000f320f8, 0x5, 0x1, 0x1, 0x164)
        /Users/michael/agoric/forks/relayer/relayer/chain.go:554 +0xad
github.com/cosmos/relayer/cmd.(*Config).ValidatePathEnd(0xc000f1e1c0, 0xc000f019d0, 0x4c, 0x300)
        /Users/michael/agoric/forks/relayer/cmd/config.go:474 +0x12a
github.com/cosmos/relayer/cmd.(*Config).ValidatePath(0xc000f1e1c0, 0xc000f242c0, 0x51d177d, 0x4)
        /Users/michael/agoric/forks/relayer/cmd/config.go:451 +0x47
github.com/cosmos/relayer/cmd.cfgFilesAdd(0x7ffeefbff2e3, 0xc, 0xc000ddfd50, 0x0, 0x0)
        /Users/michael/agoric/forks/relayer/cmd/config.go:244 +0x845
github.com/cosmos/relayer/cmd.configAddDirCmd.func1(0xc000e42dc0, 0xc000f20350, 0x1, 0x1, 0x0, 0x0)
        /Users/michael/agoric/forks/relayer/cmd/config.go:185 +0x45
github.com/spf13/cobra.(*Command).execute(0xc000e42dc0, 0xc000f20330, 0x1, 0x1, 0xc000e42dc0, 0xc000f20330)
        /Users/michael/go/pkg/mod/github.com/spf13/[email protected]/command.go:850 +0x47c
github.com/spf13/cobra.(*Command).ExecuteC(0x5e82d60, 0x0, 0x0, 0x0)
        /Users/michael/go/pkg/mod/github.com/spf13/[email protected]/command.go:958 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
        /Users/michael/go/pkg/mod/github.com/spf13/[email protected]/command.go:895
github.com/cosmos/relayer/cmd.Execute()
        /Users/michael/agoric/forks/relayer/cmd/root.go:109 +0x4d
main.main()
        /Users/michael/agoric/forks/relayer/main.go:21 +0x25

[colin-axner]

So to summarize:

Because the add dir command ranges over the files provided in the specified directory we cannot know if chain path comes before or after the chain config files. When we try to check the paths without the chain config files being set, we naturally panic.

Note, the code in this function is fairly hacky. It attempts to unmarshal the file into a chain and then checks to see if the chain parameters are empty. This seems like surface area for bugs. I think ideally, while still enabling UX, we should have config files be under chains/ and paths/. Adding more directories isn't ideal, especially if the directory in most cases only has one file, but if anyone uses this in production they may have many paths and having 20 files in one directory probably isn't clean either.

This is not a bug in the validation logic.

Proposed Fixes

Short term: Validate all path ends after all the files are added. After this loop ends, range over the config paths, then range over each path end in each path, and validate the path end.

Long term: Take in separate directories for chains and paths being added. Add all the chains then add all the paths and validate them as they are added. This should cleanup the code and reduce the surface area for more bugs in the future

Any additional thoughts @michaelfig?

[colin-axner]

@akhilkumarpilli do you have time to push a fix for this, this week?

[akhilkumarpilli]

Sure @colin-axner , we will try to push a fix, this week.