💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869230414)
Then it would be unclear whether the traffic was generated by unit tests, functional tests or fuzz tests. So I start/stop a separate monitoring for each of those set of tests. I perceived this information useful when investigating. It is already tough that it is not known which test generated the traffic.
Somehow I do not like reading from the "live" file while it is being written. `-U` would help not to miss data, but it might happen that by the time we read it the last packet is still being
...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869230414)
Then it would be unclear whether the traffic was generated by unit tests, functional tests or fuzz tests. So I start/stop a separate monitoring for each of those set of tests. I perceived this information useful when investigating. It is already tough that it is not known which test generated the traffic.
Somehow I do not like reading from the "live" file while it is being written. `-U` would help not to miss data, but it might happen that by the time we read it the last packet is still being
...
👍 danielabrozzoni approved a pull request: "refactor: Move GuessVerificationProgress into ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2478006074)
re-ACK 5555ed0c3b8d1beecb2785dc8cbe10c1693ca290
(https://github.com/bitcoin/bitcoin/pull/31393#pullrequestreview-2478006074)
re-ACK 5555ed0c3b8d1beecb2785dc8cbe10c1693ca290
💬 danielabrozzoni commented on pull request "refactor: Move GuessVerificationProgress into ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869153515)
nit: you also deleted a whitespace here, and now the line is indented with only 3 whitespaces, is this on purpose?
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869153515)
nit: you also deleted a whitespace here, and now the line is indented with only 3 whitespaces, is this on purpose?
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869235192)
> > Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
>
> That is interesting, because all tasks should be running on workers that are set up identically.
I noticed those tasks print:
> Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869235192)
> > Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
>
> That is interesting, because all tasks should be running on workers that are set up identically.
I noticed those tasks print:
> Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
💬 maflcko commented on pull request "refactor: Move GuessVerificationProgress into ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869236012)
It is done by clang-format
(https://github.com/bitcoin/bitcoin/pull/31393#discussion_r1869236012)
It is done by clang-format
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869246798)
> > That is interesting, because all tasks should be running on workers that are set up identically.
>
> I noticed those tasks print:
>
> > Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
That should be identical for all tasks as well. See:
* https://cirrus-ci.com/task/5454797781860352?logs=ci#L186
* https://cirrus-ci.com/task/5314060293505024?logs=ci#L184
The only difference should be that one is bare metal and the other is KVM, but I don't see how
...
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869246798)
> > That is interesting, because all tasks should be running on workers that are set up identically.
>
> I noticed those tasks print:
>
> > Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg
That should be identical for all tasks as well. See:
* https://cirrus-ci.com/task/5454797781860352?logs=ci#L186
* https://cirrus-ci.com/task/5314060293505024?logs=ci#L184
The only difference should be that one is bare metal and the other is KVM, but I don't see how
...
💬 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.