💬 kevkevinpal commented on pull request "wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult":
(https://github.com/bitcoin/bitcoin/pull/32958#discussion_r2550484381)
Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

💬 kevkevinpal commented on pull request "wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult":
(https://github.com/bitcoin/bitcoin/pull/32958#discussion_r2550484520)
Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

💬 ismaelsadeeq commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3564018931)
> It seems counter intuitive, but from a memory management perspective IPC clients are treated no different than our own code. And if we started FIFO deleting templates that are used by our own code, we'd crash.

IMHO I think we should separate that, and treat clients differently from our own code, because they are different codebases and separate applications with their own memory.

> Maybe there are 100 downstream ASICs, one of which is very slow at loading templates, so it’s only given a
...

🤔 brunoerg reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493836797)
Concept ACK

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