There should be no risk or downside in adding a call to SyncWithValidationInterfaceQueue here. In fact, it will make tests less brittle. For example,
test/functional/feature_fee_estimation.py to 0, on master the test will fail and here it will pass.
This makes existing tests less brittle, see
https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
node_context is never null, but if it was, it would lead to a nullptr
dereference in node_context->scheduler. Just use EnsureAnyNodeContext
everywhere for more robust, consistent, and correct code.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, furszy |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK fabef121b0cdfac6ec1985f6c08c5685a886ba5a
Seems like the correct thing to do here anyway. Also reproduced the feature_fee_estimation behavior change.
Little topic, mainly to not mix RPC commands responsibilities.
Why not implementing this on the test framework instead?
Could add a proxy function mockscheduler to test_node.py that calls to the real mockscheduler and, subsequently to syncwithvalidationinterfacequeue.
Why not implementing this on the test framework instead?
That just seems brittle, and bloated, for no reason? It is brittle because one may miss to implement a proxy, as they'll have to be done for the bitcoin-cli utility and the python rpc wrapper, and devs can still accidentally sidestep it. It is bloated because it will be more than one line of code.
That just seems brittle, and bloated, for no reason? It is brittle because one may miss to implement a proxy, as they'll have to be done for the bitcoin-cli utility and the python rpc wrapper, and devs can still accidentally sidestep it. It is bloated because it will be more than one line of code.
I'm not really strong here but It just seems a bit error-prone to always call to the syncwithvalidationinterfacequeue internally after bumping the scheduler timeout and not before. Existing callbacks using the node's time to operate could be affected and perform differently (sometimes we might want this, some others we might not?). And thought that it would be easier to change/adapt when calls are done outside of the RPC command.
About the tests bloat, it should just be a:
def mockscheduler(self, time):
self.__getattr__('mockscheduler')(time)
self.__getattr__('syncwithvalidationinterfacequeue')()
But it is just a gut feeling. All good either way.
Existing callbacks using the node's time to operate could be affected and perform differently
No, they shouldn't be affected by this change. The scheduler thread is already free to behave at any time as-if syncwithvalidationinterfacequeue was just called.
This will only reduce intermittent test failure rates where the test assumed that the scheduler thread behaved as-if syncwithvalidationinterfacequeue was called and that it must happen within a specific time. As explained in the description there are tests with this assumption. This pull request fixes those tests, and all other cases of the same kind in all current and future tests.
Code review ACK fabef121. Convinced by checking all current tests usages. Thanks Marco.