💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1868990468)
👍 for checking `%n` as well!
nit: we might inline `0` here:
```suggestion
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, 0), tfm::format_error,
```
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1868990468)
👍 for checking `%n` as well!
nit: we might inline `0` here:
```suggestion
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, 0), tfm::format_error,
```
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1869077815)
Thanks! Done.
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1869077815)
Thanks! Done.
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547)
> Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
Apparently not. Here's another case where CMake just picks some other Python:
```bash
# python --version
Python 3.13.1
# python3 --version
Python 3.13.1
# which python
/Users/xxx/.pyenv/shims/python
# cmake -B build
<snip>
-- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpr
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547)
> Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
Apparently not. Here's another case where CMake just picks some other Python:
```bash
# python --version
Python 3.13.1
# python3 --version
Python 3.13.1
# which python
/Users/xxx/.pyenv/shims/python
# cmake -B build
<snip>
-- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpr
...
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869162815)
Even better! Added `-U`.
With `-U` is does not matter anymore, but I think it was ok before as well, see my comment [below](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867509291).
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869162815)
Even better! Added `-U`.
With `-U` is does not matter anymore, but I think it was ok before as well, see my comment [below](https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1867509291).
📝 maflcko opened a pull request: " doc: Fix incorrect send RPC docs "
(https://github.com/bitcoin/bitcoin/pull/31416)
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue and also creates a special dedicated type for an import timestamp.
(https://github.com/bitcoin/bitcoin/pull/31416)
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue and also creates a special dedicated type for an import timestamp.
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869187689)
To 1. flush the file and 2. so that the next set of tests can start fresh without leftover `tcpdump` processes. Now 1. is not needed because of `-U`, but I will leave it for 2.
In my experiments `SIGTERM` would cause `tcpdump` to exit gracefully - flush the file and then exit. `SIGKILL` would result in unflushed file.
Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869187689)
To 1. flush the file and 2. so that the next set of tests can start fresh without leftover `tcpdump` processes. Now 1. is not needed because of `-U`, but I will leave it for 2.
In my experiments `SIGTERM` would cause `tcpdump` to exit gracefully - flush the file and then exit. `SIGKILL` would result in unflushed file.
Yes, in some tasks `kill` gives "permission denied" even if docker is running with `--privileged` or `--cap-add ALL`.
💬 hebasto commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516927066)
> > Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
>
> Apparently not. Here's another case where CMake just picks some other Python:
>
> ```shell
> # python --version
> Python 3.13.1
> # python3 --version
> Python 3.13.1
> # which python
> /Users/xxx/.pyenv/shims/python
> # cmake -B build
> <snip>
> -- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516927066)
> > Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?
>
> Apparently not. Here's another case where CMake just picks some other Python:
>
> ```shell
> # python --version
> Python 3.13.1
> # python3 --version
> Python 3.13.1
> # which python
> /Users/xxx/.pyenv/shims/python
> # cmake -B build
> <snip>
> -- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869206235)
> 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.
> tests can start fresh without leftover `tcpdump` processes
My thinking was to just have a single process, which is started once and never killed. This should remove the need to kill it, and also the need (and code) to start it every time.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869206235)
> 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.
> tests can start fresh without leftover `tcpdump` processes
My thinking was to just have a single process, which is started once and never killed. This should remove the need to kill it, and also the need (and code) to start it every time.
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837)
This is also ignoring the return code? So it could also lead to CI silently passing on all tasks, when it should not.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869211837)
This is also ignoring the return code? So it could also lead to CI silently passing on all tasks, when it should not.
💬 fanquake commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516945982)
> CMake picks the Python from .venv
I'm not using a `.venv`. It's Python installed with `pyenv`.
💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516958005)
Thanks @hebasto for the double-check. It has helped me work out what was going wrong for me (I think)...
Looks like I had not bumped my python version in the venv since #30527 so I was still running 3.9 in my venv. This caused the `cmake` check to (silently) _skip_ that version during configure and fallback to a "system" version (3.13).
This was then combined with me manually exporting `PYTHONPATH` in this dir (to make `Pyright` perform better in `nvim`), which was causing a right mismatch
...
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516958005)
Thanks @hebasto for the double-check. It has helped me work out what was going wrong for me (I think)...
Looks like I had not bumped my python version in the venv since #30527 so I was still running 3.9 in my venv. This caused the `cmake` check to (silently) _skip_ that version during configure and fallback to a "system" version (3.13).
This was then combined with me manually exporting `PYTHONPATH` in this dir (to make `Pyright` perform better in `nvim`), which was causing a right mismatch
...
💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869221388)
Generally, I am not sure if bash is the right language to implement something like this. Silently ignoring possible errors doesn't seem like a great approach. Other languages, such as Rust or Python are a bit more explicit in documenting when a return code is ignored.
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1869221388)
Generally, I am not sure if bash is the right language to implement something like this. Silently ignoring possible errors doesn't seem like a great approach. Other languages, such as Rust or Python are a bit more explicit in documenting when a return code is ignored.
💬 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)