test: intermittent issue in p2p_1p1c_network.py #31946

issue maflcko openend this issue on February 24, 2025

maflcko commented at 2:50 pm on February 24, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328?pr=31603#step:6:3286

Originally posted by @brunoerg in #31721
maflcko added the label CI failed on Feb 24, 2025
darosior commented at 2:50 pm on March 4, 2025: member

Another instance occurred in #31603 https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328.
glozow commented at 8:25 pm on March 4, 2025: member

Is it possible the fix in #31751 has a race, where disconnect_p2ps() hasn’t finished disconnecting / erasing the peer’s orphan requests when the package is submitted?
mzumsande commented at 10:52 pm on March 4, 2025: contributor

Is it possible the fix in #31751 has a race, where disconnect_p2ps() hasn’t finished disconnecting / erasing the peer’s orphan requests when the package is submitted?

hmm, it shouldn’t be possible, because of this wait. Looking into the logs, the python nodes never get disconnected until the end (so it’s not even a race), there are also no “Closed connection to” log entries that python would write if it attempted to close a connection. Weird…
mzumsande commented at 11:50 pm on March 4, 2025: contributor

maybe the test-each-commit job just didn’t pick up #31751 for some reason?

In both logs of the failed runs I see TEST_BASE: 5691fa93c48c5b2c767f19aad5e972e4d760414a 5691fa93c48c5b2c767f19aad5e972e4d760414a commit is older than #31751 / 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416 so if that wasn’t included these errors would make sense. @maflcko can you help?
willcl-ark added the label Tests on Mar 20, 2025
maflcko commented at 2:04 pm on March 20, 2025: member

Sorry for missing the ping. The goal of the test-each-commit is to check for bisect errors on the original commits. Thus, it will be susceptible to intermittent issues that were fixed in current master.

Maybe the docs could be improved to mention this?

Other than than, I’d recommend to just rebase on master when a known/fixed issue is hit in the test-each-commit task, or to re-run the task.
maflcko added the label Brainstorming on Mar 20, 2025
mzumsande commented at 3:46 pm on March 20, 2025: contributor

Ok thanks - then it looks like there is no actual issue with p2p_1p1c_network.py to fix anymore.

The goal of the test-each-commit is to check for bisect errors on the original commits.

Would it be possible to first rebase the PR branch onto current master, and only then do the test-each-commit procedure from there? That way, inconsistencies inside amongst the commits of the PR would still be found (maybe even more because silent conflicts that are only there in intermediate commits would now be detected), but intermittent issues fixed in current master wouldn’t be an issue anymore.
maflcko commented at 3:51 pm on March 20, 2025: member

Yeah, let’s continue review/discussion in one place ( #32103)
maflcko closed this on Mar 20, 2025

Contributors
maflcko darosior glozow mzumsande

Labels
Brainstorming Tests CI failed