👍 TheCharlatan approved a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3396350580)
ACK f5eca8d5252a68f0fbc8b5efec021c75744555b6

I have fuzzed the thread pool for some time again, and did not find any new issues. Also the previously observed memory leak during fuzzing was not observed anymore. The changes to the http workers look sane to me.

Can you run the commits through `clang-format-diff.py`? There are a bunch of formatting issues in the first two commits.

💬 trevarj commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475562342)
> Would it work with the recursion in make-mingw-w64/implementation?

Forgot about that, so no🤦🏻‍♂️. Will be much better when that PR lands.

💬 ajtowns commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2475563895)
Perhaps document how we prevent concurrent access to this while you're at it? (We can't use `GUARDED_BY(m_nodes_mutex)` since that results in lock order issues with cs_main)

AIUI, accesses are:

* `DisconnectNodes()` via `ThreadSocketHandler()` which adds nodes and removes them in normal operation
* `StopNodes()` via `Stop()` which runs it after `StopThreads()`, which ensures `threadSocketHandler` is joined before returning. `Stop()` is invoked in `~CConnman()` and `init.cpp:Shutdown()`
...

💬 TheCharlatan commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2475582408)
Ah, I see. I think I would change the order then, since typically named optional args precede positional arguments / operands in command line applications: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01 .

🤔 fjahr reviewed a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3378943773)
Concept ACK

I agree with @ajtowns that coupling this with nolisten makes sense and also means that this should not be too dangerous. Rather than turning that on by default and potentially risking the user don't fully understand the implications of this, the option could also error if nolisten isn't set.

💬 fjahr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462055701)
nit: m_ prefix?

💬 fjahr commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2462049949)
Could the naming be kept consistent between the function names and the init arg? One is "disable v1" and the other is "only v2" and I don't see a good reason for that.

💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475596211)
Thanks for catching this, I'll remove the line, but like I said above, ideally this test wouldn't be stateful at all.

💬 jlopp commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3464196972)
15,000 words from 12+ year old emails?

Further specification is needed to explain how this has anything to do with Peter's control of his DNS seed. All I see are discussions of how different aspects of Bitcoin work.

🤔 l0rinc requested changes to a pull request: "refactor: Return uint64_t from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/33724#pullrequestreview-3393067389)
I like the focused commits with good explanations.

I would like to suggest to consider something slightly simpler that may fit the data better.
We're storing these serialized sizes as 64 bits to avoid having to cast when we multiply the value. But we're always serializing these sizes as either variable length ints or 32 bit ones, so it's a bit awkward that a method returns 64 bit value and we just have to remember to cast them to 32 at call sites (otherwise they would serialize incorrectly in a
...

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472821589)
Can we migrate the constant to 64 bits as well?
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/rpc/blockchain.cpp#L1907
```C++
static constexpr uint64_t PER_UTXO_OVERHEAD{sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool)};
```
Or if we will go the other way and mirror storage and only up-cast when needed, it would be:
```C++
static constexpr uint32_t PER_UTXO_OVERHEAD{sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool)};
```

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472810064)
This value will be returned and converted to a `size_t` again
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/index/blockfilterindex.cpp#L201
and added to a `FlatFilePos::nPos`
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/index/blockfilterindex.cpp#L285
which is currently a `uint32_t`:
https://github.com/bitcoin/bitcoin/blob/fa128d1488c08d9816f425c73d01db4c1597ee68/src/flatfile.h#L17

Can we avoid converting this back a
...

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472815299)
is there any scenario where where a signed `UniValue` value is different from an unsigned one?

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472880211)
https://github.com/bitcoin/bitcoin/blob/fa128d1488c08d9816f425c73d01db4c1597ee68/src/node/blockstorage.h#L120
should also likely be updated now to:
```C++
static constexpr uint32_t STORAGE_HEADER_BYTES{std::tuple_size_v<MessageStartChars> + sizeof(uint32_t)};
```

----

Note: I understand this works on most 32 bit systems we support, but https://en.cppreference.com/w/cpp/language/types.html indicates it can vary between 16 and 32 bits.
Also, https://learn.microsoft.com/en-us/cpp/cpp/dat
...

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472903256)
Not sure I understand why `GetBlockWeight` is `int64_t`
https://github.com/bitcoin/bitcoin/blob/3c5d1a468199722da620f1f3d8ae3319980a46d5/src/consensus/validation.h#L136
but `MAX_BLOCK_WEIGHT` and `MAX_BLOCK_SERIALIZED_SIZE` are only `unsigned int`
https://github.com/bitcoin/bitcoin/blob/3c5d1a468199722da620f1f3d8ae3319980a46d5/src/consensus/consensus.h#L15

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472976911)
`MAX_FLTR_FILE_SIZE` is only used here
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/index/blockfilterindex.cpp#L39
we should adjust its type to match the usage
```C++
constexpr uint32_t FLTR_FILE_CHUNK_SIZE{0x100000}; // 1 MiB
```

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472910698)
`FindNextBlockPos` has `unsigned int nAddSize` arg still:
```C++
FlatFilePos BlockManager::FindNextBlockPos(uint32_t nAddSize, unsigned int nHeight, uint64_t nTime)
```

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2473369894)
If we're changing this, we should likely adjust the usage as well:
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/wallet/spend.cpp#L1085
```C++
int32_t tx_noinputs_size = 0;
```

💬 l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2473712142)
https://github.com/bitcoin/bitcoin/blob/fa128d1488c08d9816f425c73d01db4c1597ee68/src/flatfile.h#L16
is `int32_t` now so we should update these as well:
```C++
const int32_t nFile = pos.nFile;
if (static_cast<int32_t>(m_blockfile_info.size()) <= nFile) {
```

💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3464387417)
Rebased. Some of the feedback has been addressed.

My Guix build:
```
x86_64
366c53942430c0f310fc42445693e7ba1ff5dce25f2df8e42d3c4b044747f385 guix-build-4d31ddc6dfe6/output/dist-archive/bitcoin-4d31ddc6dfe6.tar.gz
1b0c2e70c8f85650a4b4238c74dd22b960b2f4036042d2f8c6b302dbc50ae3b7 guix-build-4d31ddc6dfe6/output/x86_64-w64-mingw32/SHA256SUMS.part
ed292458da2126949ee0a35495ba59272066364d7b458b28576dcd229ed984b5 guix-build-4d31ddc6dfe6/output/x86_64-w64-mingw32/bitcoin-4d31ddc6dfe6-win64-cod
...