Skip to content

[TierTwo] Fix more circular dependencies. #2657

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.

Sign up for GitHub

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

Jump to bottom
Merged
furszy merged 6 commits into from
Dec 16, 2021
Merged

[TierTwo] Fix more circular dependencies. #2657

furszy merged 6 commits into from
Dec 16, 2021

Conversation

@furszy
Copy link

@furszy furszy commented Nov 29, 2021 •
edited
Loading

Fixed some circular dependencies over the tier two and net_processing due the cs_main/Misbehaving calls.

Making this now, and not waiting to post-v6.0 (where we can directly remove the entire masternode manager file) because it affects the mnauth workflow introduced in #2647 which adds other circular dependencies.

@furszy furszy self-assigned this Nov 29, 2021
@random-zebra
Copy link

random-zebra commented Nov 29, 2021

There's a couple of bugs making the various Misbehaving calls unreachable.
The semantic for the boolean return value of the *::ProcessMessage functions is switched: true should mean "all good", which would be dosScore == 0.
Also, if you check its return value, there's no need to double check dosScore > 0 outside of ProcessMessage.

@furszy furszy force-pushed the 2021_circ_dependency branch 3 times, most recently from bdb9fa7 to bfcd5cc Compare November 30, 2021 14:04
@furszy
Copy link
Author

furszy commented Nov 30, 2021

done, and rebased on master. Nice quick feedback zebra.

@furszy furszy mentioned this pull request Nov 30, 2021
Merged
@random-zebra random-zebra added this to the 6.0.0 milestone Dec 1, 2021
@furszy furszy force-pushed the 2021_circ_dependency branch from bfcd5cc to 190cf69 Compare December 3, 2021 14:19
@furszy
Copy link
Author

furszy commented Dec 3, 2021

rebased, conflicts solved.

@furszy furszy force-pushed the 2021_circ_dependency branch from 190cf69 to 00a9978 Compare December 5, 2021 12:26
@furszy
Copy link
Author

furszy commented Dec 5, 2021

rebased, conflicts solved, ready to go.

@furszy furszy force-pushed the 2021_circ_dependency branch from 00a9978 to 250bb33 Compare December 5, 2021 14:14
@random-zebra
Copy link

random-zebra commented Dec 6, 2021

Code ACK. Although this is going to have a conflict with #2659. We probably want to merge that one before.

@furszy
Copy link
Author

furszy commented Dec 6, 2021

yeah, better to merge 2659 first so the 5.4 backport work is more straightforward.

@random-zebra
Copy link

random-zebra commented Dec 14, 2021

This can be rebased now.

@furszy furszy force-pushed the 2021_circ_dependency branch from 250bb33 to a02faf0 Compare December 14, 2021 14:30
@furszy
Copy link
Author

furszy commented Dec 14, 2021

rebased, conflicts solved.

random-zebra
random-zebra approved these changes Dec 14, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK a02faf0

For the future, as discussed, we should keep in mind the new interface between validation and net_processing in upstream (stop passing DoS scores around in the validation code, and just rely on CValidationState).

Fuzzbawls
Fuzzbawls approved these changes Dec 16, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK a02faf0

@furszy furszy merged commit 1709f6b into PIVX-Project:master Dec 16, 2021
@Fuzzbawls Fuzzbawls removed this from the 6.0.0 milestone Sep 11, 2022
@Fuzzbawls Fuzzbawls added this to the 5.5.0 milestone Sep 11, 2022
@furszy furszy deleted the 2021_circ_dependency branch November 29, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

+1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

P2P Refactoring

Projects

None yet

Milestone

5.5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@furszy @random-zebra @Fuzzbawls