💬 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.
💬 laanwj commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651183942)
re-ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i could test it on amazon graviton again like i did the original PR, but only the good-weather case, that won't reproduce the edge case that led to this.
Would be good if @giahuy98 (who opened #31817) could test it on the specific target hardware.
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2651183942)
re-ACK a2ae62fb4db5e014c03a7ad41ba94ca49fd3806e
i could test it on amazon graviton again like i did the original PR, but only the good-weather case, that won't reproduce the edge case that led to this.
Would be good if @giahuy98 (who opened #31817) could test it on the specific target hardware.
💬 instagibbs commented on pull request "test: add missing sync to p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951081533)
seems to cause quite a few nuisance issues in tests, may be worth it going forward
(https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1951081533)
seems to cause quite a few nuisance issues in tests, may be worth it going forward
💬 furszy commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951082137)
That's a more delicate topic. `InferScript` can return `nullptr` in valid scenarios too. It shouldn't just fail in every case as you're doing there.
For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
We need a different way to signal errors from inner `InferScript()` calls. Could be a boolean or.. a different return object like a
...
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951082137)
That's a more delicate topic. `InferScript` can return `nullptr` in valid scenarios too. It shouldn't just fail in every case as you're doing there.
For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a `addr()`. But with your changes, it would return `nullptr instead.
We need a different way to signal errors from inner `InferScript()` calls. Could be a boolean or.. a different return object like a
...
💬 sipa commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951097542)
Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
(https://github.com/bitcoin/bitcoin/pull/31404#discussion_r1951097542)
Rather than making the RPC output field optional, can we not fallback to always returning a `raw` / `addr` descriptor if inferring returns something that would be invalid?
💬 ryanofsky commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2651239390)
> I actually implemented this in [#25297](https://github.com/bitcoin/bitcoin/pull/25297), three years ago :). Will see how much work is needed to revive it next week.
@furszy since I mentioned it the other day, this is a link to the [`PtrOrValue`](https://github.com/ryanofsky/libmultiprocess/blob/ce4814f46d3c38d0bdd08df2253bc69b084f22ee/include/mp/util.h#L136-L148) utility I used in libmultiprocess to allow functions to lock internally or optionally accept an external lock, without requiring tw
...
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2651239390)
> I actually implemented this in [#25297](https://github.com/bitcoin/bitcoin/pull/25297), three years ago :). Will see how much work is needed to revive it next week.
@furszy since I mentioned it the other day, this is a link to the [`PtrOrValue`](https://github.com/ryanofsky/libmultiprocess/blob/ce4814f46d3c38d0bdd08df2253bc69b084f22ee/include/mp/util.h#L136-L148) utility I used in libmultiprocess to allow functions to lock internally or optionally accept an external lock, without requiring tw
...
💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2651275762)
> With those parameters though, BnB should be producing change.
Take a look at coinselection fuzz target. See that regardless the parameters/coins/etc we assert that BnB does not produce change (this is the expected for this algorithm).
```cpp
// Run coinselection algorithms
auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} :
SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight
...
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2651275762)
> With those parameters though, BnB should be producing change.
Take a look at coinselection fuzz target. See that regardless the parameters/coins/etc we assert that BnB does not produce change (this is the expected for this algorithm).
```cpp
// Run coinselection algorithms
auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} :
SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight
...