Bugfix for #361 #367
Bugfix for #361 #367
Conversation
There was a problem hiding this comment.
This fix resolves the issue only partially.
Please take a look at the existing effect code for IncomingData action, when state is RequestIsReady. It first dispatches AnswerFindNodeRequest (to query Kademlia routing table) and then WaitOutgoing so reducer changes the state. It looks like the order should be opposite -- we should first switch to expecting an answer from our Kademlia routing table, and only then ask it to provide the answer. I think this is a common pitfall when we dispatch actions are not "asyncronous", i.e. triggering other actions that change the current state.
Also, regarding the amount of states, incoming kademlia stream should be properly handled. A server (this node in this case) should expect more queries, and close the stream only after the client closes its half.
https://github.com/libp2p/specs/tree/master/kad-dht#rpc-messages
|
@0xMimir Don't you mind also adding a test on this? There's a brand new p2p framework, I can help you with that. This is very basic example of a test: https://github.com/openmina/openmina/blob/develop/p2p/tests/basic.rs Shortly I'll add one for |
|
There was a problem hiding this comment.
@0xMimir In order to run the cluster, you should poll its events by using some stream operations. Without it statemachines of the nodes inside it won't progress.
This might look tool low-level, but in this way you have the full control over the nodes, where to continue or stop the execution, and inject an action at the right time.
Please take a look at the basic.rs example, there's a code that waits for node to listen, then connects another node to it, and waits for it to be a ready peer, and so on.
For your case, I think it makes sense to wait for a specific event that signals that Kademlia bootstrap is finished. You might need to extend RustNodeEvent enum with that and add a code to "record" such event:
|
@0xMimir |
akoptelov
left a comment
akoptelov
left a comment
There was a problem hiding this comment.
Looks good, but I want to ask you about the last change. I've changed the default node configuration for p2p tests so discovery is turned off. Please enable it explicitly when needed.
|
@0xMimir you will need to rebase on top of the latest |
akoptelov
left a comment
akoptelov
left a comment
There was a problem hiding this comment.
LGTM, with a comment for a future.
| std::env::set_var("OPENMINA_DISCOVERY_FILTER_ADDR", "false"); | ||
|
|
||
| let mut cluster = ClusterBuilder::new() | ||
| .ports(11000..11200) |
There was a problem hiding this comment.
Tests from a single test file are executed in different threads, and we should make sure they use disjoint set of ports, otherwise they will fail randomly.
I need to implement a shared storage for port ranges...
For now this is fine as the test is ignored.
|
UPD. I see, there is no discovery enabled in the test yet... |
|
After rebase test started failing, but when testing if second node gets into |
I enabled it in last commit |
I don't have permission to merge pull requests, so can you do it. |
3 participants
I have code to handle action being
SendResponseand state beingIncoming(RequestIsReady)in kademlia stream effects, this allows for node to respond to incoming requests.@akoptelov, Could you take a look