Navigation Menu

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

Remove unused default args to Invalid and DoS #12991

Closed
wants to merge 1 commit into from

Conversation

Empact
Copy link
Member

@Empact Empact commented Apr 15, 2018

These unused default args increase risk related to pending
refactoring of these interfaces.

These unused default args increase risk related to pending
refactoring of these interfaces.
@Empact
Copy link
Member Author

Empact commented Apr 15, 2018

After taking a few passes at this refactoring, I realized I want to keep the existing names if possible, so this is a step toward reducing risk associated with that, see:
#12976 (comment)
#12463 (comment)

@practicalswift
Copy link
Contributor

Concept ACK

@jnewbery
Copy link
Contributor

Rather than these minor refactors, I'd much rather see progress made on #11639, which clarifies the interface between net_processing and validation. It'd be a shame to have to rebase that PR and waste the reviewer cycles there.

@Empact - how would you feel about reviewing that PR?

@Empact
Copy link
Member Author

Empact commented Apr 17, 2018

@jnewbery Sorry, I'm at stage where I see the codebase more in the small than in the large, and I take the smaller PRs as neatly packaged unambiguous improvements. This was setting us up for this simplification: https://github.com/bitcoin/bitcoin/compare/master...Empact:drop-dos-return-corruption-scripted?expand=1

As for #11639, I like it, I'll make some comments over there. IMO its a bit easier to reason about after applying my changes:
https://github.com/bitcoin/bitcoin/compare/master...Empact:2017-10-dos-rewrite?expand=1

@Empact
Copy link
Member Author

Empact commented Apr 18, 2018

@jnewbery thanks for the guidance, will reopen post-#11639

@Empact Empact closed this Apr 18, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants