💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550626324)
Retouched a little bit, to use `extract()`, somewhat differently.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3564170142)
`e9b436a408...7987000d36`: take some suggestions

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550639055)
Yes, hmm... what would be the TODO text? There is nothing to-do around there - the assigned string to `what` is the correct one. Whenever a caller starts calling `CWallet::SubmitTxMemoryPoolAndRelay()` with `broadcast_method` equal to `NO_MEMPOOL_PRIVATE_BROADCAST`, then `CWallet::SubmitTxMemoryPoolAndRelay()` will just work without having to change anything in it.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550662202)
Shouldn't the logic be added when it's actually used? Otherwise it's just dead code waiting for event.
An explicit throw here would clarify that this is WIP.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550717279)
Unfortunately it seems we're stuck at
https://github.com/bitcoin/bitcoin/blob/fadad7a49477cd61fbbfe20a0a61023c2d4d70a1/depends/hosts/darwin.mk#L3
where the C++20 spec isn't fully implemented yet.
For proper spaceship operator support we would need: https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes:
> The following C++20 features have been implemented:
> The Mothership has Landed: Adding <=> to the Library ([P1614R2](https://wg21.link/P1614R2))
> Symmetr
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550720411)
> comment/note that now the declaration order is now relevant

Instead of a dead comment, can we add a test case that will instead fail the build when somebody accidentally changes this?

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2550802960)
Sounds good - fixed in [e14650967d..8457a71a64](https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c).

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2550803976)
Many thanks! [e14650967d..8457a71a64](https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c)

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2550804495)
Fixed by [e14650967d..8457a71a64](https://github.com/bitcoin/bitcoin/compare/e14650967dc95da5c10e0d6183b6eac3e8243fe5..8457a71a6420792a1b9c7c8645c9e5c607a4b90c).

👍 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