Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix off by one error in frontier req server #1992

Merged
merged 1 commit into from May 16, 2019

Conversation

4 participants
@orhanhenrik
Copy link
Contributor

commented May 16, 2019

The current implementation will send 1 too many items, since count starts at 0.

@zhyatt zhyatt requested a review from SergiySW May 16, 2019

@zhyatt zhyatt added this to the V19.0 milestone May 16, 2019

@orhanhenrik

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Any idea why the test is failing? I cannot see how it would be related to my code. The test active_difficulty.recalculate_work is failing on the macos build.

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

frontier_req_server::sent_action increments the count before calling send_next. Doesn't that make the original code correct?

@orhanhenrik

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

If the wanted count is 1, and you run the code, the following happens:

  1. send_next (count=0), 0<=1, ok
  2. sent_action, increment count=1
  3. send_next (count=1), 1<=1, ok
  4. sent_action, increment count=2
  5. send_next (count=2), 2<=1, not ok

So the server will send 2 results and then exit if I have understood the code correctly.

There should probably be a test for this as well, but I'm not very good at C++ and I couldn't even get the project to build on my mac, so it's a bit hard to write and check a test..

@orhanhenrik

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Tests are passing on travis here: https://travis-ci.org/orhanhenrik/nano-node, so I'm not sure what's wrong with this test run. Maybe a race condition in the code?

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2019

I see, send_next is initially called from the visitor. This looks like it'll fix it.

I think clients currently always use max here (2^31-1), which might be why tests doesn't pick it up. We can look into fixing the test in a separate PR. The test failure is unrelated, probably one of the tests failing intermittently on CI.

@orhanhenrik

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Yeah, I noticed that max is always used. I’m writing a separate node implementation and was getting the wrong number of results, so that’s how I found this minor bug.

@SergiySW
Copy link
Collaborator

left a comment

LGTM

@cryptocode cryptocode merged commit d685b34 into nanocurrency:master May 16, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@wezrule wezrule added this to RC 3 (TBD) in V19 May 17, 2019

@orhanhenrik orhanhenrik deleted the orhanhenrik:frontier-req-off-by-one branch May 18, 2019

argakiig added a commit to argakiig/raiblocks that referenced this pull request May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.