💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2684920983)
Looks like this has 3 acks so should be good to merge after the 29 release branch is created (https://github.com/bitcoin/bitcoin/issues/31029)
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2684920983)
Looks like this has 3 acks so should be good to merge after the 29 release branch is created (https://github.com/bitcoin/bitcoin/issues/31029)
💬 maflcko commented on issue "ci: Intermittent failure "Could not resolve host: github.com"":
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2684924594)
Likely unrelated, but on one of my machines, another issue reproduces *reliably*. Though, only on that machine, and only under aarch64 qemu, and only for `ubuntu:24.04`:
```
$ podman run --rm --platform=linux/arm64 'mirror.gcr.io/ubuntu:24.04' bash -c 'apt update && apt install curl -y && curl -v --location --fail --remote-name https://github.com/bitcoin-core/qa-assets/raw/main/unit_test_data/script_assets_test.json' # (fails as of Feb 26 2025)
...
* SSL connection using TLSv1.3 / TLS_AES
...
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2684924594)
Likely unrelated, but on one of my machines, another issue reproduces *reliably*. Though, only on that machine, and only under aarch64 qemu, and only for `ubuntu:24.04`:
```
$ podman run --rm --platform=linux/arm64 'mirror.gcr.io/ubuntu:24.04' bash -c 'apt update && apt install curl -y && curl -v --location --fail --remote-name https://github.com/bitcoin-core/qa-assets/raw/main/unit_test_data/script_assets_test.json' # (fails as of Feb 26 2025)
...
* SSL connection using TLSv1.3 / TLS_AES
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684932467)
> All of them returned "The coverage was not deterministic between runs.". I noticed that the reason is basically locks and crypto functions as expected.
I'd say that non-determinism shouldn't be expected, though fixing them can be done in a follow-up. If you want to check a deterministic test case you can try `util_string_tests` or `util_tests`.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684932467)
> All of them returned "The coverage was not deterministic between runs.". I noticed that the reason is basically locks and crypto functions as expected.
I'd say that non-determinism shouldn't be expected, though fixing them can be done in a follow-up. If you want to check a deterministic test case you can try `util_string_tests` or `util_tests`.
💬 brunoerg commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684942423)
> If you want to check a deterministic test case you can try util_string_tests or util_tests.
Both returned "The coverage was not deterministic between runs.". One of the differences:
```sh
/Users/brunogarcia/projects/bitcoin-core-dev/src/crypto/sha512.h:
1| |// Copyright (c) 2014-2022 The Bitcoin Core developers
@@ -138933,7 +138933,7 @@
62| 0|}
63| |
64| |inline int64_t GetPerformanceCounter() noexcept
- 65| 958|{
+ 65| 966|{
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684942423)
> If you want to check a deterministic test case you can try util_string_tests or util_tests.
Both returned "The coverage was not deterministic between runs.". One of the differences:
```sh
/Users/brunogarcia/projects/bitcoin-core-dev/src/crypto/sha512.h:
1| |// Copyright (c) 2014-2022 The Bitcoin Core developers
@@ -138933,7 +138933,7 @@
62| 0|}
63| |
64| |inline int64_t GetPerformanceCounter() noexcept
- 65| 958|{
+ 65| 966|{
...
💬 maflcko commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2684952596)
Looks like CI failed here in https://cirrus-ci.com/task/5918923641585664:
`ConnectionResetError: [Errno 104] Connection reset by peer` when calling `v16_3_node.submitblock(b)`. However the process (node 1) seems to be alive and later killed `[node 1] Cleaning up leftover process`.
Probably one of the old RPC server bugs fixed in non-EOL versions.
Going to re-run CI.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2684952596)
Looks like CI failed here in https://cirrus-ci.com/task/5918923641585664:
`ConnectionResetError: [Errno 104] Connection reset by peer` when calling `v16_3_node.submitblock(b)`. However the process (node 1) seems to be alive and later killed `[node 1] Cleaning up leftover process`.
Probably one of the old RPC server bugs fixed in non-EOL versions.
Going to re-run CI.
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684966702)
> Both returned "The coverage was not deterministic between runs.". One of the differences:
The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2684966702)
> Both returned "The coverage was not deterministic between runs.". One of the differences:
The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2684996658)
Force-push: addressed @hodlinator 's comments.
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2684996658)
Force-push: addressed @hodlinator 's comments.
💬 ryanofsky commented on pull request "doc: Update translation generation instructions":
(https://github.com/bitcoin/bitcoin/pull/31930#issuecomment-2685000131)
Reviewing ba3bb09a12d77b467aec8833e316ee4b5e45ee75
IMO, it would be simpler and more consistent to just update instructions like:
```diff
-cmake --preset dev-mode -DWITH_USDT=OFF
+cmake --preset dev-mode -DWITH_USDT=OFF -DWITH_MULTIPROCESS=OFF
```
It is true that files in `src/ipc/` may contained translated `_()` strings in the future, but I don't think they do right now. In the longer term, the suggestion to use depends should not be needed, because #31741 should allow easily buildi
...
(https://github.com/bitcoin/bitcoin/pull/31930#issuecomment-2685000131)
Reviewing ba3bb09a12d77b467aec8833e316ee4b5e45ee75
IMO, it would be simpler and more consistent to just update instructions like:
```diff
-cmake --preset dev-mode -DWITH_USDT=OFF
+cmake --preset dev-mode -DWITH_USDT=OFF -DWITH_MULTIPROCESS=OFF
```
It is true that files in `src/ipc/` may contained translated `_()` strings in the future, but I don't think they do right now. In the longer term, the suggestion to use depends should not be needed, because #31741 should allow easily buildi
...
💬 brunoerg commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2685008640)
> The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.
Yes, I just verified it is the right commit, I re-built it again and I'm getting the same. Anyway, I'm fine with the script, we can address it in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2685008640)
> The last commit is supposed to fix that and it seemingly did for hodlinator and myself. Are you sure you compiled the right commit? Though, even if it persists, my recommendation would be to address it in a follow-up. The goal here is mostly to add the contrib script itself.
Yes, I just verified it is the right commit, I re-built it again and I'm getting the same. Anyway, I'm fine with the script, we can address it in a follow-up.
👍 ryanofsky approved a pull request: "build: align debugging flags to `-O0`"
(https://github.com/bitcoin/bitcoin/pull/29796#pullrequestreview-2644592785)
Code review ACK 07907e7ef800bcb6b59d1f934c2b75280cf00141
Would be good to update the PR description to match current state of the PR and how it is changing depends debug flags from -O1 to -O0. Could also update the "I'm not completely sure yet what the right choice is" part to say what the problems were with other alternatives tried here.
(https://github.com/bitcoin/bitcoin/pull/29796#pullrequestreview-2644592785)
Code review ACK 07907e7ef800bcb6b59d1f934c2b75280cf00141
Would be good to update the PR description to match current state of the PR and how it is changing depends debug flags from -O1 to -O0. Could also update the "I'm not completely sure yet what the right choice is" part to say what the problems were with other alternatives tried here.
💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685123873)
> > Seems good, just want to to point out that the UI claims to be showing settings from the command line, when this one could be coming from bitcoin.conf.
>
> Yes, that's also a potential source of confusion. Makes it a bit questionable thing to do.
I think most GUI users don't start from the command line. And most users in general don't care about the distinction between `bitcoin.conf` and command line arguments. I'm hoping this message triggers their memory of the last time they edited
...
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685123873)
> > Seems good, just want to to point out that the UI claims to be showing settings from the command line, when this one could be coming from bitcoin.conf.
>
> Yes, that's also a potential source of confusion. Makes it a bit questionable thing to do.
I think most GUI users don't start from the command line. And most users in general don't care about the distinction between `bitcoin.conf` and command line arguments. I'm hoping this message triggers their memory of the last time they edited
...
👋 dergoegge's pull request is ready for review: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/31367)
(https://github.com/bitcoin/bitcoin/pull/31367)
✅ dergoegge closed a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/31367)
(https://github.com/bitcoin/bitcoin/pull/31367)
👍 ryanofsky approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2644640989)
Code review ACK 568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
I just
...
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2644640989)
Code review ACK 568fcdddaec2cc8decba5a098257f31729cc1caa. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
I just
...
💬 maflcko commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2685140630)
Not sure about this. Going from O1 to O0 in the CI will make the CI tasks a lot slower. For example the multiprocess task is now taking almost two hours https://cirrus-ci.com/task/6440292238229504, whereas it never was that slow in the whole history of CI: https://0xb10c.github.io/bitcoin-core-ci-stats/graph/execution/#multiprocess,%20i686,%20DEBUG (the uppper outliers are this pull request)
I understand that O0 can be used to detect more bugs, but when it comes to the CI, we have to realize
...
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2685140630)
Not sure about this. Going from O1 to O0 in the CI will make the CI tasks a lot slower. For example the multiprocess task is now taking almost two hours https://cirrus-ci.com/task/6440292238229504, whereas it never was that slow in the whole history of CI: https://0xb10c.github.io/bitcoin-core-ci-stats/graph/execution/#multiprocess,%20i686,%20DEBUG (the uppper outliers are this pull request)
I understand that O0 can be used to detect more bugs, but when it comes to the CI, we have to realize
...
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2685144957)
> forces re-calculation later if needed
It wouldn't. We can introduce `total_fees` and set it in the same place where we now set this dummy value.
The recalculation in #31283 is done in order to avoid relying on this code. I considered adding `total_fees` for that PR, but it doesn't seem useful because I intend to replace that code when cluster mempool comes around.
> other feature I haven't PR'd yet
That would justify a `total_fees` field. I'm happy to refactor `waitNext()` after sa
...
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2685144957)
> forces re-calculation later if needed
It wouldn't. We can introduce `total_fees` and set it in the same place where we now set this dummy value.
The recalculation in #31283 is done in order to avoid relying on this code. I considered adding `total_fees` for that PR, but it doesn't seem useful because I intend to replace that code when cluster mempool comes around.
> other feature I haven't PR'd yet
That would justify a `total_fees` field. I'm happy to refactor `waitNext()` after sa
...
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685149599)
> And most users in general don't care about the distinction between bitcoin.conf and command line arguments
Agree, but if the dialog mentions something specifically it would be more or less a lie.
> Another, more correct, solution could be to trigger an info / warning alert, either as a popup (annoying) or inline (like how we warn about testnet coins not having value).
We just switched away from the popup here, we're not bringing it back.
A new inline alert in the options dialog wou
...
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685149599)
> And most users in general don't care about the distinction between bitcoin.conf and command line arguments
Agree, but if the dialog mentions something specifically it would be more or less a lie.
> Another, more correct, solution could be to trigger an info / warning alert, either as a popup (annoying) or inline (like how we warn about testnet coins not having value).
We just switched away from the popup here, we're not bringing it back.
A new inline alert in the options dialog wou
...
👍 ryanofsky approved a pull request: "init: Handle dropped UPnP support more gracefully"
(https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2644665716)
Code review ACK 44041ae0eca9d2034b7c2bdef24724809cc35e90. Code changes look good. Could potentially add test coverage for this, though I don't think it is too important.
(https://github.com/bitcoin/bitcoin/pull/31916#pullrequestreview-2644665716)
Code review ACK 44041ae0eca9d2034b7c2bdef24724809cc35e90. Code changes look good. Could potentially add test coverage for this, though I don't think it is too important.
🤔 i-am-yuvi reviewed a pull request: "Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/31492#pullrequestreview-2644678405)
re-ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06
- Tested all the changes
(https://github.com/bitcoin/bitcoin/pull/31492#pullrequestreview-2644678405)
re-ACK 1561575c2d984fd94c9679de3d1de207ef6c7c06
- Tested all the changes
💬 ryanofsky commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685172480)
I'm a little confused by discussion above about options being overridden on the command line or configuration file, but maybe I need to think about it more. In general options being specified on command line should override GUI settings, but options specified in the GUI should override options specified in the config file.
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685172480)
I'm a little confused by discussion above about options being overridden on the command line or configuration file, but maybe I need to think about it more. In general options being specified on command line should override GUI settings, but options specified in the GUI should override options specified in the config file.