bug fix: Bitswap now records 'BytesSent' in ledger #3876
Conversation
|
The Also, I'm unsure about the GitCop error regarding commit message length -- when I first got that message I changed the corresponding message and pushed the change. |
|
@Kubuxu wow, I definitely should have gone through the contribution guidelines before this PR. Thanks for the link! |
| t.Fatal(err) | ||
| } | ||
|
|
||
| ra := instances[0].Exchange.LedgerForPeer(instances[1].Peer) |
There was a problem hiding this comment.
I think it might also be useful to assert some actual values here
There was a problem hiding this comment.
@whyrusleeping makes sense, I saw that in one of the other tests but forget to include it in mine. I'll add that in the next push, thanks!
|
Is it common for CircleCI tests (for IPFS) to fail on one run then succeed on a subsequent re-run? It looks like it failed once, then I pushed again with only commit message changes, then it passed. I just pushed again and it failed, so maybe a rerun is suitable here? |
|
@dgrisham Yeah.... that particular test failing is a known issue. It only fails on travis CI or circle CI for some reason |
|
@dgrisham mind rebasing this on master to avoid having an extra merge commit? |
When sending data to another user, the number of bytes sent to that user (saved by the corresponding Bitswap ledger) was not updated (it was always 0). This also meant that the debt ratio was also always 0. The function that updates the `BytesSent` value in the ledger, `MessageSent()`, was already implemented, however it was not called when the peer was sent data. To fix this, a call to `MessageSent()` was made in the `taskWorker()` function, which is where both the message in question and the Bitswap engine were available to make the call. `MessageSent()` requires the peer's ID and `BitSwapMessage` as its arguments, the latter of which had to be created by making a new `BitSwapMessage`, then the block being sent was added to the new message. Note that, similar to the analagous call to `MessageReceived()`, records *all* of the bytes sent to a particular user. At some point, both of these should be updated to only record the numbers of *useful* bytes sent and received between peers. License: MIT Signed-off-by: David Grisham <[email protected]>
Tests were added to ensure that the bug fix in commit 000fbd25 was correct. The tests caught an error where a peer's ledger was not properly locked when updating it in the `MessageSent()` function. The appropriate calls to lock the ledger were made, and the tests successfully passed. License: MIT Signed-off-by: David Grisham <[email protected]>
Updated the `TestBitswapLedger*` tests and added assertions to check concrete values for ledgers (rather than just checking that two peers' ledgers match). The names for these tests were also changed from the previous commit, according to 's/BytesSent/Ledger/'. License: MIT Signed-off-by: David Grisham <[email protected]>
|
@whyrusleeping You got it, just pushed without that merge commit. |
|
Nice amount of tests 👍. Good work. |
|
@Kubuxu thanks! The tests were straightforward to write given the existing framework and other tests to go off of. The codebase in general is very well written as well, I had no experience with Go before looking at the Bitswap portion of the IPFS codebase but it was easy to pick up the common coding patterns and find what I needed to fix this bug. |
|
@dgrisham this is how I started hacking on go-ipfs (I had a bit of experience in Go, mostly C). |
This is a fix for the bug detailed in #3875. I add the call to
MessageSent()where it seemed to make the most sense, then manual testing (akin to the Steps to reproduce in the linked issue) was done and automated tests were addedbitswap_test.goto verify that the update worked. Upon running the automated tests, I received a read/write conflict for the ledger that I fixed by addingLock()/Unlock()calls for the ledger at the beginning ofMessageSent().