The destruction/resetting of node context members in the tests should roughly follow the behavior of the Shutdown function in init.cpp.
This was originally requested by MarcoFalke in this comment in response to the original pull request introducing the kernel::Context.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
143 | @@ -144,6 +144,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, const std::vecto 144 | 145 | BasicTestingSetup::~BasicTestingSetup() 146 | { 147 | + m_node.kernel.reset();
In commit "tests: Reset node context members on ~ChainTestingSetup" (960112dbb1fb028b73b6e83932dd457f5a69ab5d)
I think this change makes sense but the log message is misleading because in the Shutdown() function the kernel context is currently destroyed before chainman, mempool, scheduler, not after:
The test code is more correct than the Shutdown() code because ~BasicTestingSetup runs after ~ChainTestingSetup, and the chainman and scheduler objects should be destroyed before kernel context because they might rely on it.
It'd be good to either clarify the log message in this commit to say mention this part of the change doesn't match Shutdown function, or update the Shutdown function to destroy the kernel context last like the test. (I could also update the Shutdown function in #28051 if you want this PR to be test-only)
Code review ACK 960112dbb1fb028b73b6e83932dd457f5a69ab5d
Are you still working on this?
Yes.
I think there is an outstanding comment: #28224 (review) ?
Updated 960112dbb1fb028b73b6e83932dd457f5a69ab5d -> 99a668ceb80b8241c9eb24dca2858dbdd3ddf7ee (testSetupDestructorOrder_0 -> testSetupDestructorOrder_1, compare)
Code review ACK 99a668ceb80b8241c9eb24dca2858dbdd3ddf7ee
Changes look good, but I would suggest changing the PR title to something like: "shutdown: destroy kernel last and make test shutdown order consistent" because the current title "tests: Reset node context members on ~ChainTestingSetup" makes it seems like this is a test-only change.
Also think it would be better to split changes in init.cpp and setup_common.cpp into separate commits. The changes are both related to shutdown but they have different rationales and effects, so it doesn't really make sense for them to be in the same commit.
Currently the shutdown function resets the kernel before the
chainman and scheduler. Invert this order by resetting the kernel
last, since they might rely on the kernel.
The destruction/resetting of node context members in the tests should
roughly follow the behaviour of the Shutdown function in `init.cpp`.
Updated 99a668ceb80b8241c9eb24dca2858dbdd3ddf7ee -> c1144f0076339c775f41d4b5fcfdc72191440d96 (testSetupDestructorOrder_1 -> testSetupDestructorOrder_2, compare)
Code review ACK c1144f0076339c775f41d4b5fcfdc72191440d96. No code changes since last review, just updated commits and descriptions
ACK c1144f0076339c775f41d4b5fcfdc72191440d96 🗣
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK c1144f0076339c775f41d4b5fcfdc72191440d96 🗣
T9dJ5U2qRZJzAofSxNNeOCDY1x9g1v2UzSnLfzGQ+Ge86LGQmtA47PGOhyV4baDkH4joyvRrBHb2K0taFvI/Dw==
</details>
ACK c1144f0076339c775f41d4b5fcfdc72191440d96
