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

Left sig right sig match failure #3819

Merged
merged 2 commits into from
Nov 6, 2019

Conversation

jhoneycutt
Copy link

@jhoneycutt jhoneycutt commented Oct 30, 2019

Submitter Checklist:

Resolves brave/brave-browser#6545

Test Plan:

  1. Launch 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. Recover wallet with 30 BAT
  3. Set 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 trigger
  7. You should see 26 BAT goes through for monthly and 4 BAT from AC
  8. Watch terminal and verify that no "Left sig != right sig" error messages appear.
  9. Verify that all BAT was delivered to appropriate recipients.

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:48
@jhoneycutt jhoneycutt self-assigned this Oct 30, 2019
@jhoneycutt jhoneycutt force-pushed the issue-6545-left-sig-right-sig-match-failure branch 2 times, most recently from 0ea7e0f to 3708a5f Compare October 30, 2019 07:05
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. 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-6545-left-sig-right-sig-match-failure branch 2 times, most recently from ce29c00 to 6dfe361 Compare November 1, 2019 20:11
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.

ledger_state is corrupted.

STR:

  • claim grant
  • do 5 BAT tips to verified publisher one after another
  • open ledger_state
  • transactions are created for 3 contributions that I did, but data in empty
"auto_contribute": true,
  "transactions": [
    {
      "viewingId": "",
      "surveyorId": "",
      "rates": {},
      "contribution_probi": "",
      "submissionStamp": "",
      "anonizeViewingId": "",
      "registrarVK": "",
      "masterUserToken": "",
      "surveyorIds": [],
      "votes": 0,
      "ballots": []
    },
    {
      "viewingId": "",
      "surveyorId": "",
      "rates": {},
      "contribution_probi": "",
      "submissionStamp": "",
      "anonizeViewingId": "",
      "registrarVK": "",
      "masterUserToken": "",
      "surveyorIds": [],
      "votes": 0,
      "ballots": []
    },
    {
      "viewingId": "",
      "surveyorId": "",
      "rates": {},
      "contribution_probi": "",
      "submissionStamp": "",
      "anonizeViewingId": "",
      "registrarVK": "",
      "masterUserToken": "",
      "surveyorIds": [],
      "votes": 0,
      "ballots": []
    }
  ],

@NejcZdovc NejcZdovc dismissed their stale review November 4, 2019 08:48

found that transactions being deleted after restart is problem on master as well

Jon Honeycutt added 2 commits November 4, 2019 13:38
…so it can

be tested.

There are no behavioral changes.
Resolves #6545

The code in PhaseTwo::PrepareBatchCallback assumed that surveyor IDs were
unique, but they can be reused by different transactions. We were assigning
prepareBallot objects meant for one viewing ID to ballots for another viewing
ID. This caused the Anonize library to fail its signature checks.

To fix this, pass the viewing ID for the current transaction when calling
PhaseTwo::PrepareBatchCallback, and ensure that only ballots for that viewing
ID are updated with prepareBallot objects.
@jhoneycutt jhoneycutt force-pushed the issue-6545-left-sig-right-sig-match-failure branch from 6dfe361 to 31ea9b6 Compare November 4, 2019 19:48
@jhoneycutt
Copy link
Author

Failing CI steps are npm run audit_deps which are unrelated to this PR.

@jhoneycutt jhoneycutt merged commit 3986080 into master Nov 6, 2019
@jhoneycutt jhoneycutt deleted the issue-6545-left-sig-right-sig-match-failure branch November 6, 2019 17:17
brave-builds pushed a commit that referenced this pull request Nov 6, 2019
brave-builds pushed a commit that referenced this pull request Nov 6, 2019
mihaiplesa pushed a commit that referenced this pull request Nov 6, 2019
@bsclifton bsclifton removed this from the 0.73.x - Dev milestone Nov 7, 2019
@bsclifton bsclifton added this to the 0.74.x - Nightly milestone Nov 7, 2019
@bsclifton
Copy link
Member

Updated milestone to be 0.74.x 😄 (since that is what master is, when this was merged)

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.

Left sig != right sig. Signature check failure. message occurs very frequently - follow up to 6288
4 participants