This fixes a null pointer crash in the bitcoin-gui PSBT dialog. The bitcoin-gui interfaces::Node object has a null NodeContext pointer, and can’t broadcast transactions directly. It needs to broadcast transactions through the bitcoin-node process instead.
This PR is part of the process separation project.
This fixes a null pointer crash in the bitcoin-gui PSBT dialog. The
bitcoin-gui interfaces::Node object has a null NodeContext pointer, and
can't broadcast transactions directly. It needs to broadcast
transactions through the bitcoin-node process instead.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
context, a name that implies it can be used in production code. Wouldn’t it be better to pick an ugly name like getMockedContext or getTestContext?
Seems dangerous to call a test-only method
context, a name that implies it can be used in production code. Wouldn’t it be better to pick an ugly name like getMockedContext or getTestContext?
I don’t really consider it a test-only method. The method is only currently used from tests, but it’s not returning a fake context, and I think it could be legitimately be called outside of tests. For example, the current code calling this method works because the method returns the real working context pointer. The problem just happens after #10102, because the code calling it is not checking for null, and #10102 makes the method start to return null if the node is out-of-process. The method could be legitimately used in other cases to enable or disable features depending on whether the node is in-process, or change GUI behavior to deal with latency or overhead.
I guess I’d prefer not to rename the method, but would reconsider it or move it to a friend class if it causes more problems. This is actually the first time I’ve seen the method misused any time, and it has been around a little while.
In that case the “Useful for testing” docstring can be removed? Any function is “Useful for testing” if it is called by the tests.
I guess I would replace “Useful for testing” with “Mostly useful for testing” because like you say any function can be useful for testing. But the point of these short method descriptions is to help narrow down what methods you may want to call when you are writing code, and “mostly useful for testing” should be a good hint that calling this is useful when you are writing test code and need to access state, and a hint that this is probably not what you need otherwise. If the documentation is a problem, it could explicitly go on to say that the method can return null and callers should check for nulls. But this won’t return null after #10102, and if there is actually another problem here I would look into a more solid protection like moving the method it to a friend class than trying to do something with documentation.