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 Max Reorganization Depth #295

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Add Max Reorganization Depth #295

merged 1 commit into from
Sep 18, 2018

Conversation

blondfrogs
Copy link
Contributor

@blondfrogs blondfrogs commented Sep 14, 2018

This allows clients to run the wallet and not allow reorganizations that are 55 or more blocks. This can be changed on client start by passing in the flag maxreorg=<n>

This should help stop potential chain attacks by malicious users as well as allow exchanges to augment the required amount of confirmations required.

@blondfrogs blondfrogs changed the title Add Max Reorganization Depth [WIP] Add Max Reorganization Depth Sep 14, 2018
Copy link
Collaborator

@TronBlack TronBlack left a comment

Choose a reason for hiding this comment

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

Looks good for just depth reorg. Because connecting to one or two rogue nodes first can cause a split if the rogue nodes are able to send 55 blocks before additional connections, let's add these two conditions before rejecting surprise re-org:

  1. Our node is caught up to the best blockchain.
  2. Our node is well-connected to the network, and has been well-connected for a while.
    https://gist.github.com/gavinandresen/630d4a6c24ac6144482a
    Goal: Prevent surprise selfish mining re-org (Scenario C and Scenario D)

@blondfrogs
Copy link
Contributor Author

Updated required checks for max reorg

Copy link
Contributor

@spyder46n2 spyder46n2 left a comment

Choose a reason for hiding this comment

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

Looks good.

//If this is a reorg, check that it is not too deep
int nMaxReorgDepth = gArgs.GetArg("-maxreorg", Params().MaxReorganizationDepth());
bool fGreaterThanMaxReorg = chainActive.Height() - nHeight >= nMaxReorgDepth;
if (fGreaterThanMaxReorg && g_connman && g_connman->GetNodeCount(CConnman::CONNECTIONS_ALL) > 3 && !IsInitialBlockDownload())
Copy link
Contributor

Choose a reason for hiding this comment

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

could make '3' a parameter

@blondfrogs blondfrogs changed the title [WIP] Add Max Reorganization Depth Add Max Reorganization Depth Sep 18, 2018
@blondfrogs blondfrogs merged commit d5ffead into develop2 Sep 18, 2018
@metacoin
Copy link

There's an issue with this that I'd like to bring up: any time a client coming online syincing to the network connects to a pool (honest or malicious) mining on a chain > 55 blocks, that client will never be able to connect to the rest of the network, even if they connect to other nodes and get the other part of the chain later on. The scenario could be a lot worse if two exchanges are on separate forks that they both consider valid because there's no way to tell which fork users are on and deposits and withdrawals will be lost.

I don't think it's a viable solution to consider which chain the client saw first as a consensus rule determining block validity because different clients can see different chains first. In classic PoW, these chain splits can be resolved by measuring cumulative proof-of-work, but in this case there would be a irreconcilable fork.

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.

5 participants