💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992357083)
info: The `NetworkStringToId()` duplication was introduced in db4d2c282afd46709792aaf2d36ffbfc1745b776. Could add a reference in the commit message if you retouch.

👍 hodlinator approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2679972740)
Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4

Good to see `-getinfo` negation behavior fixed. I encountered it when exploring args in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013 (in the "Expand/collapse lengthy exploration"-section).

Still think the 3rd commit is out of character compared to my brief experience with this project as it is purely stylistic, but at least the formatting was [fixed](https://github.com/bitcoin/bitcoin/pull/31887#discussi
...

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992388251)
Thread https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960004078:

I still read https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982 as *what can be done* technically, not *what should be done* (convention). What comes close to indicating convention is the SO-link which shows that some random guy on the internet also prioritizes default visibility over other conventions. His example of taking a `struct` from a C library and inheriting from it into a `class` is far fr
...

💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992361754)
nit: If you re-touch it might be good to add the missing `#` to the commit message (for tooling/consistency). Unless "PR <number>" is also a convention for tooling unbeknownst to me.

```diff
-PR 21192
+PR #21192
```

💬 hodlinator commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992427691)
> I ran `CheckBlockBench` a few times to see if there's any difference and there's barely any (the current one seemed a tiny bit faster with Clang) - so I'd rather go for simpler code.

I have yet to have a run where my alternative was not faster when I set `-min-time=30000`. Seems to be around 2% faster on my CPU.

Agree that it's good to not complicate consensus code. As you say the loop exits immediately after initializing the iterator and checking that it's not at the end, then checking
...

💬 achow101 commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719368979)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8

Other than the issue with the example bitcoin.conf, everything seems correct. I think we can move forward with rc1 and just backport the gen-bitcoin-conf.sh fixes for rc2 (or final). The script has to be rerun for every rc anyways.

✅ fanquake closed a pull request: "docs: Enhance TODO section in README.md with detailed explanations"
(https://github.com/bitcoin/bitcoin/pull/32048)

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317)
I omit the `#` in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992477649)
Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here.

📝 theStack opened a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050)
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and it almost never makes sense to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC
...

💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992480335)
Your review comment provides that context (thanks). I wouldn't usually add that to the commit message for the same reason as https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317. In this case, however, where the PR discussion is locked, maybe it wouldn't add notifications there. Not sure.

💬 jonatack commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2719457484)
Concept ACK, will begin nuts and bolts review.

✅ fanquake closed an issue: "test: 32-bit Clang `ipc_test` failure at `-O0`"
(https://github.com/bitcoin/bitcoin/issues/31772)

🚀 fanquake merged a pull request: "depends: patch around PlacementNew issue in capnp"
(https://github.com/bitcoin/bitcoin/pull/31998)

👍 fanquake approved a pull request: "ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env""
(https://github.com/bitcoin/bitcoin/pull/32044#pullrequestreview-2680189403)
ACK fa21597064b8599b04461b8d3553e802e47d063e

🚀 fanquake merged a pull request: "ci: Revert "Temporary workaround for old CCACHE_DIR cirrus env""
(https://github.com/bitcoin/bitcoin/pull/32044)

💬 davidgumberg commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719475327)
ACK https://github.com/bitcoin/bitcoin/commit/47e2fa86dc5433852fd9e5050a23de2accfdca8d

I'm ~0 on whether or not the release candidate should be held up by the example `bitcoin.conf`, but everything else looks good to me.

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2719581088)
Addressing the review from [comment](https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2656773286)
- The coverage now includes functional tests
- Removed coverage from external deps (crc32,leveldb,minisketch,secp256k1) and `src/test`.
- Now uses `-DCMAKE_BUILD_TYPE=Debug`
- Notes are now in Blockquotes
- Incuding the functional tests has increased the mismatch from 2 to 61 .

📝 jonatack opened a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051)
When an addnode peer is disconnected by our IBD headers/blocks download timeout or stalling logic, ThreadOpenAddedConnections attempts to immediately reconnect it.

During IBD in a low-bandwidth environment, I see these disconnections/reconnections frequently with high-quality addnode peers.

This pull attempts to avoid unnecessary disconnections where it may make sense to do so. For now, it is an initial quick draft.

💬 fanquake commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2719746790)
```bash
[23:32:25.656] /ci_container_base/src/rpc/net.cpp:310:125: error: invalid operands to binary expression ('const char[198]' and 'const char[196]')
[23:32:25.656] "Nodes added using addnode (or -connect) are protected from disconnection due to DoS or IBD header/block\n" +
[23:32:25.656] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
[23:32:25.656] 1 error generated.
```