💬 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
💬 hebasto commented on issue "macOS 13.7 depends build can't find qt (symlink)":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2517454703)
@Sjors
I've managed to reproduce the issue.
I believe https://github.com/bitcoin/bitcoin/pull/31358 should resolve it. Could you please verify the fix on your system?
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2517454703)
@Sjors
I've managed to reproduce the issue.
I believe https://github.com/bitcoin/bitcoin/pull/31358 should resolve it. Could you please verify the fix on your system?
💬 hebasto commented on pull request "depends, refactor: Avoid hardcoding `host_prefix` in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2517459170)
> Is there any particular motivation (to do this now), given it doesn't work/can't be tested?
Apparently, this PR fixes https://github.com/bitcoin/bitcoin/issues/31050.
(https://github.com/bitcoin/bitcoin/pull/31358#issuecomment-2517459170)
> Is there any particular motivation (to do this now), given it doesn't work/can't be tested?
Apparently, this PR fixes https://github.com/bitcoin/bitcoin/issues/31050.
💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r1869569271)
added
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r1869569271)
added
💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-2517544785)
Rebased after #31096 and took @instagibbs' comment, thank you
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-2517544785)
Rebased after #31096 and took @instagibbs' comment, thank you
👋 glozow's pull request is ready for review: "package validation: relax the package-not-child-with-unconfirmed-parents rule"
(https://github.com/bitcoin/bitcoin/pull/31385)
(https://github.com/bitcoin/bitcoin/pull/31385)
👍 hodlinator approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2477648972)
ACK f4df783f1ca22d96476d52ec5d1929547691ba13
Increased validation of format strings. :+1:
---
Maybe amend PR summary or final commit by spelling out something like:
Delays `bilingual_str`-construction until the end of the overload of `tinyformat::format` in *translation.h*. The `fmt` parameter to the overload is changed from `bilingual_str` -> `util::bilingual_fmt`, the latter being a new type which contains a `ConstevalFormatString`. `bilingual_fmt` in turn is only constructible fr
...
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2477648972)
ACK f4df783f1ca22d96476d52ec5d1929547691ba13
Increased validation of format strings. :+1:
---
Maybe amend PR summary or final commit by spelling out something like:
Delays `bilingual_str`-construction until the end of the overload of `tinyformat::format` in *translation.h*. The `fmt` parameter to the overload is changed from `bilingual_str` -> `util::bilingual_fmt`, the latter being a new type which contains a `ConstevalFormatString`. `bilingual_fmt` in turn is only constructible fr
...
💬 hodlinator commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1869532227)
nit: Can't we just be explicit here in the test to document what is happening, since it is slightly tricky?
```suggestion
constexpr util::Translatable<false> format{Untranslated("original [%s]")};
```
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1869532227)
nit: Can't we just be explicit here in the test to document what is happening, since it is slightly tricky?
```suggestion
constexpr util::Translatable<false> format{Untranslated("original [%s]")};
```
💬 hodlinator commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1868949895)
nit:
```suggestion
std::string translate() const { return translatable ? util::translate(original.fmt) : original.fmt; }
```
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1868949895)
nit:
```suggestion
std::string translate() const { return translatable ? util::translate(original.fmt) : original.fmt; }
```
⚠️ fanquake opened an issue: "test: failure in `interface_usdt_net.py`"
(https://github.com/bitcoin/bitcoin/issues/31418)
Master @ ae69fc37e4fff237a119225061d68f69e6cd61d7.
Running on Rawhide.
GCC: `gcc (GCC) 14.2.1 20241104 (Red Hat 14.2.1-6)`.
Systemtap: `systemtap-5.2-1.fc42.src.rpm`
```bash
cmake -B build -DWITH_USDT=ON
cmake --build build -j17
./build/test/functional/test_runner.py --jobs=17
```
```bash
123/314 - interface_usdt_net.py failed, Duration: 3 s
stdout:
2024-12-04T14:13:14.071000Z TestFramework (INFO): PRNG seed is: 2457885293535030371
2024-12-04T14:13:14.072000Z TestFramework (IN
...
(https://github.com/bitcoin/bitcoin/issues/31418)
Master @ ae69fc37e4fff237a119225061d68f69e6cd61d7.
Running on Rawhide.
GCC: `gcc (GCC) 14.2.1 20241104 (Red Hat 14.2.1-6)`.
Systemtap: `systemtap-5.2-1.fc42.src.rpm`
```bash
cmake -B build -DWITH_USDT=ON
cmake --build build -j17
./build/test/functional/test_runner.py --jobs=17
```
```bash
123/314 - interface_usdt_net.py failed, Duration: 3 s
stdout:
2024-12-04T14:13:14.071000Z TestFramework (INFO): PRNG seed is: 2457885293535030371
2024-12-04T14:13:14.072000Z TestFramework (IN
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869619567)
The version number is always going to be displayed whether you run `bitcoind` with `-version` `-help` or no arguments at all, because it is logged at startup. If you don't want to see the version number you need to run `bitcoind` with `-noprinttoconsole` or `-daemon` to not log to the console.
The point of `-version` is to show the version number and copyright information and exit, and the point of `-noversion` is to do the opposite and start up normally. You can think of `-version` as being
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869619567)
The version number is always going to be displayed whether you run `bitcoind` with `-version` `-help` or no arguments at all, because it is logged at startup. If you don't want to see the version number you need to run `bitcoind` with `-noprinttoconsole` or `-daemon` to not log to the console.
The point of `-version` is to show the version number and copyright information and exit, and the point of `-noversion` is to do the opposite and start up normally. You can think of `-version` as being
...
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869626885)
I'm not yet sure this makes the behavior clearer for the version parameter, but the rest seems fine to me, thanks for clarifying.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869626885)
I'm not yet sure this makes the behavior clearer for the version parameter, but the rest seems fine to me, thanks for clarifying.