💬 carnhofdaki commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2516525566)
Concept NACK.
I use a simple script setting datadir according to the current working directory. If the change would be backward-compatible I don't care. But running multiple custom signets and never needed to make a mess. The proposed naming convention (`signet_XXXXXXX`) seems like a mess to me.
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2516525566)
Concept NACK.
I use a simple script setting datadir according to the current working directory. If the change would be backward-compatible I don't care. But running multiple custom signets and never needed to make a mess. The proposed naming convention (`signet_XXXXXXX`) seems like a mess to me.
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869004667)
I was wondering how it will behave with `%c` which would add a `\0` which could end the const char* prematurely, but since we have a string it seems to work correctly.
Are there any checks we could do with the result here? Or make `TfmFormatZeroes` void if we don't need the actual result. Maybe something like this:
```suggestion
auto result{TfmFormatZeroes<NumArgs>(fmt.fmt)};
BOOST_CHECK_EQUAL(result.empty(), std::string_view{fmt.fmt}.empty());
```
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869004667)
I was wondering how it will behave with `%c` which would add a `\0` which could end the const char* prematurely, but since we have a string it seems to work correctly.
Are there any checks we could do with the result here? Or make `TfmFormatZeroes` void if we don't need the actual result. Maybe something like this:
```suggestion
auto result{TfmFormatZeroes<NumArgs>(fmt.fmt)};
BOOST_CHECK_EQUAL(result.empty(), std::string_view{fmt.fmt}.empty());
```
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869005752)
Is it coincidental that every single format type accepts `0` as a valid substitution?
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1869005752)
Is it coincidental that every single format type accepts `0` as a valid substitution?
💬 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