👍 darosior approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3494282906)
ACK 6e37b3b4861733ca97ec5d27d0bf52d187b6a2c9

nit: i think it would be good to have a sentence that fuzzing support is only maintained for Linux platforms as the first line of the document, so readers know what to expect. It's fine if it's also repeated in the "MacOS notes" section.

💬 achow101 commented on pull request "wallet: Add separate balance info for non-mempool wallet txs":
(https://github.com/bitcoin/bitcoin/pull/33671#issuecomment-3564531029)
> Another approach would be to report it as:
>
> ```
> trusted: 6049.99
> untrusted_pending: 0
> immature: 3200
> nonmempool-pending: -150
> ```
>
> ie "your balance is this, but you have X funds that are currently not in your mempool that you might lose some/all of"

I would prefer this approach and docs just need to be clear that some of that amount may come back as change.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2550914901)
> nit: inline is implicit

clang warns about unused functions if I remove the `inline`

💬 Eunovo commented on pull request "validation: fix assumevalid is ignored during reindex":
(https://github.com/bitcoin/bitcoin/pull/33854#issuecomment-3564555819)
> 4\. try to connect that block, which will result in the entire chain up to that block being connected by the `msghand` thread. This could block the thread hours, in which no other peer would be processed, so existing peers will get disconnected (ping timeout), while net thread will still continually make new connections (that never complete the version handshake because we are too busy connecting blocks)

I've pushed a solution to this. In the current implementation, the background init thre
...

👋 Eunovo's pull request is ready for review: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854)

💬 darosior commented on issue "Higher **reported** memory usage of Bitcoin Core after version 29":
(https://github.com/bitcoin/bitcoin/issues/33351#issuecomment-3564638453)
@tyuxar this is expected and what i expect in OP. If other applications start using memory, caching will be scaled down.

🤔 hodlinator reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3493993983)
Concept ACK de6a28396b8bf7a3fd1124418b4cd5beba1906d6

`ComputeMerkleRoot(std::vector<uint256> hashes, ...)` takes a vector by value, but luckily call sites already move into it, and since the underlying array/memory is transferred on move, the capacity "transfers as well" - neat!

---

> Sorry to be this guy again, but it seems to me unreasonable to touch such critical consensus code for such marginal benefits. Unless observable benefits can be shown, ...

Thanks for being that guy!

Going by ht
...

💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2550683488)
Why are we changing from `GetHash()` to `GetWitnessHash()` here?

For txs without witness data they return the same value, which master seems to implicitly validate.

💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2550702253)
If we keep the lambda, `_` is more the Python way of declaring unused variables, while C/C++ usually omits the identifier entirely:
```suggestion
auto leaves{ToMerkleLeaves(block.vtx, [](bool, const auto& tx) {
```
or
```suggestion
auto leaves{ToMerkleLeaves(block.vtx, [](auto /*first*/, const auto& tx) {
```
Notice I also removed the capture-`&` when it is not needed, which makes the lambda more pure (less cognitive load). (Also prefer `bool` over `auto` since it doesn't cost a
...

💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2550791531)
How about `(txs.size() + 1) & ~(decltype(txs.size()){1})`? Accessing `txs.size()` one time less at runtime. :)

💬 achow101 commented on pull request "test: Retry download in get_previous_releases.py":
(https://github.com/bitcoin/bitcoin/pull/33915#issuecomment-3564659764)
ACK fad06f3bb436a97683e8248bfda1bd0d9998c899

✅ achow101 closed an issue: "ci: failure to download 0.14.3-win64.zip in Win test cross-built"
(https://github.com/bitcoin/bitcoin/issues/33913)

🚀 achow101 merged a pull request: "test: Retry download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/33915)

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2551029734)
nit: With the latest push (https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c#diff-d6d633592a40f5f3d8b03863e41547de8751b874c1d20f129a616b9dd719b999), we are testing the same thing on two lines.

⚠️ plebhash opened an issue: "should concurrent IPC requests directed to the same thread cause a crash?"
(https://github.com/bitcoin/bitcoin/issues/33923)
recently I reported https://github.com/bitcoin/bitcoin/issues/33554, which was fixed by the introduction of `interruptWait` via #33575

then I noticed crashes for similar reasons (although unrelated to `waitNext`), which I explain in detail here: https://github.com/stratum-mining/sv2-apps/issues/88#issuecomment-3558003166

TLDR: I was trying to do a `getBlock` while a `submitSolution` was happening at the same time, both against the same Bitcoin Core thread

this made me realize that regardless
...

💬 achow101 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3564768759)
I agree with @ajtowns and don't entirely see the benefit of having split this commit into its own PR.

As someone who frequently writes gigantic PRs that need to be split up, this commit for me would not have met the bar to be split into its own PR. It isn't a complicated refactor that needs focused review on its own; it isn't something that is useful across multiple PRs; and it isn't itself a feature that is useful on its own.

I get that having smaller PRs can make it feel like things are
...

💬 achow101 commented on pull request "net_processing: reorder the code that handles the VERSION message":
(https://github.com/bitcoin/bitcoin/pull/33792#issuecomment-3564788109)
Similar to #33565, I don't see why it is useful to have this commit be its own PR.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3564792110)
Latest push f2ce3626a6a40b8688d711da1924db156dc2f02c -> 039c3aa breaks up the harness into more reviewable chunks. I forgot to add @stringintech as co-author on a commit during rebase, will address the next time I push up.

💬 achow101 commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564826323)
@fjahr DrahtBot is ignoring your ACK, is that intended? If you mean to withdraw your ACK, I think editing your comment with a strikethrough and explanation is the typical way people do that.

💬 achow101 commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564833814)
I'm slightly confused as to what the plan is between this and #33770. This PR both has code conflicts and it seems logical conflicts with #33770. 33770 redefines `-asmap` in one way, while this redefines it in the opposite way.

Since 33770 seems more likely to be merged soon, how would this PR change to resolve those conflicts.