Repository: bitcoin/bitcoin
File: test/functional/test_framework/test_node.py (Python)

Code before:
                    # loading the mempool. Usually importing happens fast or
                    # even "immediate" when the node is started. However, there
                    # is no guarantee and sometimes ImportBlocks might finish
                    # later. This is going to cause intermittent test failures,
                    # because generally the tests assume the node is fully
                    # ready after being started.
                    #
                    # For example, the node will reject block messages from p2p
                    # when it is still importing with the error "Unexpected
                    # block message received"
                    #
                    # The wait is done here to make tests as robust as possible
                    # and prevent racy tests and intermittent failures as much
                    # as possible. Some tests might not need this, but the
                    # overhead is trivial, and the added guarantees are worth
                    # the minimal performance cost.
                self.log.debug("RPC successfully started")
                if self.use_cli:
                    return
                self.rpc = rpc
                self.rpc_connected = True
                self.url = self.rpc.rpc_url
                return
            except JSONRPCException as e:  # Initialization phase
                # -28 RPC in warmup
                # -342 Service unavailable, RPC server started but is shutting down due to error
                if e.error['code'] != -28 and e.error['code'] != -342:
                    raise  # unknown JSON RPC exception
            except ConnectionResetError:
                # This might happen when the RPC server is in warmup, but shut down before the call to getblockcount
                # succeeds. Try again to properly raise the FailedToStartError
                pass
            except OSError as e:
                if e.errno == errno.ETIMEDOUT:
                    pass  # Treat identical to ConnectionResetError
                elif e.errno == errno.ECONNREFUSED:
                    pass  # Port not yet open?
                else:
                    raise  # unknown OS error
            except ValueError as e:  # cookie file not found and no rpcuser or rpcpassword; bitcoind is still starting
                if "No RPC credentials" not in str(e):
                    raise
            time.sleep(1.0 / poll_per_s)
        self._raise_assertion_error("Unable to connect to bitcoind after {}s".format(self.rpc_timeout))

    def wait_for_cookie_credentials(self):
        """Ensures auth cookie credentials can be read, e.g. for testing CLI with -rpcwait before RPC connection is up."""
        self.log.debug("Waiting for cookie credentials")
        # Poll at a rate of four times per second.
        poll_per_s = 4
        for _ in range(poll_per_s * self.rpc_timeout):

Review comment (refactor):
In commit "qa refactor: wait_for_rpc_connection - Treat OSErrors the same" (8fa04cb13a4bf62a42b18e7c0c79151d13cb494b)

Various thoughts:

- I think "initialization phase" comment that was removed provided more context and was more understandable than the new "Suppress these in favor of" comment.
- I don't see why it is useful to list later outcomes that can happen after these errors are suppressed, especially when the list seems to contains every imaginable outcome.
- I don't understand why "-342 Service unavailable, RPC server started but is shutting down due to error" comment is being changed to "-342 Service unavailable, could be starting up or shutting down". These descriptions seem inconsistent and I don't know which one is correct since I don't see where -342 is being returned and https://github.com/bitcoin/bitcoin/blob/master/src/rpc/protocol.h doesn't seem to contain that error code.

Given all this, I'd suggest just keeping the comments in this except block unchanged.

But if not doing that, I think "Suppress these" should be changed to something like "Suppress early init errors" so overall purpose of the except block is clear.



Code after:
                    self.wait_until(lambda: rpc.getmempoolinfo()['loaded'])
                    # Wait for the node to finish reindex, block import, and
                    # loading the mempool. Usually importing happens fast or
                    # even "immediate" when the node is started. However, there
                    # is no guarantee and sometimes ImportBlocks might finish
                    # later. This is going to cause intermittent test failures,
                    # because generally the tests assume the node is fully
                    # ready after being started.
                    #
                    # For example, the node will reject block messages from p2p
                    # when it is still importing with the error "Unexpected
                    # block message received"
                    #
                    # The wait is done here to make tests as robust as possible
                    # and prevent racy tests and intermittent failures as much
                    # as possible. Some tests might not need this, but the
                    # overhead is trivial, and the added guarantees are worth
                    # the minimal performance cost.
                self.log.debug("RPC successfully started")
                # Set rpc_connected even if we are in use_cli mode so that we know we can call self.stop() if needed.
                self.rpc_connected = True
                if self.use_cli:
                    return
                self.rpc = rpc
                self.url = self.rpc.rpc_url
                return
            except JSONRPCException as e:
                # Suppress these as they are expected during initialization.
                # -28 RPC in warmup
                # -342 Service unavailable, could be starting up or shutting down
                if e.error['code'] not in [-28, -342]:
                    raise  # unknown JSON RPC exception
                suppressed_errors[f"JSONRPCException {e.error['code']}"] += 1
                latest_error = repr(e)
            except OSError as e:
                error_num = e.errno
                # Work around issue where socket timeouts don't have errno set.
                # https://github.com/python/cpython/issues/109601
                if error_num is None and isinstance(e, TimeoutError):
                    error_num = errno.ETIMEDOUT

                # Suppress similarly to the above JSONRPCException errors.
                if error_num not in [
                    errno.ECONNRESET,   # This might happen when the RPC server is in warmup,
                                        # but shut down before the call to getblockcount succeeds.
                    errno.ETIMEDOUT,    # Treat identical to ECONNRESET
                    errno.ECONNREFUSED  # Port not yet open?
                ]:
                    raise  # unknown OS error
                suppressed_errors[f"OSError {errno.errorcode[error_num]}"] += 1
                latest_error = repr(e)