💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1716082861)
Ah, missed one, fixed.
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1716082861)
Ah, missed one, fixed.
💬 justinvforvendetta commented on pull request "remove repeated word in note":
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2287375699)
maybe you have a label for such?
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2287375699)
maybe you have a label for such?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716215869)
Agree, it turned out it could be worked around by switching to `BOOST_CHECK_EQUAL_COLLECTIONS` in **key_tests.cpp**.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716215869)
Agree, it turned out it could be worked around by switching to `BOOST_CHECK_EQUAL_COLLECTIONS` in **key_tests.cpp**.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716226929)
This is in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689162345
To summarize - the concern is not stack memory being used inside `ArrayFromBytes` itself, but rather that the `std::array` returned contains an inner C-array (https://github.com/gcc-mirror/gcc/blob/61715e9340ab8941d40d62158fe437e9dbe3e068/libstdc%2B%2B-v3/include/std/array) that is not allocated from the heap and so will bump the stack pointer in the calling function by a fair bit for long hex strings.
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716226929)
This is in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689162345
To summarize - the concern is not stack memory being used inside `ArrayFromBytes` itself, but rather that the `std::array` returned contains an inner C-array (https://github.com/gcc-mirror/gcc/blob/61715e9340ab8941d40d62158fe437e9dbe3e068/libstdc%2B%2B-v3/include/std/array) that is not allocated from the heap and so will bump the stack pointer in the calling function by a fair bit for long hex strings.
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716229145)
I was running into an issue with `CScript` (inheriting `prevector`) and `std::array`s, similar to here: https://gitlab.freedesktop.org/pipewire/media-session/-/issues/4
Reverted this change in the latest push as I'm no longer modifying `CScript`.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716229145)
I was running into an issue with `CScript` (inheriting `prevector`) and `std::array`s, similar to here: https://gitlab.freedesktop.org/pipewire/media-session/-/issues/4
Reverted this change in the latest push as I'm no longer modifying `CScript`.
💬 maflcko commented on pull request "feat(build): improve dependency error reporting in autogen.sh":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2288031311)
Tend toward NACK. No need to apply style fixes to a file that will be deleted in 10 days (or whenever the branch-off happens and the cmake pull is reviewed sufficiently)
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2288031311)
Tend toward NACK. No need to apply style fixes to a file that will be deleted in 10 days (or whenever the branch-off happens and the cmake pull is reviewed sufficiently)
💬 maflcko commented on pull request "doc: Deduplicate list of possible chain strings in RPC help texts":
(https://github.com/bitcoin/bitcoin/pull/30648#issuecomment-2288040823)
lgtm ACK 9b297555207b4ea54bc0051f09c7084797aa9def
(https://github.com/bitcoin/bitcoin/pull/30648#issuecomment-2288040823)
lgtm ACK 9b297555207b4ea54bc0051f09c7084797aa9def
💬 maflcko commented on pull request "remove repeated word in note":
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2288041109)
ACK 3f05a1068d10ffe0f2859cd20c5fc9bc8efa1c70
please add the `fuzz: ` prefix to the pull title.
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2288041109)
ACK 3f05a1068d10ffe0f2859cd20c5fc9bc8efa1c70
please add the `fuzz: ` prefix to the pull title.
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716446494)
I don't like the suggestion, because it makes it harder to change `GetNetworkForMagic` to accept a span in the future. Generally, when a function accepts a read-only non-lifetimebound view of a byte blob, a span can be used. `std::span` supports fixed size and dynamic size, so the prior workarounds to force a fixed-size array no longer apply and it seems odd to rely on them in new code.
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716446494)
I don't like the suggestion, because it makes it harder to change `GetNetworkForMagic` to accept a span in the future. Generally, when a function accepts a read-only non-lifetimebound view of a byte blob, a span can be used. `std::span` supports fixed size and dynamic size, so the prior workarounds to force a fixed-size array no longer apply and it seems odd to rely on them in new code.
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716448092)
Leaving as-is for now to not invalidate review. I'll follow-up with iwyu on all files, I guess.
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716448092)
Leaving as-is for now to not invalidate review. I'll follow-up with iwyu on all files, I guess.
💬 maflcko commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288076458)
> `nproc` is linux only
I don't think this is true. I don't have a mac, but I fail to see why installing it won't work. Also, a bash alias similar to `alias nproc="sysctl -n hw.logicalcpu"` or `alias nproc="getconf _NPROCESSORS_ONLN"` should work as well.
No objection to changing this, but I think the claim that `nproc` is linux-only may not be true.
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288076458)
> `nproc` is linux only
I don't think this is true. I don't have a mac, but I fail to see why installing it won't work. Also, a bash alias similar to `alias nproc="sysctl -n hw.logicalcpu"` or `alias nproc="getconf _NPROCESSORS_ONLN"` should work as well.
No objection to changing this, but I think the claim that `nproc` is linux-only may not be true.
👍 MarnixCroes approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237465538)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237465538)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def
💬 maflcko commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288148372)
> Regarding Dockerfile, I believe it can be considered part of Bitcoin Core Software, as a Bitcoin Core user I need it and I can see through the discussions that many need it, could someone enlighten me of what is needed to start accepting Dockerfile as part of Bitcoin Core Software? What are your requirements?
No one is holding anyone back from opening a pull request to add a container engine imagefile. Not sure how much actual interest is there, given that no one has done so far, or the pre
...
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288148372)
> Regarding Dockerfile, I believe it can be considered part of Bitcoin Core Software, as a Bitcoin Core user I need it and I can see through the discussions that many need it, could someone enlighten me of what is needed to start accepting Dockerfile as part of Bitcoin Core Software? What are your requirements?
No one is holding anyone back from opening a pull request to add a container engine imagefile. Not sure how much actual interest is there, given that no one has done so far, or the pre
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716512192)
The return value is used here:
```cpp
// This is done when we get a PONG from the peer. Repeat here too in case we never receive a PONG.
if (node.IsPrivateBroadcastConn() &&
m_tx_for_private_broadcast.FinishBroadcast(nodeid, /*confirmed_by_node=*/false)) {
...
log a message that we never got a PONG response
```
The point is to avoid printing this log message in the happy path where we do get a PONG response.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716512192)
The return value is used here:
```cpp
// This is done when we get a PONG from the peer. Repeat here too in case we never receive a PONG.
if (node.IsPrivateBroadcastConn() &&
m_tx_for_private_broadcast.FinishBroadcast(nodeid, /*confirmed_by_node=*/false)) {
...
log a message that we never got a PONG response
```
The point is to avoid printing this log message in the happy path where we do get a PONG response.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288154025)
> What is the expected speed up?
Probably depends on the machine, but it should be +30% or so.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288154025)
> What is the expected speed up?
Probably depends on the machine, but it should be +30% or so.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288228196)
I see another 5% in the ECC_Start flame on my machine, which can be avoided, so I'll try to rework the second commit.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288228196)
I see another 5% in the ECC_Start flame on my machine, which can be avoided, so I'll try to rework the second commit.
💬 paplorinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288241884)
Thanks @maflcko, I would actually prefer mentioning nproc (+ alternatives) only once in the docs - will be easier once the cmake PR updates are merged.
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288241884)
Thanks @maflcko, I would actually prefer mentioning nproc (+ alternatives) only once in the docs - will be easier once the cmake PR updates are merged.
👍 hebasto approved a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2237595730)
ACK 8f2522d242961ceb9e79672aa43e856863a1a6dd.
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2237595730)
ACK 8f2522d242961ceb9e79672aa43e856863a1a6dd.
💬 hebasto commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2288250542)
cc @furszy
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2288250542)
cc @furszy
💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716596528)
Oh sorry, missed that. It's fine.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1716596528)
Oh sorry, missed that. It's fine.