-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: improved initialization of members of LLMQContext and related changes #5150
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
4d2cb7f to
ad8f79e
Compare
|
This pull request has conflicts, please rebase. |
|
I attempted to get around the mempool problem by passing them by reference, not sure if this is the best way to do it. kwvg@6396c89 and kwvg@f201072 |
|
Mempool refactorings moved here: #5169 |
|
This pull request has conflicts, please rebase. |
698d52e to
e36f5b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK
NodeContext is a container for references that constitute a node, not a self-contained object that initializes its members or cleans up after itself. There is no NodeContext "unit" to initialize, so to speak, only the container, because that's what NodeContext is.
The burden of initialization of its members falls on each entity's initialization logic (that be setup_common.cpp for tests or init.cpp for daemons), which means when passing a NodeContext, we have to be very sure that everything within it is initialized and valid before passing any instance of it. An assurance that as of now, we cannot make.
This PR at least seems to be less interested in NodeContext's members but rather LLMQContext's and LLMQContext, unlike NodeContext mostly sets itself up and cleans up after, the key word being mostly.
Members of LLMQContext (CQuorumBlockProcessor, CQuorumManager, CChainLocksHandler and CInstantSendManager) are very much globals with its corresponding LLMQContext members being an alias to them, meant to migrate code to using NodeContext (and hence, LLMQContext) when possible and document the dependencies of each subsystem.
That is ultimately why we went with having long argument lists, documentation. During the initial deglobalization effort, it was a painstaking process to document all the uses of a global and then passing them by reference as an argument leading to a common initialization point, I had also attempted to use LLMQContext to cleanup the argument list, but found debugging to be painful as it obscured the order of initialization.
Perhaps if LLMQContext is entirely self-contained and we can guarantee that all its members are initialized before passing it, I would be open to it replacing our (admittedly quite long) arguments but as it stands, I feel it'll cause more problems than it'll solve.
|
This pull request has conflicts, please rebase. |
|
@PastaPastaPasta I removed commit with new circular dependencies. Please, help to get merged this PR. |
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
|
This pull request has conflicts, please rebase. |
New constructors uses LLMQContext instead of direct usage llmq's component. Refactoring some internal methods accordingly in rpc/mining.cpp It helps to avoid passing too many arguments and make code cleaner. Beside that it can add more components or refactor llmq/ module without chaging interface of BlockAssembler that is widely used in unit tests
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK for squash merge
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
…not using default ports 010eed3 doc: warn that incoming conns are unlikely when not using default ports (Adam Jonas) Pull request description: Closes dashpay#5150. This was mostly copied from dashpay#5285 by sulks, who has since quit GitHub. The issue has remained open for 6 years, but the extra explanation still seems useful. ACKs for top commit: laanwj: re-ACK 010eed3 Tree-SHA512: d240fb06bba41ad8898ced59356c10adefc09f3abb33e277f8e2c5980b40678f2d237f286b476451bb29d2b94032a7dee2ada3b2efe004ed1c2509e70b48e40f
LLMQContext uses RAII to initialize all members. Ensured that all members always initialized correctly in proper order if LLMQContext exists.
BlockAssembler, CChainState use too many agruments and they are making wrong assumption that members of LLMQContext can be constructed and used independently, but that's not true. Instead, let's pass LLMQContext whenever possible.
Issue being fixed or feature implemented
https://github.com/dashpay/dash-issues/issues/52
How Has This Been Tested?
Run unit/functional test and introduce no breaking changes.
Checklist: