Stream accounting #1026
Stream accounting #1026
Conversation
swarm/swap_test.go
Outdated
| //(16), and each of the nodes uploads a file of same size | ||
| //Afterwards we check that every node's balance WITH ANOTHER PEER | ||
| //has the same value but opposite sign | ||
| func TestSwapNetworkSymmetricFileUpload(t *testing.T) { |
There was a problem hiding this comment.
I dont see why we would need symmetric and asymmetric versions of this test. What you check is the same no matter how many and what files a node retrieves. Why not just use the exact same simulation test as we have in accounting.
In fact adding this test has not much additional value, unless you check the exact amounts as well.
The exact amounts should be tested however with much simpler unit tests for a particular exchange
There was a problem hiding this comment.
I dont see why we would need symmetric and asymmetric versions of this test. What you check is the same no matter how many and what files a node retrieves.
Well yes theoretically. But I was unsure this was indeed the case and wanted proof.
The exact amounts should be tested however with much simpler unit tests for a particular exchange
I only partly agree. I will never resist more and simpler unit tests - the more, the better. Nevertheless we don't have any end-to-end tests of the complete stack concerning accounting, involving actual file transfers and accounting for them. So I maintain this simulation to be very valuable.
I do think though that the test may be moved into a set of end-to-end tests - which we don't have any infrastructure and setup yet. So we wouldn't have such IMHO important functionality tests which are not strictly unit test adding up to the automated test runs (prolonging test duration).
But until we don't have such infrastructure (where I have long advocated we should move the snapshot sync tests to), I would very much like these kind of tests be kept - until proven wrong of course.
There was a problem hiding this comment.
They are not affected. These tests are at the streamer protocol level, so they just assume peers have been correctly established.
I insist we can not ditch e2e just because they are not unit tests. When we have an established way to run and maintain e2e tests, then I will certainly agree to move tests like this there. But for the time being, this test confirms to me that uploading real files results in correct balances.
There was a problem hiding this comment.
Unit tests would in any case only account one way, to really check that the balances are working correctly for the high level file API you need message exchanges and multiple nodes and thus e2e tests.
There was a problem hiding this comment.
I removed all e2e tests
There was a problem hiding this comment.
@holisticode i'm not sure either about the need for symmetric and asymmetric tests. i vote too for more unit tests over these e2e tests, very much also because of the reason that it would be really hard to debug a test with 16 nodes that didn't have the correct byte count on some node. it is very hard to reason about.
also, i think this is really a lot of code for testing, a lot of it repeated too between the two tests and could be further compacted.
thirdly, shouldn't the test actually assert the balance trail of a chunk from the originating node till the NN node which is supposed to store it? i'm not sure we are approaching these assertions from the right direction
|
@justelad there are enough unit tests. These tests here build on the This PR is adding code at |
There was a problem hiding this comment.
@holisticode there are still a few outstanding comments which weren't addressed.
My opinion about having both of the symmetric and the asymmetric tests is that:
Due to syncing both actually test for the same thing. if you upload a file to a node, that file syncs evenly across the network. Retrieve requests will spread all across the network. So the test case of uploading to random nodes does not really introduce any special case that you're testing for (if anything, it sanity tests for the fact that account just works on these specific nodes). In general it would be nice to have more information on the tests - which message types should be accounted for.
Also please carefully rebase over master because @frncmx and I have made changes to the p2p/simulations package and swarm/network/simulation package.
|
re-opened upstream as ethereum/go-ethereum#18337 |
Introduces pricing for the stream protocol.
Discussion PR, to be re-opened upstream for merging.