💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2550274232)
Maybe it's safer to track by Wtxid?

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3563724551)
> Unless observable benefits can be shown

The change itself is trivial, it's just a **+1** for the reserve before calling the Merkle calculation.
I don't mind simplifying the change if you think that's easier to review (I could also just bump `leaves.reserve(block.vtx.size())` to `leaves.reserve(block.vtx.size() + 1)` without the refactors).
Originally it was similar to that, but based on [concerns](https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2264185044) to decouple callers fr
...

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#discussion_r2550291196)
b9306b79b8f5667a2679236af8792bb1c36db817: in addition, we might be wiping the dummy coinbase from the template later: https://github.com/Sjors/bitcoin/pull/106

🤔 TumaBitcoiner reviewed a 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#pullrequestreview-3493400959)
I made some suggestions to keep consistency between naming and files. Sometimes `result` was used, other times `err`.

💬 TumaBitcoiner 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_r2550237244)
```suggestion
const auto result{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
```
Should this be defined as `result` too, as you did in other parts of the code?

💬 TumaBitcoiner 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_r2550238619)
```suggestion
assert(result== PSBTResult::OK);
```
As above.

💬 TumaBitcoiner 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_r2550248118)
```suggestion
const auto result{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
```

💬 TumaBitcoiner 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_r2550248662)
```suggestion
assert(result== PSBTResult::OK);
```

💬 TumaBitcoiner 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_r2550251277)
```suggestion
if (result != PSBTResult::OK || complete) {
```

💬 TumaBitcoiner 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_r2550250397)
```suggestion
const auto result{wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)};
```

💬 TumaBitcoiner 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_r2550261850)
```suggestion
if (result != PSBTResult::OK) return false;
```

💬 TumaBitcoiner 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_r2550261153)
```suggestion
auto result{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
```

💬 TumaBitcoiner 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_r2550264017)
```suggestion
const auto result{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
```

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3563746342)
@instagibbs After discussing this offline with @sdaftuar, I think the complexity here is due to the combination of a number of requirements:
* We want to bound the latency of a single `TxGraph::DoWork()` invocation (to something in the order of milliseconds).
* We want to bound what percentage of CPU time in total is spent by `TxGraph`'s linearization efforts (inside `DoWork()`, or elsewhere), over long periods of time.
* In practice get everything optimal.

The current code, which sets `PO
...

🤔 ismaelsadeeq reviewed a pull request: "mining: add getMemoryLoad() and track template non-mempool memory footprint"
(https://github.com/bitcoin/bitcoin/pull/33922#pullrequestreview-3493499544)
Concept ACK

I think it would be better if we have internal memory management for the mining interface IPC, since we hold on to the block templates.

I would suggest the following approach:

- Add memory budget for the mining interface.

- Introduce a tracking list of recently built block templates and total memory usahe.

- Add templates to the list and increment the memory usage after every `createnewblock` or `waitnext` return.

- Whenever the memory budget is exhausted, we should release tem
...

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3563782644)
reACK d782bffc5461f7361ae410963466ae1577ef63df

The only [difference](https://github.com/bitcoin/bitcoin/compare/c6ac239124b3e66f2d5ff52edc6ff422926abead..d782bffc5461f7361ae410963466ae1577ef63df) since last ACK was [to omit adding empty leaves](https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2541499665)

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3563799563)
> In my opinion, we should not rely on the IPC client to manage our memory.

> Whenever the memory budget is exhausted, we should release templates in FIFO order

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.

So I think FIFO deletion should be a last resort (not implemented here).

There's another reason why we should give cl
...

💬 fjahr commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3563843760)
Concept ACK

💬 maflcko commented on pull request "CI: silent merge check":
(https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3563863948)
I think what could work (hacky, untested):

* Setup a separate "silent-merge-check" repo with the desired ci config
* Push the selected ci runs to it on the desired schedule
* Get the result, and on failure, pass it back to the upstream repo

💬 fanquake commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2550408178)
I'm wondering, give we are building IWYU from source, can we just patch it, so the output is correct (uses <>); avoiding the need for this fixup.