💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549751593)
No need to have `std::optional` for the offset, could just be sent in as `0` when reading whole blocks. Same for parameter types in rest.cpp.
```suggestion
bool BlockManager::ReadRawBlockPart(std::vector<std::byte>& data, const FlatFilePos& pos, size_t part_offset, std::optional<size_t> part_size) const
```

<details><summary>Full compiling diff (includes suggestion to skip noop seeks, see other comment).</summary>

```diff
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index
...

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549741549)
nit: Could avoid syscall in common case
```suggestion
if (offset > 0) filein.seek(offset, SEEK_CUR);
```
Other PR avoiding similar syscall which was leading to slowdowns on Windows: #30884
Alternatively one could perform the noop-check inside of `AutoFile` itself.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2549772096)
nit: Could call `block_part.clear()` between each read.

💬 hebasto commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3563408869)
> > In my view, dropping support for x86_64-w64-mingw32 should be considered separately and at a later stage, once the codebase becomes incompatible with MSVCRT or the toolchain landscape has matured.
>
> Shouldn't it be removed once we remove it from the CI? which, according to the PR description in #33764, will be done once the Guix migration is done:

Agreed.

> > MSVCRT-related CI jobs should be removed from the CI framework once the migration to UCRT is complete.
>
> Just trying t
...

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2550080275)
> Yeah, I guess there were some silent conflicts.
Indeed. Fixed.

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3563434095)
> [#33764 (comment)](https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718)

Thanks! Fixed.

💬 diegoviola commented on pull request "Fix bitcoin-qt visual glitches on Wayland":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563438977)
> For example, on Fedora 43 with Gnome 49 and Wayland

Ah, I haven't tested this on GNOME. I suspect the use of `showNormal()` likely helps with the desired window behavior, which is why the revert works better.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2550098336)
Not a very strict reason, but to allow more flexibility and not to break the search for minimal whitespaces or indentation changes.

💬 hebasto commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2550139808)
> Concept ACK
>
> I am however unable to trigger the fatal error on `Ubuntu 24.04.3 LTS` with `Python 3.9.23`

I cannot reproduce the issue:
```
$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 24.04.3 LTS"
$ python3 --version
Python 3.9.23
$ cmake -B build
<snip>
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Required is at least version "3.10")
Reason given by package:
Interpreter: Wrong version for the interpreter "/home/hebasto/.pyenv/shim
...

📝 Sjors opened a pull request: "mining: add getMemoryLoad() and track template non-mempool memory footprint"
(https://github.com/bitcoin/bitcoin/pull/33922)
Implements the template memory footprint tracking discussed #33899, but does not yet impose a limit.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2550158500)
Agree that would be slightly more optimal, avoiding some needless pushes/pops. Taken in latest push + changed `auto& i` to `Node& n` for clarity.

💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3563543871)
I haven't benchmarked this yet on mainnet, so I'm not sure if checking every (unique) transaction for mempool presence is unacceptably expensive.

If people prefer, I could also add a way for the `getblocktemplate` RPC to opt-out of the memory bookkeeping, since it holds on to one template max and no longer than a minute.

💬 mzumsande commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2550173315)
hmm, the conflicting a0eaa4492548800ba1b2cdd8232195ab5d5c49c7 was merged on August 8. I thought we had regular re-runs of the CI with respect to current master that should have failed - or was the CI already red yesterday and I missed it?

💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3563559444)
> We would just need to replace `size_t` with a more complicated data structure that's aware of transaction counts (like `map<CTransaction*, long>`) and update this in the `BlockTemplateImpl` constructor and destructor.

I implemented this part #33922.

💬 fanquake commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2550230893)
> or was the CI already red yesterday and I missed it?

The CI was green yesterday. The open/close today kicked the CI.

💬 maflcko commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2550234606)
https://github.com/bitcoin/bitcoin/pull/33145#issuecomment-3409554826

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2550235052)
Close, fixed the *feature_asmap.py* failure by implementing special logic in the move-ctor. But I recommend keeping it as a non-const member so that it can be moved from instead of copied using the default move-ctor.

<details><summary>Not recommended diff for the curious</summary>

```diff
--- a/src/netgroup.h
+++ b/src/netgroup.h
@@ -17,7 +17,10 @@
class NetGroupManager {
public:
NetGroupManager(const NetGroupManager&) = delete;
- NetGroupManager(NetGroupManager&&) = defau
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550252802)
Memory ordering is the default `std::memory_order_seq_cst`. I don't think we need to worry about performance here, so we can use the default strongest ordering.

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3563683917)
@sdaftuar if that's the real concern, shouldn't we be ideally dealing with that at TxGraph level? Something analogue to `m_acceptable_iters`? Is it that SFL alone only "fixes" it by just making it a lot less likely to run a long time?

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