💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869261700)
Hmm, right. Then I have no idea why `kill` does not work on some. It is ok as long as it works on at least one.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869261700)
Hmm, right. Then I have no idea why `kill` does not work on some. It is ok as long as it works on at least one.
✅ fanquake closed an issue: "fuzz, parse_iso8601: attempt to dereference an end-of-stream istreambuf_iterator"
(https://github.com/bitcoin/bitcoin/issues/28917)
(https://github.com/bitcoin/bitcoin/issues/28917)
🚀 fanquake merged a pull request: "util: Drop boost posix_time in ParseISO8601DateTime"
(https://github.com/bitcoin/bitcoin/pull/31391)
(https://github.com/bitcoin/bitcoin/pull/31391)
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517101949)
> > CMake picks the Python from .venv
>
> I'm not using a `.venv`. It's Python installed with `pyenv`.
My apologies.
Adding `-DPython3_FIND_FRAMEWORK=LAST` should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517101949)
> > CMake picks the Python from .venv
>
> I'm not using a `.venv`. It's Python installed with `pyenv`.
My apologies.
Adding `-DPython3_FIND_FRAMEWORK=LAST` should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517114493)
> Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517114493)
> Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517149116)
> > Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
>
> Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
The behaviour is extensively documented:
1. From the [`FindPython3`](https://cmake.org/cmake/help/latest/module/FindPython3.html) module docs:
> On
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517149116)
> > Adding -DPython3_FIND_FRAMEWORK=LAST should [work](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints).
>
> Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version?
The behaviour is extensively documented:
1. From the [`FindPython3`](https://cmake.org/cmake/help/latest/module/FindPython3.html) module docs:
> On
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869367232)
Ignoring the return status of `tcpdump` here is deliberate because some VMs, the ones that couldn't stop `tcpdump` failed to read the unflushed file with an error like "tcpdump: truncated dump file; tried to read 4 file header bytes, only got 0". Now they don't due to the added `-U`, hmm...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869367232)
Ignoring the return status of `tcpdump` here is deliberate because some VMs, the ones that couldn't stop `tcpdump` failed to read the unflushed file with an error like "tcpdump: truncated dump file; tried to read 4 file header bytes, only got 0". Now they don't due to the added `-U`, hmm...
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517179160)
> The behaviour is extensively documented:
Sure, but none of that means anything to a (new) developer, who doesn't care about the intricacies of the build system. It's just unintuituve that CMake would pick some version of Python, other than the one their shell/sytem is (globally) configured to use.
We even suggest using `pyenv` in our [documentation](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines), so it makes even less sense that this workflow w
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517179160)
> The behaviour is extensively documented:
Sure, but none of that means anything to a (new) developer, who doesn't care about the intricacies of the build system. It's just unintuituve that CMake would pick some version of Python, other than the one their shell/sytem is (globally) configured to use.
We even suggest using `pyenv` in our [documentation](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines), so it makes even less sense that this workflow w
...
🤔 l0rinc reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2478197206)
I find a lot of negations very confusing, but I guess it's existing functionality, so we might as well embrace it
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2478197206)
I find a lot of negations very confusing, but I guess it's existing functionality, so we might as well embrace it
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869266813)
I'm not sure I understand how `-noversion` is supposed to work.
I tried changing `test/functional/feature_help.py` to:
```python
def run_test(self):
self.log.info("Start bitcoin with -h for help text")
self.nodes[0].start(extra_args=['-h', '-noversion']) # added -noversion
# Node should exit immediately and output help to stdout.
output, _ = self.get_node_output(ret_code_expected=0)
assert b'Options' in output
assert b'version' not i
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869266813)
I'm not sure I understand how `-noversion` is supposed to work.
I tried changing `test/functional/feature_help.py` to:
```python
def run_test(self):
self.log.info("Start bitcoin with -h for help text")
self.nodes[0].start(extra_args=['-h', '-noversion']) # added -noversion
# Node should exit immediately and output help to stdout.
output, _ = self.get_node_output(ret_code_expected=0)
assert b'Options' in output
assert b'version' not i
...
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869307947)
nit: `len(debug_logs) > 0 && len(debug_logs) < 2` is basically `len(debug_logs) == 1`.
Which could likely be consolidated to a switch, something like:
```python
debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
match len(debug_logs):
case 0: chain = 'regtest'
case 1: chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
case _: raise AssertionError(f"Max one debug.log is supported, found several:\n\t" + "\n\t".jo
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869307947)
nit: `len(debug_logs) > 0 && len(debug_logs) < 2` is basically `len(debug_logs) == 1`.
Which could likely be consolidated to a switch, something like:
```python
debug_logs = list(pathlib.Path(tmp_dir).glob('node0/**/debug.log'))
match len(debug_logs):
case 0: chain = 'regtest'
case 1: chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
case _: raise AssertionError(f"Max one debug.log is supported, found several:\n\t" + "\n\t".jo
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869409094)
I'd say this is unrelated to flushing. Ignoring the return code in all tasks can mean that even the tasks where the detection is working are going to silently break tomorrow. As mentioned previously in the thread you closed, I think this is brittle.
To clarify, my feedback not just applies to the line I commented on, but it applies to all code where the return code is silently ignored and where it could lead to silent breakage.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869409094)
I'd say this is unrelated to flushing. Ignoring the return code in all tasks can mean that even the tasks where the detection is working are going to silently break tomorrow. As mentioned previously in the thread you closed, I think this is brittle.
To clarify, my feedback not just applies to the line I commented on, but it applies to all code where the return code is silently ignored and where it could lead to silent breakage.
🤔 glozow reviewed a pull request: "test: orphan parent is re-requested from 2nd peer"
(https://github.com/bitcoin/bitcoin/pull/31414#pullrequestreview-2478457174)
lgtm ACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5
(https://github.com/bitcoin/bitcoin/pull/31414#pullrequestreview-2478457174)
lgtm ACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5
💬 maflcko commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517261111)
Does any of this matter in practise for running the tests? The tests should be passing for any supported python version, as long as it is above the minimum, which is what cmake already checks for. Also, the tests can be run with a python version explicitly, like `python3.NN ./bld-cmake/test/functional/test_runner.py`.
Though, even for writing tests in python, the version should mostly be irrelevant now that python3 has matured for the commonly used batteries. Historically, one could accidenta
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517261111)
Does any of this matter in practise for running the tests? The tests should be passing for any supported python version, as long as it is above the minimum, which is what cmake already checks for. Also, the tests can be run with a python version explicitly, like `python3.NN ./bld-cmake/test/functional/test_runner.py`.
Though, even for writing tests in python, the version should mostly be irrelevant now that python3 has matured for the commonly used batteries. Historically, one could accidenta
...
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517288426)
> Does any of this matter in practise for running the tests?
If CMake picks a different Python to the one you expect it to use, which is not the one you've `pip` installed any requirements into, i.e `pyzmq`, aren't you now (likely unknowningly) skipping tests that otherwise should be run.
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2517288426)
> Does any of this matter in practise for running the tests?
If CMake picks a different Python to the one you expect it to use, which is not the one you've `pip` installed any requirements into, i.e `pyzmq`, aren't you now (likely unknowningly) skipping tests that otherwise should be run.
💬 edilmedeiros commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2517350643)
I believe everyone running multiple signets will have a workaround for this. Mine is to maintain different `datadir`s around which, in practice, is pretty much the same of what's being proposed here.
A different naming scheme for signet actually makes sense since there's no signet (as in mainnet), but potentially infinite signets serving different purposes. I can't see how this being automatically handled by the tool instead of exposing the problem to the user would be called a mess.
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2517350643)
I believe everyone running multiple signets will have a workaround for this. Mine is to maintain different `datadir`s around which, in practice, is pretty much the same of what's being proposed here.
A different naming scheme for signet actually makes sense since there's no signet (as in mainnet), but potentially infinite signets serving different purposes. I can't see how this being automatically handled by the tool instead of exposing the problem to the user would be called a mess.
📝 maflcko opened a pull request: "test: Avoid F541 (f-string without any placeholders)"
(https://github.com/bitcoin/bitcoin/pull/31417)
An extra `f` string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.
Try to avoid the confusion and mistakes by applying the `F541` linter rule.
(https://github.com/bitcoin/bitcoin/pull/31417)
An extra `f` string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.
Try to avoid the confusion and mistakes by applying the `F541` linter rule.
🚀 fanquake merged a pull request: "[28.x] Backports & 28.1rc1"
(https://github.com/bitcoin/bitcoin/pull/31104)
(https://github.com/bitcoin/bitcoin/pull/31104)
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869542423)
Ok, I changed this to fail if `tcpdump` gives error when it reads the file.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869542423)
Ok, I changed this to fail if `tcpdump` gives error when it reads the file.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2517450862)
`feabffd808...925ee8707e`: (hopefully) address https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2517450862)
`feabffd808...925ee8707e`: (hopefully) address https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837