Lisk-sdk: Could not respond to peer request list because of error

[ManuGowda]

Expected behavior

Peer request should succeed when syncing or in general when the nodes in network with version 2.0.1, 2.0.0, 1.6.0

Actual behavior

14:12:38.819Z ERROR lisk-framework: Peer request not fulfilled event: Could not respond to peer request list because of error: Cannot read property 'isPublic' of undefined
14:12:39.488Z ERROR lisk-framework: Peer request not fulfilled event: Could not respond to peer request list because of error: Cannot read property 'isPublic' of undefined
14:12:39.671Z ERROR lisk-framework: Peer request not fulfilled event: Could not respond to peer request list because of error: Cannot read property 'isPublic' of undefined
14:12:40.184Z ERROR lisk-framework: Peer request not fulfilled event: Could not respond to peer request list because of error: Cannot read property 'isPublic' of undefined

Steps to reproduce

• Deploy 2.3.0-alpha.1 node
• Check the logs while syncing
  

  Which version(s) does this affect? (Environment, OS, etc...)

2.1.0

[mitsuaki-u]

This bug also involves framework layer. In bus:invokeAction function, there should be an extra check to see that the action exists before checking isPublic.

[michielmulders]

If an action needs to be public, you have to explicitly define it as isPublic: true. If it is not public, the isPublic parameter is not there. Might this be the bug? The action exists, but the isPublic parameter is not present on the object?

If so, you can define isPublic: false for every action by default to solve it (in chain/network/...).

[jondubois]

@michielmulders In this case, the list action does not exist on any module.
The Network module calls it by mistakes on the chain module, but the chain module exposes no such action.

I think we should support the case that the module action does not define isPublic: false explicitly.

[jondubois]

So we have 2 issues here:

1. The controller should send back a better error to tell us that the action does not exist.
2. The Network module should not be calling this action on the chain module to begin with.

[michielmulders]

We can go for the explicit notation adding isPublic: false/true for every action.
So, we should also add a check inside each module that first verifies if an action exists or not, right?

[jondubois]

@michielmulders The caller module should not check if the action exist on the target module, it's OK for one module to assume that the action exists on the other target module, but if the action doesn't exist (e.g. maybe that target module is a different version and the action was removed), then the framework should send back an error to tell us this in a clean way.

Also the problem here is bigger because through the Network module, peers can try to call custom public actions on any module, so they can make any RPC with any string. Right now, the error Peer request not fulfilled event: Could not respond to peer request mycustomrpc because of error: Cannot read property 'isPublic' of undefined will be sent to the peer. The peer won't know what that means.

A better error message would be The action ACTION_NAME on module MODULE_NAME does not exist or is not public.

[michielmulders]

Great suggestion, this should be the proper error message that gets returned. Can we coordinate offline how we will tackle this (I assume include in this sprint as it is urgent?).

[michielmulders]

Addition to the bug: App stopped with error Action name "nonexistent" must be a valid name with module name. -> I called from chain:

await this.channel.invoke('nonexistent', {
    height: 2,
});

Crashes the application which is good for internal actions.