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

Ballots assigned incorrect proof #3820

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

jhoneycutt
Copy link

@jhoneycutt jhoneycutt commented Oct 30, 2019

Submitter Checklist:

Resolves brave/brave-browser#6607

Test Plan:

  1. Enable rewards with short contribution interval on staging. Launched with --enable-logging=stderr --vmodule=rewards=6 --rewards=staging=true,reconcile-interval=10,short-retries=true to see log messages, use staging env, set short contribution interval, and short retry interval.
  2. Recovered wallet with 30 BAT
  3. Aet AC amount to Up to 20 BAT
  4. Add verified and non verified publishers to AC table
  5. Add verified recurring tips in this order (5, 10, 10, 10, 1)
  6. Wait for monthly contribution to complete
  7. Watch console window output. Verify that there are no messages containing the string "invalid surveyor proof"

Because this bug is timing related and not 100% reproducible, repeat the steps above several (5?) times.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@jhoneycutt jhoneycutt added this to the 0.73.x - Nightly milestone Oct 30, 2019
@jhoneycutt jhoneycutt requested review from NejcZdovc and a team October 30, 2019 02:53
@jhoneycutt jhoneycutt self-assigned this Oct 30, 2019
@jhoneycutt jhoneycutt force-pushed the issue-6607-ballots-assigned-incorrect-proof branch from e1de49e to 08a8f35 Compare October 30, 2019 06:55
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++

@jhoneycutt
Copy link
Author

CI shows several unrelated test failures (timeouts). However, those failures cause the browser tests to abort early, meaning that the relevant browser tests to this PR are unable to run. The network audit step also fails for unrelated reasons.

@jhoneycutt jhoneycutt force-pushed the issue-6607-ballots-assigned-incorrect-proof branch 5 times, most recently from e02e4c7 to 296491e Compare November 6, 2019 19:08
Jon Honeycutt added 2 commits November 6, 2019 15:55
Resolves #6607: Ballots assigned incorrect proof.

The code in PhaseTwo::ProofBallots assumed that surveyor IDs are unique, but
they may be reused between transactions. This could cause the proof for a
ballot with one viewing ID to be assigned to a ballot for a different viewing
ID if they shared the same surveyor ID.

To fix this, check that both the surveyor ID and viewing ID are a match before
assigning the proof.
@jhoneycutt jhoneycutt force-pushed the issue-6607-ballots-assigned-incorrect-proof branch from 296491e to 36279d3 Compare November 6, 2019 21:55
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code change looks good to me. I see that CI was aborted, so I restarted it. When CI passes we can merge it

@jhoneycutt
Copy link
Author

All tests passed (after retries), but some pipeline steps related to publishing test results failed. Another step timed out with this error message:

09:56:44  Cannot contact i-09f79fb4f5b7386d4: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@7ead8495:i-09f79fb4f5b7386d4": Remote call on i-09f79fb4f5b7386d4 failed. The channel is closing down or has closed down
10:01:51  Could not connect to i-09f79fb4f5b7386d4 to send interrupt signal to process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ballots may be assigned an incorrect proof
3 participants