test: remove circular dependency between authproxy and util #35049

rkrux commented at 11:26 AM on April 10, 2026: contributor

I noticed this issue while reviewing PR 34773 where a lazy import is added in the call method of the AuthServiceProxy class in authproxy.py

There's a circular dependency between authproxy.py and util.py due to which the former can't use the common utility functions and thus lazy imports are used as a workaround.

This patch set breaks the dependency so that authproxy.py can use the utility functions from util.py in a standard fashion. Few tests that explicitly use get_rpc_proxy and JSONRPCException needed to have their imports updated.

DrahtBot added the label Tests on Apr 10, 2026

DrahtBot commented at 11:26 AM on April 10, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35179 (test: Add importdescriptors rpc error coverage by polespinasa)
#31668 (Added rescan option for import descriptors by saikiran57)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

node_number: the node number (or id) that this calls to -> node_number: the node number (or id) that this refers to [“this calls to” is grammatically broken and obscures the meaning]

rkrux commented at 11:31 AM on April 10, 2026: contributor

Ah taking a look at the failing tests, the test runner had passed on my local.

l0rinc commented at 11:32 AM on April 10, 2026: contributor

Can you do the bulk of the change via a scripted diff to help with the review?

DrahtBot added the label CI failed on Apr 10, 2026

rkrux force-pushed on Apr 10, 2026

DrahtBot removed the label CI failed on Apr 10, 2026

maflcko commented at 1:34 PM on April 10, 2026: member

Not sure about placing the different assert_ functions into different modules. If the circular dep is an issue, I think it may be easier to just leave the file as-is and not create the circular dep in the first place

rkrux commented at 1:42 PM on April 10, 2026: contributor

Not sure about placing the different assert_ functions into different modules.

I had not planned on it but I had to do it because they use JSONRPCException which is present in authproxy. Maybe there could be a way wherein JSONRPCException can be removed from authproxy so that assert_* need not be moved out - I will take a look; I, too, would prefer a minimal diff.

rkrux force-pushed on Apr 10, 2026

DrahtBot added the label CI failed on Apr 10, 2026

rkrux force-pushed on Apr 11, 2026

DrahtBot removed the label CI failed on Apr 11, 2026

rkrux commented at 8:46 AM on April 11, 2026: contributor

Re previously failing job run: I've reordered the commits so that the first commit doesn't fail with the circular dependency issue in the test ancestor commits job.

Not sure about placing the different assert_ functions into different modules.

I have changed the approach so that the assert_* functions need not be split into multiple modules. Only the get_rpc_proxy function is moved out from and JSONRPCException is moved in the util module. This removes util's dependency on authproxy so that the latter can use the former freely.

Can you do the bulk of the change via a scripted diff to help with the review?

With the new approach, the diff has reduced significantly and the PR now consisting of two commits should be easier to review.

Bicaru20 commented at 11:37 PM on April 12, 2026: none

If I understood the problem correctly, in #34773, importing assert_equal in authproxy.py it creates a circular dependency since authproxy.py imports utils.py to access assert_equal, while utils.py imports authproxy.py because it needs AuthServiceProxy and JSONRPCException. And in order to avoid lazy imports and still allow assert_equal to be used in authproxy.py, this PR is necessary.

That being said, I have two concerns:

I am not sure if it is okey to just create a file for the function get_rpc_proxy. Shouldn't we also move all RPC/I2P connections constants and functions to the same file to keep the code organized?

I think it may be easier to just leave the file as-is and not create the circular dep in the first place

If this is only needed for PR 34773, I agree it might be better to leave the file as-is. However by doing this changes we enforce the use of assert_equal everywhere and keep the code cleaner. So again, if this is just to solve the lazy import in PR 34773, it may be better to leave the code as it was, but if by doing this changes we avoid this same problem in the future (although that’s hard to know right now), it may be worth pursuing this.

DrahtBot added the label Needs rebase on Apr 13, 2026

maflcko commented at 7:52 AM on April 13, 2026: member

Not sure about placing the different assert_ functions into different modules.

I had not planned on it but I had to do it because they use JSONRPCException which is present in authproxy. Maybe there could be a way wherein JSONRPCException can be removed from authproxy so that assert_* need not be moved out - I will take a look; I, too, would prefer a minimal diff.

The minimal diff would be:

git show d9a3cf20a4960634440168fd5b09a40d577c3224 ./test/functional/test_framework/authproxy.py | git apply --reverse

rkrux commented at 8:30 AM on April 13, 2026: contributor

The minimal diff would be:

I meant a minimal diff to remove the circular import issue. :)

Are you suggesting that you'd prefer adding the lazy import instead of removing the circular import issue so that the import be added at the top of the file? If yes, then can you please n-a-c-k this PR?

I don't prefer such lazy imports at all but I can close this PR if there are multiple n-a-c-k-s.

maflcko commented at 8:37 AM on April 13, 2026: member

The minimal diff would be:

I meant a minimal diff to remove the circular import issue. :)

The diff removes the import. It is applied in reverse.

rkrux commented at 8:50 AM on April 13, 2026: contributor

It is applied in reverse.

Oh I see.

git show d9a3cf20a4960634440168fd5b09a40d577c3224 ./test/functional/test_framework/authproxy.py | git apply --reverse

-            from .util import assert_equal
-            assert_equal(response['jsonrpc'], '2.0')
+            assert response['jsonrpc'] == '2.0'

I'm a ~0 on this suggestion, I can add it but it'd defeat the purpose of PR #34773.

rkrux commented at 8:53 AM on April 13, 2026: contributor

Re #34773 (review)

If the import is a problem then it may be fine to just leave the file as-is for now. But anything is fine here

Now I understand this comment of yours, I was confused earlier by "leave the file as-is for now" when I read it in the other PR.

maflcko commented at 9:25 AM on April 13, 2026: member

I'm a ~0 on this suggestion, I can add it but it'd defeat the purpose of PR #34773.

Let's recall that the goal is "more useful failure output", not to use a helper by itself. For this particular line, I'd say a failure of KeyError is much more likely than the hard-coded value being off. Even if it was, running git grep 'pushKV("jsonrpc", ' to see the values that can possibly be pushed should be trivial.

The benefit of assert_equal failure output is when an (intermittent) runtime err occurs. E.g. when the wallet balance is checked to be equal to something, but the check fails. Then, it could be helpful to see the left and right side printed.

No strong opinion, but I think a 50+ line diff here doesn't seem worth it.

in test/functional/test_framework/rpc_util.py:5 in 271e357193 outdated

   0 | @@ -0,0 +1,32 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2026-present The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +from typing import Optional

l0rinc commented at 10:17 AM on April 13, 2026:

271e357 test: move out get_rpc_proxy from util:

"""RPC connection helpers for the functional test framework."""

in test/functional/test_framework/rpc_util.py:10 in 271e357193

   5 | +from typing import Optional
   6 | +
   7 | +from .authproxy import AuthServiceProxy
   8 | +from .coverage import AuthServiceProxyWrapper, get_filename
   9 | +
  10 | +def get_rpc_proxy(url: str, node_number: int, *, timeout: Optional[int]=None, coveragedir: Optional[str]=None) -> AuthServiceProxyWrapper:

l0rinc commented at 10:18 AM on April 13, 2026:

271e357 test: move out get_rpc_proxy from util:

nit: this is already non-move-only (from .coverage import AuthServiceProxyWrapper, get_filename), consider reformatting these lines while we're here:

def get_rpc_proxy(url: str, node_number: int, *, timeout: Optional[int] = None, coveragedir: Optional[str] = None) -> AuthServiceProxyWrapper:

in test/functional/test_framework/authproxy.py:1 in a6858d90d5 outdated

l0rinc commented at 10:30 AM on April 13, 2026:

We need to rebase after #34773:

diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py
--- a/test/functional/test_framework/authproxy.py	(revision 400b1e5e1cebbd231bb4ec3361275869256cbccf)
+++ b/test/functional/test_framework/authproxy.py	(date 1776075854888)
@@ -44,7 +44,7 @@
 import time
 import urllib.parse
 
-from .util import JSONRPCException
+from .util import JSONRPCException, assert_equal
 
 HTTP_TIMEOUT = 30
 USER_AGENT = "AuthServiceProxy/0.1"
@@ -139,7 +139,6 @@
             else:
                 return response['result']
         else:
-            from .util import assert_equal
             assert_equal(response['jsonrpc'], '2.0')
             if status != HTTPStatus.OK:
                 raise JSONRPCException({

l0rinc approved

l0rinc commented at 10:32 AM on April 13, 2026: contributor

Concept ACK

This patch set breaks that cycle in two steps, moving get_rpc_proxy() into a dedicated helper module so util.py no longer depends on coverage or AuthServiceProxy, followed by moving JSONRPCException into util.py, removing util.py's remaining dependency on authproxy.py.

The PR needs a rebase, I'm fine with the changes otherwise.

test: move out get_rpc_proxy from util

So that util is not dependent on coverage and AuthServiceProxy.
Reviewing with --color-moved=dimmed-zebra option will make it easier.

2c5d2594bb

test: move out JSONRPCException from authproxy to util

So that util is not dependent on authproxy at all and going
forward authproxy can use util methods.

Reviewing with --color-moved=dimmed-zebra option can be helpful.

c617a905e8

test: remove the lazy import of util in authproxy

This lazy import prompted the removal of the circular dependency, so
remove it now that we can.

404d5a9009

rkrux force-pushed on Apr 24, 2026

rkrux commented at 3:07 PM on April 24, 2026: contributor

Re: #35049 (comment)

I sat on this comment for couple of weeks (unintentionally) and ended up thinking over the pros & cons here meanwhile.

Let's recall that the goal is "more useful failure output", not to use a helper by itself. For this ... The benefit of assert_equal failure output is when ...

I agree with these points.

No strong opinion, but I think a 50+ line diff here doesn't seem worth it.

This 50+ line diff is in the testing framework, some of which can be reviewed with --color-moved=dimmed-zebra option.

I was uncomfortable ack-ing #34773 because of this lazy import but still went ahead because I didn't want to stop that PR from getting merged. I am unable to convince myself that we shouldn't use the common utility helper we have because of this circular dependency issue. Specially now that this issue has been identified and can be rectified with a relatively small diff, I don't see the benefit in either reverting the partial changes of #34773 or in leaving this circular dependency in the framework.

It could be the case that authproxy.py might be updated in the future that requires utilities in which case also removing this circular dependency can prove to be useful.

l0rinc commented at 3:28 PM on April 24, 2026: contributor

code review ACK 404d5a9009a3c9bececdb8d08de0b9c617383ca3

DrahtBot removed the label Needs rebase on Apr 24, 2026