💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285964524)
What is this and how does this even compile? I presume `const uint256&&` decays into `const uint256&`, but that already exists??

Would be better to just remove this line, unless there is a reason to do add a line here.

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285972399)
Any reason to delete these? The compiler already has them deleted and provides a better error message than yours: `candidate function (the implicit move assignment operator) not viable: no known conversion from 'transaction_identifier<true>' to 'transaction_identifier<false>' for 1st argument`.

Would be better to just remove those lines, unless there is a reason to do add lines here.

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285956464)
The "opposite" type would be *any* type other than the type itself. For example, currently it is possible to compare `uint256` (a txid) with Wtxid, or `uint256` (a wtxid) with `Txid`.

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285972674)
Same?

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285978619)
nit: Any reason to use static_cast when reinterpret_cast can be used? (`reinterpret_cast` is guaranteed to always be compiled to a noop)

💬 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