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

Block and network dead code removal. #1329

Closed
wants to merge 2 commits into from
Closed

Block and network dead code removal. #1329

wants to merge 2 commits into from

Conversation

barrystyle
Copy link

Possibly controversial due to the spam filter removal, but it is dealt with in a different manner.
Some minor code removed from the network core.

@furszy
Copy link

furszy commented Feb 12, 2020

Can you please describe how the spam protection is achieved after 672cfb2?.

The more information the better for anyone reviewing this PR, less imagination and pre-concepts from our side.

@barrystyle
Copy link
Author

barrystyle commented Feb 14, 2020

Sorry for late reply; this is a derivative of bitcoin@9be0e68 / bitcoin@9d60602 which I had adapted from Dash 0.12.1.x series.
Unless we had explicitly requested this block ; or it is from a whitelisted peer, then we will not process it via ProcessNewBlock. Since it is not possible to know the 'chainwork' of a proof of stake block (like PoW can via fHasMoreWork), that type of comparative logic isn't applied.
MarkBlockAsReceived has been adapted to return a bool to indicate whether we did indeed request this block.

As well - the function in net.cpp/h isnt referenced from any piece of code.

@furszy
Copy link

furszy commented Feb 16, 2020

Ok, now know where it's going. Thanks for the description.

As far as I can see it's not doing what you want, the fRequested is just being set, then not used. It will process every single block that it's received, independently if the node have it or not.

Will need to think more about this after check how the not requested block rejection is coded but, at least in my mind, i'm pretty sure that we will find a way to surpass it and DoS the peer under this flow.
It will not be trivial to implement something like this properly without the PoW restrictions.


Side from that, create another PR for the RecvLine not used code removal. We can merge it right away.

@furszy
Copy link

furszy commented Apr 1, 2020

Closing this for lack of response.

@furszy furszy closed this Apr 1, 2020
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.

None yet

2 participants