[NC-1337] Support responding to GetNodeData requests. #587
Conversation
| } | ||
|
|
||
| @Test | ||
| public void shouldRespondToNodeDataRequests() throws Exception { |
There was a problem hiding this comment.
The other tests of this flavor are in EthProtocolManagerTest - we should probably consolidate them in one place. I originally put these tests on EthProtocolManager so that the test were less closely tied to implementation details (the serving logic being segregated in this internal package-private EthServer class). I have a slight preference for keeping them there, but I'll leave it up to you.
There was a problem hiding this comment.
The EthProtocolManagerTest cases are kind of more integration tests - they cover quite a large chunk of system. Good to have as they ensure it all fits together well but for testing the detail like that limits are applied and missing data is skipped I generally find it's better to focus on a smaller unit which EthServer works well for.
I've added a test in EthProtocolManagerTest for getNodeData to ensure it does all hang together properly.
I suspect if I had to change the other request types much in the future I'd likely write tests in EthServerTest for them just because they're smaller and easier to work with. EthProtocolManagerTest is ensuring they work well now though so I don't feel particularly compelled to go add them now. I'm happy to if you feel strongly about it though.
There was a problem hiding this comment.
I do tend to prefer higher-level tests for this stuff, but I am not super opinionated. So, lgtm - merge away!
PR description
Support responding to ETH63 GET_NODE_DATA requests. With this change, Geth (and any other client that implements fast sync) can use Pantheon as a fast sync source.
Fixed Issue(s)
NC-1337