Skip to content

Conversation

@3nprob
Copy link

@3nprob 3nprob commented Sep 27, 2025

  • Bump to twisted 24.11.0 to fix python3.13 compatibility
  • Indicate python3.13 support in pyproject.toml
  • CI(unittests): Add python3.13 to python-versions
  • CI(unittests): Make ubuntu version explicit
    • During periods of migration it can be inconsistent and/or unpredictable when the transition of ubuntu-latest will happen for a given repo. Making it explicit removes it as source of confusion.
    • Run on both ubuntu-22.04 and ubuntu-24.04 to make sure there are no regressions in the former as long as it is supported.

Fixes:

@seamo1
Copy link

seamo1 commented Sep 27, 2025

NACK
You're rewriting the whole bencoder project & code and creating a whole new release of it in this repo!
This is too complex, convoluted and hard to audit.
Please submit your changes upstream to bencoder.pyx itself & get everyone to review it there.

@3nprob
Copy link
Author

3nprob commented Sep 27, 2025

You're rewriting the whole bencoder project & code and creating a whole new release of it in this repo! This is too complex, convoluted and hard to audit.

Hm?

This can be reviewed and audited commit-by-commit and is far from "rewriting the whole project".

  • Here upstream is copied/vendored as-is: 92e73e1
  • Here is the one-line change: 1bd27af
  • The rest is all to make CI actually run and test with improved coverage
    • Happy to break out or tweak this if preferred by maintainers

Please submit your changes upstream to bencoder.pyx itself & get everyone to review it there.

That would be my preferred default approach as well but unfortunately, upstream is unmaintained for > 2 years now.

An alternative would be to replace the module completely with an alternative implementation (which?) but to me it seems preferred to first do this minimal change, and then do that replacement as a follow-up (given the current state and timeline).

@3nprob
Copy link
Author

3nprob commented Sep 27, 2025

I guess it's appropriate to add an entry to the bencoder.pyx changelog. Done now.

There is an upstream PR with the same change but again the package doesn't appear maintained: whtsky/bencoder.pyx#145

@roshii
Copy link
Contributor

roshii commented Sep 29, 2025

NACK switching to a maintained library seem more appropriate to me.

@3nprob
Copy link
Author

3nprob commented Sep 29, 2025

NACK switching to a maintained library seem more appropriate to me.

Could this (vendoring) be a stepping stone? Unrelated development and fixes appear blocked by the broken CI (#1741, #1782, etc). Unbreaking that will also help validate a replacement.

@kristapsk
Copy link
Member

#1809 seems nicer solution.

@kristapsk
Copy link
Member

Bump to twisted 24.11.0 to fix python3.13 compatibility

Could you split off this one in a separate PR?

@3nprob
Copy link
Author

3nprob commented Oct 26, 2025

Bump to twisted 24.11.0 to fix python3.13 compatibility

Could you split off this one in a separate PR?

Sure! For now holding off waiting for #1809 or equivalent to be merged first so CI tests can run cleanly. I wouldn't feel comfortable merging changes without CI suites running on a project of this nature.

@3nprob 3nprob marked this pull request as draft October 26, 2025 07:32
@3nprob 3nprob force-pushed the python3.13-compat branch from a7a604f to 7bbd5bf Compare October 26, 2025 21:44
def failed_refresh_response_handler(
self, response, *, message=None, error_description=None
):
jlog.debug(f"failed_refresh_response_handler '{message}' ({error_description})")
Copy link
Author

@3nprob 3nprob Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this line was added, response.code 200 in CI here (but only for python3.12 on linux - not for 3.9 or 3.13), causing the test to fail:

After, they all passed: https://github.com/JoinMarket-Org/joinmarket-clientserver/actions/runs/18824722441?pr=1802

Currently I don't have an explanation for why this happened and if it's actually related to the twisted upgrade. The failures only appearing for 3.12 but not 3.13 or 3.9 may or may not be a red herring given the small sample size but it does look consistent. Some form of race? Related to buffer flushing...?

It's concerning if this is indicative of potential auth bypass...

Is anyone able to reproduce the test assert failure in test/jmclient/test_wallet_rpc.py locally?

Copy link
Author

@3nprob 3nprob Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subsequent failing runs with the log (also python3.12) indicating it's the unsupported_grant_type case being triggered:

https://github.com/3nprob/joinmarket-clientserver/actions/runs/18824977574/job/53706175110#step:10:176

Details

=================================== FAILURES ===================================
_________________ TrialTestWRPC_JWT.test_refresh_token_request _________________

self = <test_wallet_rpc.TrialTestWRPC_JWT testMethod=test_refresh_token_request>
response = <twisted.web._newclient.Response object at 0x7f5849081cd0>

    @defer.inlineCallbacks
    def failed_refresh_response_handler(
        self, response, *, message=None, error_description=None
    ):
        jlog.debug(f"failed_refresh_response_handler '{message}' ({error_description})")
>       assert response.code == 400
E       AssertionError: assert 200 == 400
E        +  where 200 = <twisted.web._newclient.Response object at 0x7f5849081cd0>.code

test/jmclient/test_wallet_rpc.py:850: AssertionError
----------------------------- Captured stdout call -----------------------------
User data location: .
2025-10-26 23:31:53,679 [DEBUG]  rpc: getblockchaininfo []
2025-10-26 23:31:53,680 [DEBUG]  rpc: listwallets []
2025-10-26 23:31:53,681 [DEBUG]  rpc: getwalletinfo []
2025-10-26 23:31:53,746 [DEBUG]  rpc: getnewaddress []
2025-10-26 23:31:53,748 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:53,762 [DEBUG]  rpc: sendtoaddress ['bcrt1qe0zcwq57ky4d8uql4nujrhpkkxpktpedl0e0n0', 2.0]
2025-10-26 23:31:53,959 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:53,967 [DEBUG]  rpc: sendtoaddress ['bcrt1qupdzgzv79qdhkcylqfs2jkjxpv3j0h4ff6spuu', 2.0]
2025-10-26 23:31:54,164 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:54,170 [DEBUG]  rpc: sendtoaddress ['bcrt1qy7626rmvgxmdx6wj30qwgke7w8zxd2vstej5nx', 2.0]
2025-10-26 23:31:54,368 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:54,374 [DEBUG]  rpc: sendtoaddress ['bcrt1q8xd2lhe6sx5ses85jdjwvv5ja8lpweskqc78gd', 2.0]
2025-10-26 23:31:54,573 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:54,575 [DEBUG]  rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
2025-10-26 23:31:54,576 [DEBUG]  rpc: listaddressgroupings []
2025-10-26 23:31:54,724 [INFO]  Detected new wallet, performing initial import
2025-10-26 23:31:54,725 [DEBUG]  requesting detailed wallet history
2025-10-26 23:31:54,725 [DEBUG]  rpc: listlabels []
2025-10-26 23:31:54,728 [DEBUG]  rpc: listunspent [0]
2025-10-26 23:31:54,964 [DEBUG]  bitcoind sync_unspent took 0.23537540435791016sec
2025-10-26 23:31:54,976 [DEBUG]  failed_refresh_response_handler 'invalid_request' (None)
2025-10-26 23:31:54,979 [DEBUG]  failed_refresh_response_handler 'unsupported_grant_type' (None)
----------------------------- Captured stderr call -----------------------------
Unhandled error in Deferred:

Traceback (most recent call last):
  File "/home/runner/work/joinmarket-clientserver/joinmarket-clientserver/jmvenv/lib/python3.12/site-packages/twisted/internet/defer.py", line 2017, in _inlineCallbacks
    result = context.run(gen.send, result)
  File "/home/runner/work/joinmarket-clientserver/joinmarket-clientserver/test/jmclient/test_wallet_rpc.py", line 850, in failed_refresh_response_handler
    assert response.code == 400
builtins.AssertionError: assert 200 == 400
 +  where 200 = <twisted.web._newclient.Response object at 0x7f5849081cd0>.code

------------------------------ Captured log call -------------------------------
DEBUG    joinmarket:blockchaininterface.py:431 rpc: getblockchaininfo []
DEBUG    joinmarket:blockchaininterface.py:431 rpc: listwallets []
DEBUG    joinmarket:blockchaininterface.py:431 rpc: getwalletinfo []
DEBUG    joinmarket:blockchaininterface.py:431 rpc: getnewaddress []
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: sendtoaddress ['bcrt1qe0zcwq57ky4d8uql4nujrhpkkxpktpedl0e0n0', 2.0]
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: sendtoaddress ['bcrt1qupdzgzv79qdhkcylqfs2jkjxpv3j0h4ff6spuu', 2.0]
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: sendtoaddress ['bcrt1qy7626rmvgxmdx6wj30qwgke7w8zxd2vstej5nx', 2.0]
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: sendtoaddress ['bcrt1q8xd2lhe6sx5ses85jdjwvv5ja8lpweskqc78gd', 2.0]
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: generatetoaddress [1, 'bcrt1qxt62fdlsh8dhe4mj5cs4eldxccyv89sk0wm3jk']
DEBUG    joinmarket:blockchaininterface.py:431 rpc: listaddressgroupings []
INFO     joinmarket:wallet_service.py:560 Detected new wallet, performing initial import
DEBUG    joinmarket:wallet_service.py:755 requesting detailed wallet history
DEBUG    joinmarket:blockchaininterface.py:431 rpc: listlabels []
DEBUG    joinmarket:blockchaininterface.py:431 rpc: listunspent [0]
DEBUG    joinmarket:wallet_service.py:868 bitcoind sync_unspent took 0.23537540435791016sec
DEBUG    joinmarket:test_wallet_rpc.py:849 failed_refresh_response_handler 'invalid_request' (None)
DEBUG    joinmarket:test_wallet_rpc.py:849 failed_refresh_response_handler 'unsupported_grant_type' (None)
=========================== short test summary info ============================
FAILED test/jmclient/test_wallet_rpc.py::TrialTestWRPC_JWT::test_refresh_token_request
================== 1 failed, 425 passed in 301.58s (0:05:01) ===================
+ local success=1
+ [[ -f ./joinmarket.cfg ]]
+ unlink ./joinmarket.cfg
+ '[' -f /dev/shm/jm_test_home-runner/.bitcoin/bitcoind.pid ']'
+ [[ '' == true ]]
+ rm -rf /dev/shm/jm_test_home-runner/.bitcoin
+ return 1

Headers and response body: https://github.com/3nprob/joinmarket-clientserver/actions/runs/18826006246/job/53708752813

[DEBUG]  headers: (Headers({b'Server': [b'TwistedWeb/24.11.0'], b'Date': [b'Mon, 27 Oct 2025 00:35:58 GMT'], b'Content-Type': [b'application/json'], b'Access-Control-Allow-Origin': [b'*'], b'Cache-Control': [b'no-cache, no-store, must-revalidate'], b'Pragma': [b'no-cache'], b'Expires': [b'Sat, 26 Jul 1997 05:00:00 GMT']}))
[DEBUG]  reason: 'b'OK''

@3nprob
Copy link
Author

3nprob commented Oct 26, 2025

Bump to twisted 24.11.0 to fix python3.13 compatibility

Could you split off this one in a separate PR?

I have updated this PR with the reduced scope (twisted update + python3.13).

@3nprob 3nprob force-pushed the python3.13-compat branch from 6e7cfcb to 8adfb57 Compare October 28, 2025 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants