💬 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.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288318410)
Force pushed for :rocket: lazy re-init, which gives:
* 100x iterations/s in the lazy case
* 1.35x iterations/s in the expensive case
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288318410)
Force pushed for :rocket: lazy re-init, which gives:
* 100x iterations/s in the lazy case
* 1.35x iterations/s in the expensive case
🤔 glozow reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2237602476)
Thanks @theStack!
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2237602476)
Thanks @theStack!
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716582838)
Ah, that looks like a bad rebase - deleted the comment now, thanks
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716582838)
Ah, that looks like a bad rebase - deleted the comment now, thanks
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644034)
Right, I don't think it's problematic to not log tx invs during IBD since they are ignored anyway.
I've moved the logging change to its own commit, to keep the pure refactoring-ness of this commit :+1:
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644034)
Right, I don't think it's problematic to not log tx invs during IBD since they are ignored anyway.
I've moved the logging change to its own commit, to keep the pure refactoring-ness of this commit :+1:
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644076)
Ah, good observation. I've now gated the `AlreadyHaveTx` check inside of `AddTxAnnouncement` on `p2p_inv` so this is no longer happening.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716644076)
Ah, good observation. I've now gated the `AlreadyHaveTx` check inside of `AddTxAnnouncement` on `p2p_inv` so this is no longer happening.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716591125)
done :+1:
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1716591125)
done :+1: