💬 Psifour commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463882411)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).

Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that speci
...

💬 ajtowns commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765)
> After [#27675](https://github.com/bitcoin/bitcoin/pull/27675) (but also before, with a few more restrictions), we relay any tx from our mempool (except for those that arrived after the last INV sent to the peer), so someone wanting to fingerprint our mempool could just ask for those transactions directly instead I think and then check whether we send it to them or answer with `NOTFOUND`.

I don't think this issue makes sense as an attack -- we aim to avoid giving away tight timing information
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3463894000)
I'd say it's time to update the PR description:
<details>
<summary>24% faster reindex-chainstate | 921129 blocks | dbcache 450 | rpi5-16-2 | aarch64 | Cortex-A76 | 4 cores | 15Gi RAM | ext4 | SSD</summary>

```
COMMITS="cb0fdfdf3704d5ffe6ccc634de6fdba6b7b57a85 2aa510348143521a14146e41b5cf87cb3e60b29e"; \
STOP=921129; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q ori
...

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2475412779)
I agree that mentioning a consensus error makes sense, so I added it to the warning message. Note that I didn't change the existing `CheckForkWarningConditions()` message as well, but could do that too if reviewers suggest it.

In principle, I don't see a difference whether this scenario happens during IBD or close to the tip. In both cases, it is most likely corruption but a consensus error is possible too. Current behaviour is that in the IBD case, we will cycle through header-syncs with n
...

💬 TheCharlatan commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2475436253)
In commit fa128d1488c08d9816f425c73d01db4c1597ee68:

Would the return type of `GetTotalSize` fit into the criteria for this refactor?

```diff
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index cf6da55aa9..9d50dfe949 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -195 +195 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
- unsigned int tx_missing_size = 0;
+ uint32_t tx_missing_size = 0;
diff --git a/src/ne
...

💬 l0rinc commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3463979308)
I have also documented some workarounds for mac in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 - they kinda' work, without sanitizers at least, which cannot handle exceptions for some reason.

But I'm not sure why we want to give up fuzzing on Macs, everything we do is a whack-a-mole game, if we want people to write fuzz tests, we can't expect them to run them on a different machine and not even try them locally. We'll just end up with 100 PR pushes because people will
...

💬 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)};
```