👍 hodlinator approved a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2608732876)
re-ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2599809742):
- Broke out initial commit which tests prior behavior (with minor improvements).
- Terser test for last occurrence of negated arg suggested by self.
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2608732876)
re-ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1
Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2599809742):
- Broke out initial commit which tests prior behavior (with minor improvements).
- Terser test for last occurrence of negated arg suggested by self.
💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650860541)
> > I want to check if this helps [#30852 (comment)](https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430) when used with #31545 by not having to keep the base images around anymore.
>
> @0xB10C Are you waiting for me to rebase this once more? I've held back, because there is already one review, but I am happy to rebase.
Thanks for the reminder. I played around with this a bit and it does indeed help:
When using https://github.com/bitcoin/bitcoin/pull/31545, the base
...
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650860541)
> > I want to check if this helps [#30852 (comment)](https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2558198430) when used with #31545 by not having to keep the base images around anymore.
>
> @0xB10C Are you waiting for me to rebase this once more? I've held back, because there is already one review, but I am happy to rebase.
Thanks for the reminder. I played around with this a bit and it does indeed help:
When using https://github.com/bitcoin/bitcoin/pull/31545, the base
...
📝 maflcko opened a pull request: "test: Clear scheduler after init to increase fuzz stability"
(https://github.com/bitcoin/bitcoin/pull/31841)
Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:
* Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
* Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.
Fix one such issue.
...
(https://github.com/bitcoin/bitcoin/pull/31841)
Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:
* Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
* Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.
Fix one such issue.
...
💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650867558)
My understanding is that https://github.com/bitcoin/bitcoin/pull/28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650867558)
My understanding is that https://github.com/bitcoin/bitcoin/pull/28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2650868943)
I've created https://github.com/bitcoin/bitcoin/pull/31841 to make it easier to test this on a real patch.
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2650868943)
I've created https://github.com/bitcoin/bitcoin/pull/31841 to make it easier to test this on a real patch.
💬 maflcko commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650891965)
> My understanding is that #28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?
I do not know if they were ever needed to be set inside the CI pod, given that they fall back to the default anyway (see pull description). This may have just been added out of caution, as on the outside the pod, they are required sometimes to be set, see the docs:
https://github.com/bitcoin/bitcoin/blame/86528937e5c4da2e12c46085fc41e87ed75925
...
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650891965)
> My understanding is that #28138 was only needed for `DANGER_RUN_CI_ON_HOST`. That's why removing it here isn't a regression to pre 28138, correct?
I do not know if they were ever needed to be set inside the CI pod, given that they fall back to the default anyway (see pull description). This may have just been added out of caution, as on the outside the pod, they are required sometimes to be set, see the docs:
https://github.com/bitcoin/bitcoin/blame/86528937e5c4da2e12c46085fc41e87ed75925
...
💬 eval-exec commented on pull request "Limit retries in GetRNDRRS to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1950895954)
Thank you, updated.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1950895954)
Thank you, updated.
💬 0xB10C commented on pull request "ci: Skip read-write of default env vars":
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650904601)
Makes sense, thanks.
ACK fa952acdb6ef1b07b7927202d1373fa7479fd5e4
(https://github.com/bitcoin/bitcoin/pull/31678#issuecomment-2650904601)
Makes sense, thanks.
ACK fa952acdb6ef1b07b7927202d1373fa7479fd5e4
💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2650954493)
> > Summarizing #31476, the CentOS CI task has been configuring a build with python `3.9` (the latest version in the Stream 9 core repo) which is below the minimum version of `3.10`
>
> I don't think this is entirely right. appstream ships with 3.11 and 3.12 on centos. (You can install them just like you can install g++-14 on centos 9).
By 'core repo' I meant what I guess CentOS calls the 'BaseOS' repository, fixed in the description.
> > Making this matter a bit worse, we now also [use
...
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2650954493)
> > Summarizing #31476, the CentOS CI task has been configuring a build with python `3.9` (the latest version in the Stream 9 core repo) which is below the minimum version of `3.10`
>
> I don't think this is entirely right. appstream ships with 3.11 and 3.12 on centos. (You can install them just like you can install g++-14 on centos 9).
By 'core repo' I meant what I guess CentOS calls the 'BaseOS' repository, fixed in the description.
> > Making this matter a bit worse, we now also [use
...
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1950980811)
> it's possible to start the node and quickly shut it down. If that happens then `m_tip_block` remains null.
> If you call `createNewBlock()` during that time I'm not sure what happens
The RPC is not yet serving requests when [`m_tip_block` is assigned a value](https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/node/kernel_notifications.cpp#L53-L58), but IPC is started before that. I put a long sleep just before assigning to `m_tip_block` but I am not sure h
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1950980811)
> it's possible to start the node and quickly shut it down. If that happens then `m_tip_block` remains null.
> If you call `createNewBlock()` during that time I'm not sure what happens
The RPC is not yet serving requests when [`m_tip_block` is assigned a value](https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/src/node/kernel_notifications.cpp#L53-L58), but IPC is started before that. I put a long sleep just before assigning to `m_tip_block` but I am not sure h
...
💬 sipa commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951006504)
There is one difference, in that `SeedHardwareFast` only tries to get 64 bits of entropy, while `SeedHardwareSlow` tries to get a full (and fresh) 256 bits, if possible. But without the rndrrs instruction, the difference isn't all that significant (especially as `SeedSlow` also gets 256 bit of entropy from the OS.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951006504)
There is one difference, in that `SeedHardwareFast` only tries to get 64 bits of entropy, while `SeedHardwareSlow` tries to get a full (and fresh) 256 bits, if possible. But without the rndrrs instruction, the difference isn't all that significant (especially as `SeedSlow` also gets 256 bit of entropy from the OS.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1951009074)
> it's possible
In theory, I don't think this is easy to hit in practice. And #31785 should fix it entirely. You could try modifying `bitcoin-mine` in #30437.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1951009074)
> it's possible
In theory, I don't think this is easy to hit in practice. And #31785 should fix it entirely. You could try modifying `bitcoin-mine` in #30437.
⚠️ hebasto opened an issue: "Wallet symlinks are not rejected as expected"
(https://github.com/bitcoin/bitcoin/issues/31842)
Since https://github.com/bitcoin/bitcoin/pull/10885, `bitcoind` is expected to reject symlinks to wallets. This behaviour is also intended to be tested in the following test:https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/test/functional/wallet_multiwallet.py#L168-L170
However, this is not the case:
```
$ ls -l ~/.bitcoin/regtest/wallets/
total 3
drwx------ 2 hebasto hebasto 3 Feb 11 14:49 w8
lrwxrwxrwx 1 hebasto hebasto 41 Feb 11 14:11 w8_symlink_abs -> /home/
...
(https://github.com/bitcoin/bitcoin/issues/31842)
Since https://github.com/bitcoin/bitcoin/pull/10885, `bitcoind` is expected to reject symlinks to wallets. This behaviour is also intended to be tested in the following test:https://github.com/bitcoin/bitcoin/blob/86528937e5c4da2e12c46085fc41e87ed759258e/test/functional/wallet_multiwallet.py#L168-L170
However, this is not the case:
```
$ ls -l ~/.bitcoin/regtest/wallets/
total 3
drwx------ 2 hebasto hebasto 3 Feb 11 14:49 w8
lrwxrwxrwx 1 hebasto hebasto 41 Feb 11 14:11 w8_symlink_abs -> /home/
...
⚠️ fanquake opened an issue: "build: `-static-pie` builds no-longer working with CMake"
(https://github.com/bitcoin/bitcoin/issues/31843)
Using 28.x, it's easy to produce a `-static-pie` binary doing:
```bash
# aarch64 alpine, 28.x (32efe850438ef22e2de39e562af557872a402c31)
make -C depends/ NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 NO_UPNP=1 NO_NATPMP=1 LDFLAGS="-static-pie"
./autogen.sh
CONFIG_SITE=/bitcoin/depends/aarch64-unknown-linux-musl/share/config.site ./configure
make src/bitcoind
# link line
libtool: link: /usr/bin/ccache g++ -std=c++20 <snip> -fPIE <snip> -pie -static-pie -o bitcoind bitcoind-bitcoind.o ....
# file src/bit
...
(https://github.com/bitcoin/bitcoin/issues/31843)
Using 28.x, it's easy to produce a `-static-pie` binary doing:
```bash
# aarch64 alpine, 28.x (32efe850438ef22e2de39e562af557872a402c31)
make -C depends/ NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 NO_UPNP=1 NO_NATPMP=1 LDFLAGS="-static-pie"
./autogen.sh
CONFIG_SITE=/bitcoin/depends/aarch64-unknown-linux-musl/share/config.site ./configure
make src/bitcoind
# link line
libtool: link: /usr/bin/ccache g++ -std=c++20 <snip> -fPIE <snip> -pie -static-pie -o bitcoind bitcoind-bitcoind.o ....
# file src/bit
...
🤔 laanwj reviewed a pull request: "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop"
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2609096562)
Code review ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i do not have RNDR-supporting hardware to test this
(https://github.com/bitcoin/bitcoin/pull/31826#pullrequestreview-2609096562)
Code review ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i do not have RNDR-supporting hardware to test this
💬 sipa commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951059294)
Nit: the entire loop can be skipped here (replace `if (g_rndr_supported) {` with `if (g_rndrrs_support) {`).
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951059294)
Nit: the entire loop can be skipped here (replace `if (g_rndr_supported) {` with `if (g_rndrrs_support) {`).
💬 laanwj commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951061433)
Eh yes, that would be better.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951061433)
Eh yes, that would be better.
💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951069695)
Yes, Updated.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1951069695)
Yes, Updated.
💬 sipa commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651173341)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. I have not tested this.
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651173341)
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. I have not tested this.
💬 mzumsande commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951078538)
didn't get to it before merge.
>not many remaining cases
well, I count ~130 still, so definitely some churn involved changing most of these. Still, it would make sense to me.
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951078538)
didn't get to it before merge.
>not many remaining cases
well, I count ~130 still, so definitely some churn involved changing most of these. Still, it would make sense to me.