💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285912365)
The only check it does is, that if the default value is used, the return type must be able to represent it. (The other checks were already done before.

However, I think it is an implementation detail of `RPCHelpMan` when to check for internal bugs. Developers should just avoid writing bugs in the first place. And if they do, they'll get a helpful error message (in the tests) and they shouldn't care where it is from.

Thus, I've removed this from the docs completely.

💬 dergoegge commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1667922866)
Can be marked up for grabs

✅ dergoegge closed a pull request: "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode"
(https://github.com/bitcoin/bitcoin/pull/27050)

✅ fanquake closed an issue: "tracepoints: gnu-zero-variadic-macro-arguments warnings"
(https://github.com/bitcoin/bitcoin/issues/26916)

🚀 fanquake merged a pull request: "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings"
(https://github.com/bitcoin/bitcoin/pull/27401)

💬 MarcoFalke commented on pull request "test: locked_wallet, skip default fee estimation":
(https://github.com/bitcoin/bitcoin/pull/28232#issuecomment-1667955527)
concept ACK

🤔 furszy reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-1565482617)
re-ACK 144dd46.
No changes since the last review.

📝 andrewtoth opened a pull request: "validation: don't clear cache on periodic flush"
(https://github.com/bitcoin/bitcoin/pull/28233)
Now that #17487 is merged we no longer need to clear the cache when syncing the coins cache. For periodic flushes that happen every 24 hours, there's no need to clear the cache. This can result in much faster block connection (see https://github.com/bitcoin/bitcoin/pull/18941#issuecomment-633684774).

Also note that this still clears the cache for pruning flushes. Having frequent pruning flushes with a large cache that doesn't clear is less performant than the status quo https://github.com/bit
...

🤔 MarcoFalke reviewed a pull request: "util: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107#pullrequestreview-1565463240)
left some quick comments

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1285950696)
```
<stdin>:66: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

💬 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?