💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3463997000)
I'm not sure this makes sense, per https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765 . Perhaps it would be better to:

* treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
* change our behaviour when sending GETBLOCKTXN to also request any transactions that
...

💬 l0rinc commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3464012836)
> -L/opt/homebrew/opt/llvm@14/lib

this is too old, you need at least version 16. Do a general `brew upgrade` on the system and retry, I have the same OSx, it should work.

✅ mzumsande closed a pull request: "validation: Make ReplayBlocks interruptible"
(https://github.com/bitcoin/bitcoin/pull/30155)

💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#issuecomment-3464018442)
Closing as "up for grabs" - https://github.com/bitcoin/bitcoin/issues/11600 is already closed, and because we now flush the chainstate every hour replays in general shouldn't take as much time anymore, so there is less need to interrupt/continue them.

That said, it could still be a nice-to-have if someone wants to pick it up (note https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1633721111 in this case, maybe there is a more elegant way I didn't see).

👍 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
```