🤔 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.

💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564836443)
> @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.

It falsely interpreted this comment as an ACK previously and that's when I put the ignore: https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497304815 I have removed the emoji reaction, not sure if that makes it reconsider my latest ACK but that is still valid.

💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3564842717)
re-ACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4

Only minor fixes since last review.

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

It is not the opposite way, it's an orthogonal issue. #33770 removes the usage of the default file and this PR splits the `-asmap` usage from being a path arg (maybe interpreted as a bool) to a bool arg and a path arg seperately which is much cleaner compared to master (with
...

💬 achow101 commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3564906970)
ACK de7c3587cd4586bbed94a4ea6eae4a252301daee

💬 sipa commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564910471)
I think having two options to control such a minor feature is unintuive and overkill. If that's what people like, no objection as it really isn't important. But my preference would be to put everything in the `-asmap` option:

* `-asmap=none` (and for compatibility, `-asmap=0`) to get the /16 based splitting.
* `-asmap=builtin` to get the file built-in to the binary, or the same as `none` if nothing was built in.
* `-asmap=<filename>` to get the specified filename if it exists, and error oth
...

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3564915518)
I guess there are too many walls of text here and elsewhere so I will make it more concise, this PR here (and https://github.com/bitcoin/bitcoin/pull/33632 with a different approach) fixes two things:

1. `bitcoind -asmap=1` => errors because it looks for a file named "1" in datadir
2. You can not use asmap=1 in the config file for the same reason

https://github.com/bitcoin/bitcoin/pull/33770 doesn't fix this, it just gets rid of the current reason anyone would use `-asmap=1`, to activate
...