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

don't ban peers when loading pre-sapling (and pre-blossom) blocks #3670

Merged
merged 1 commit into from Nov 17, 2018

Conversation

@LarryRuane
Copy link
Contributor

LarryRuane commented Nov 14, 2018

Resolves #3399.

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Nov 14, 2018

While syncing the mainnet blockchain from scratch, after reaching the Overwinter activation height, many instances of this began appearing in debug.log:

2018-11-13 17:30:38 ERROR: CheckTransaction(): invalid Overwinter tx version
2018-11-13 17:30:38 ERROR: AcceptToMemoryPool: ContextualCheckTransaction failed
2018-11-13 17:30:38 Misbehaving: 45.76.173.7:8233 (90 -> 100) BAN THRESHOLD EXCEEDED

Many peers were banned, causing the peer count to become very low, such as 1 or even zero for some time. Sync progress was very slow, but didn't permanently stop and eventually reached Sapling activation with no manual intervention.

Once sync had reached Sapling activation height, the messages (and banning) stopped, and sync rate increased.

This is the same problem that we fixed in #3410, except that fix only applied to pre-Overwinter transactions.

This fix also looks ahead one network upgrade (blossom), and prevents this problem from happening when syncing with pre-blossom (but post-sapling) blocks. Unfortunately, given the way the code is currently structured, there is no easy way to fix this now permanently for all future NUs. So we will encounter this problem again after whatever follows blossom activates.

@LarryRuane LarryRuane requested review from str4d , bitcartel , daira and ebfull Nov 14, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 16, 2018

@LarryRuane Just saw this now. If you add labels to the PR and add to project board it will help ensure things don't get lost. Thanks. Maybe we can get this into 2.0.2

@bitcartel
Copy link
Contributor

bitcartel left a comment

utACK.

@Eirik0

Eirik0 approved these changes Nov 17, 2018

Copy link
Contributor

Eirik0 left a comment

utACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Nov 17, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

📌 Commit 4f53418 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

⌛️ Testing commit 4f53418 with merge ed6b100...

zkbot added a commit that referenced this pull request Nov 17, 2018

Auto merge of #3670 - LarryRuane:3399-peer-ban-overwinter, r=bitcartel
don't ban peers when loading pre-sapling (and pre-blossom) blocks

Resolves #3399.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Nov 17, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing ed6b100 to master...

@zkbot zkbot merged commit 4f53418 into zcash:master Nov 17, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Nov 17, 2018

@bitcartel bitcartel added this to the v2.0.2 milestone Nov 17, 2018

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