💬 MarcoFalke commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1285993750)
unrelated: May be good to write a clang-tidy plugin to enforce the limits are compile-time constants and in range to avoid silent UB at runtime?

The in-range one can be submitted to upstream and the other check can be done in this repo.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1668023008)
rmf or is anything left to be done here for this test-only pull?

💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1285990092)
This interface is not the right place for a callback like this because it is not a "net event".

Would be better to have a method on connman, e.g. `CConnman::SetDesirableServiceFlags` that is then called by peerman.

🤔 dergoegge reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1565523628)
I don't think the scenario you describe is likely to happen since we consider ourselves to be in IBD if our tip is older than 24h (which corresponds to ~144 blocks).

> Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections
...

💬 MarcoFalke commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1286001205)
```suggestion
options.max_orphan_txs = uint32_t(std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()));
```

nit, if you re-touch?

💬 andrewtoth commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#issuecomment-1668030656)
@furszy thank you for the review. Sorry I forgot to address your nits on this rebase. Will do them if I have to rebase again.

👋 fanquake's pull request is ready for review: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1668052979)
> is anything left to be done here

I missed that Cory had already ACK'd, and was waiting for him to re-ACK. Given that's already happened, I don't think there's anything left here, and docs can be fixed in the followup.

💬 glozow commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1668053761)
concept ACK

💬 MarcoFalke commented on pull request "doc: remove Fedora libdb4-*-devel install docs":
(https://github.com/bitcoin/bitcoin/pull/28231#issuecomment-1668066449)
lgtm ACK 11a499eb4d57f2c5eabd7955bcc7e419364b3194

🚀 fanquake merged a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)

💬 TheCharlatan commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668073018)
Concept ACK

What about this one:
```
wallet/wallet.h should remove these lines:
- #include <boost/signals2/signal.hpp> // lines 52-52
```
Probably should be moved to the implementation file, no?

📝 ChrisCho-H opened a pull request: "fix: bad opcode err msg includes reserved opcode"
(https://github.com/bitcoin/bitcoin/pull/28234)
bad opcode err msg includes `reserved`, because reserved opcode is evaluated as BAD_OPCODE(and it's wrong to state `missing or not undestood` for it).

👍 MarcoFalke approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1565583066)
ACK d8f1222ac50f089a0af29eaf8ce0555bad8366ef 🔠

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK d8f1222ac50f089a0af29eaf8
...

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1286038136)
c95b37d641b1eed4a62d55ca5342a6ed8c7a1ce7: same

💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1286025216)
style nit in ea8135de7e617259cda3fc7b1c8e7569d454fd57: May be better to add a newline before `:` to reduce the leading whitespace bloating block.

💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668087768)
Can you post a patch that compiles?

💬 jonatack commented on pull request "fix: bad opcode err msg includes reserved opcode":
(https://github.com/bitcoin/bitcoin/pull/28234#issuecomment-1668107467)
If this change is correct, the failing tests would need to be updated in the same commit (see https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core for more details).

💬 RandyMcMillan commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1668112752)
Concept ACK

👍 MarcoFalke approved a pull request: "assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1565641144)
lgtm. Left style nits