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

Add signed witness filter #4124

Merged
merged 6 commits into from
Apr 10, 2020
Merged

Conversation

sqrrm
Copy link
Member

@sqrrm sqrrm commented Apr 6, 2020

  • Add a filter to pubkeys used in AccountAgeWitness signing
  • Fix inverted arbitrator signing of initial account age witnesses from
    disputes
  • Add test to verify that signed witness filter works
  • Add test to verify that the arbitrator signing was fixed

This PR adds a filter to signed witness pubkeys, which are used as the identifier in the signing tree. If a pubkey is filtered the witnesses dependent on that key will also be invalidated.

Example:

  Arbitrator
  |        |
  w1'      w2
 /  \     /  \
w3' w4'  w5  w6'
|   |    |   |  \
w7' w8'  w9  wA' wB'

w1 and w6 are filtered, causing all dependent witnesses to be filtered as well (all the prime ' witnesses in the graph). In this example only w2, w5 and w9 are still considered signed.

Bad data fix

A fix to how already signed witnesses are retrieved (SignedWitnessService.getVerifiedWitnessDateList) also affects cases where some data in the signed tree is missing, as seen in #3931.

The extra check

        if (!isSignedAccountAgeWitness(accountAgeWitness)) {
            return new ArrayList<>();
        }

will cause a verification each time rather than allowing to start a trade without checking and later being surprised that the counterparty isn't signed.

For now this will likely cause some issues for the CAD market for people that have already signed accounts but that are missing an ancestor signed witness data item. It will avoid any surprises with users not getting signed where they expect it though.

- Add a filter to pubkeys used in AccountAgeWitness signing
- Fix inverted arbitrator signing of initial account age witnesses from
disputes
- Add test to verify that signed witness filter works
- Add test to verify that the arbitrator signing was fixed
@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 7, 2020
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

Code looks fine and I tested multiple cases on Regtest filtering out malicious signed witnesses.

@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Apr 10, 2020
@ripcurlx ripcurlx merged commit 09141eb into bisq-network:master Apr 10, 2020
@ripcurlx
Copy link
Contributor

Fixes #3768.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants