Skip to content

Conversation

@roshii
Copy link
Contributor

@roshii roshii commented Oct 26, 2025

No description provided.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

these two should be updated too

JoinMarket requires Python >=3.8, <3.13 installed.

JoinMarket requires Python >=3.8, <3.13.

@roshii roshii force-pushed the chore/python-3.13 branch from 1326667 to f4d58ae Compare October 27, 2025 08:58
@3nprob
Copy link

3nprob commented Oct 27, 2025

@roshii did you try running this locally?
Last time I gave this a shot myself, I got runtime issues which weren't caught in unit tests due to twisted < 24.10 using deprecated functionality removed in python 3.13.

@roshii
Copy link
Contributor Author

roshii commented Oct 27, 2025

ideally yes, we'd want twisted>=24.10 but this release bring no added value to joinmarket when it comes to py3.13 compatibility, the latter solely fix their test suite, not the source code itself.
and yes: i'm running joinmarket on python 3.13 for a couple weeks now, as a charm :)

@3nprob
Copy link

3nprob commented Oct 27, 2025

I don't see any issues with python3.13 anymore either - looks like the twisted 24.7 bump in #1732 was sufficient to do the trick.

BTW, given results in #1802 (comment) I think it would be prudent to keep testing python 3.12 for regressions alongside 3.13 and 3.9? Rerunning the CI on my fork a bunch of times the failure only reproduces for 3.12 so far.

chore(docs): update python version requirement
@roshii roshii force-pushed the chore/python-3.13 branch from f4d58ae to ea9557b Compare October 28, 2025 09:52
matrix:
os: [macos-13, ubuntu-latest]
python-version: ["3.9", "3.12"]
python-version: ["3.9", "3.13"]
Copy link

@3nprob 3nprob Nov 1, 2025

Choose a reason for hiding this comment

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

Suggested change
python-version: ["3.9", "3.13"]
python-version: ["3.9", "3.12", "3.13"]

#1814 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels overkill to me, not sure there's much added value...

Copy link

@3nprob 3nprob Nov 1, 2025

Choose a reason for hiding this comment

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

In the linked thread we already see a possible regression stemming from the twisted upgrade only arising in 3.12 but not 3.13, no? Detecting such situations before merge and release doesn't seem like overkill as long as 3.12 is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@3nprob, afaict the failing test you are referring too is know to be flaky. i had written a potential structural fix in #1565

bottom line, extending test matrix would not bring much imo

Copy link

@3nprob 3nprob Nov 1, 2025

Choose a reason for hiding this comment

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

@3nprob, afaict the failing test you are referring too is know to be flaky. i had written a potential structural fix in #1565

I think this is actually unrelated and different?

Ie, what I believe is an example of the flakiness you mention: https://github.com/JoinMarket-Org/joinmarket-clientserver/actions/runs/18870804891/job/53848480732

_____ ERROR at teardown of TrialTestWRPC_DisplayWallet.test_unlock_locked ______
'NoneType' object is not iterable

During handling of the above exception, another exception occurred:
NOTE: Incompatible Exception Representation, displaying natively:

twisted.trial.util.DirtyReactorAggregateError: Reactor was unclean.
E           Exception: Failed to sync in fast mode after 20 batches; please re-try wallet sync with --recoversync flag.

I've also anecdotally seen spurious timeout and some AssertionError 0 === 1 happen locally.

Different failing unit and errors (afaict the unexpected 200 status code there have not been seen otherwise?). The failures I mention can also occur earlier than 120s in, vs the ones mentioned in #1565. I've gone as far back in JM CI runs as logs are retained and can not see it happening historically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is actually unrelated and different?

right, i went too quick in conclusion, this is likely unrelated

I've also anecdotally seen spurious timeout and some AssertionError 0 === 1 happen locally.

i've seen these ones too from time to time, much less, or not, with 3.13 as far as i recall.

bottom line, flaky tests should be identified locally and addressed structurally, i'm not in favor of "fat" ci runs to capture flakiness

Copy link
Member

Choose a reason for hiding this comment

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

I'm against increasing matrix too much here. Actually, when I proposed originally to have tests with both two Python and two Bitcoin Core versions, there was some opposition even against that. In any case, this discussion is out of scope of this PR.

@3nprob 3nprob mentioned this pull request Nov 1, 2025
Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK ea9557b. Checked locally that tests passes and there are no other changes that could break things.

@kristapsk kristapsk merged commit 21cbc31 into JoinMarket-Org:master Nov 2, 2025
7 of 12 checks passed
@roshii roshii deleted the chore/python-3.13 branch November 3, 2025 19:48
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