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

Reproduce attack and implement fix for liveness issue with ABA/commoncoin #12

Merged
merged 7 commits into from
Aug 24, 2018

Conversation

sbellem
Copy link
Collaborator

@sbellem sbellem commented Aug 2, 2018

Addresses amiller/HoneyBadgerBFT#59:

  • The constructive attack proposed by MacBrough should be reflected in the test cases
  • the ABA implementation must be modified to reflect the new fix

NOTES

The adversarial network scheduler is currently limited to the minimal case of 4 nodes with one node being the attacker.

Modifying the network scheduler to support multiple nodes should not be too difficult, and is currently work in progress.

Logging was added for troubleshooting purposes. It is currently only meant to be used when testing, and future work will extend this work so that logging is available when the application is run in real scenarios.

Reproducing the Attack

Run:

$ pytest -v -m demo test/demo_attack_issue59.py -s

@sbellem sbellem changed the title Issue 59 commoncoin Reproduce attack and implement fix or liveness issue with ABA/commoncoin Aug 2, 2018
@sbellem sbellem requested a review from amiller August 2, 2018 17:17
@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #12 into dev will decrease coverage by 0.01778%.
The diff coverage is 98.24561%.

@@                 Coverage Diff                 @@
##                 dev         #12         +/-   ##
===================================================
- Coverage   98.50917%   98.49138%   -0.01779%     
===================================================
  Files             18          18                 
  Lines            872         928         +56     
  Branches         122         130          +8     
===================================================
+ Hits             859         914         +55     
  Misses            11          11                 
- Partials           2           3          +1

@sbellem sbellem force-pushed the issue-59-commoncoin branch from d8821c2 to b2b297a Compare August 9, 2018 12:09

from honeybadgerbft.exceptions import RedundantMessageError, AbandonedNodeError


logger = logging.getLogger(__name__)
CONF_PHASE = strtobool(environ.get('CONF_PHASE', '1'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are environment variables documented anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to disable the conf phase except for testing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove the switch. It was put there temporarily to ease the demonstration of the attack.

By putting the CONF phase logic in a function we could also simply mock that function in order to demonstrate the attack.

@sbellem
Copy link
Collaborator Author

sbellem commented Aug 21, 2018

@amiller The CONF_PHASE env var was removed and instead a separate "demo" test was written (commits 6842b7b & 09fec40).

This test should demonstrate the liveness issue, and consequently should only be invoked for demonstration purposes. Consequently invoking the test requires passing the option -m demo to pytest and also explicitly selecting the file. That is:

pytest -v -m demo test/demo_attack_issue59.py -s

So, the following two alternatives won't work:

# file not picked up by default because it does not start with "test_"
pytest -v -m demo -s
# option "-m demo" missing
pytest -v test/demo_attack_issue59.py -s

@amiller
Copy link
Contributor

amiller commented Aug 24, 2018

This looks ready to go to me.

@amiller amiller merged commit 4e64d73 into initc3:dev Aug 24, 2018
@sbellem sbellem changed the title Reproduce attack and implement fix or liveness issue with ABA/commoncoin Reproduce attack and implement fix for liveness issue with ABA/commoncoin Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants