💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867496666)
cc @martinus

💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1434850353)
```suggestion
peers_dat = self.nodes[i].chain_path / "peers.dat"
```

nit: You can use `/` to join paths

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#discussion_r1434931358)
Maybe we can shuffle this all around again, the next time we bump Qt (new LTS has just been released), and have to inevitably rebase/update all these patches?

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1867514479)
> EDIT: I guess this is a cache issue, just weird that both my machines ran into this. Will rebuild.

Thankfully the likelyhood if this occuring will be largely reduced after #28880.

🚀 fanquake merged a pull request: "build: switch to using LLVM 17.x for macOS builds"
(https://github.com/bitcoin/bitcoin/pull/28880)

💬 Sjors commented on pull request "refactor: share and use `GenerateRandomKey` helper":
(https://github.com/bitcoin/bitcoin/pull/28455#discussion_r1434949134)
Indeed directly including `key.h` makes sense.

💬 Sjors commented on pull request "refactor: share and use `GenerateRandomKey` helper":
(https://github.com/bitcoin/bitcoin/pull/28455#issuecomment-1867538485)
ACK 81187d04c2925bbd1281fe3cbdc2a30c2a02cca5

The Stratum v2 Template Provider in #28983 also uses the `CKey static_key; static_key.MakeNewKey(true);` pattern, so would (slightly) benefit from this change.

> do we plan on making this modification to these files aswell?

I can re-review if more tests are modified to use this new helper, but I'm also fine with leaving as is. New code can use it.

💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1867538883)
I actually found a way to get the compiler to produce a warning for these cases, but it is a bit tricky and has limitations: https://godbolt.org/z/GfzseM4q3

How it works:
* Add a static counter variable to `FuzzedDataProvider
* Add a default argument that uses `++` for the counter

That way a `-Wsequence-point` warning is triggered. This is also triggered for initializer lists, and gcc man page has some arguments why that is so.

💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1434950725)
yes, order in initializer clauses are defined: https://eel.is/c++draft/dcl.init.list#4

🤔 vasild reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1794246976)
Looks better!

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434906612)
`GetBlockTime()` returns `int64_t`. This could possibly cause some compiler warning about assigning signed integer to an unsigned variable. Better use the same type as the return type of the function.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434908033)
nit:
```suggestion
std::atomic<std::chrono::seconds> m_best_block_time{0s};
```

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434948774)
Commit a710e08398 `net: move state dependent peer services flags` moves this code to `src/net_processing.cpp` and changes from `g_initial_block_download_completed` to `m_initial_sync_finished`. The variable `m_initial_sync_finished` is used for something else and has a different semantic, thus using `m_initial_sync_finished` here is wrong. Later another commit changes from `m_initial_sync_finished` to the proper check.

I think it would be better to reorder the commits, so that the proper chec
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434937507)
I like how the best block's time is cached in `PeerManagerImpl::m_best_block_time` but further caching the expression "is the best block time older than 288 blocks" seems excessive and unnecessary to me. It makes the code more complex and obviously gets stale one second after it is set.

Here you can use something like:
```cpp
if (ApproximateBestBlockAgeInNumberOfBlocks() < NODE_NETWORK_LIMITED_MIN_BLOCKS) {
return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
}
...
int64_t Appr
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434938857)
Better expose this constant in `net_processing.h` instead of duplicating it here. This file already includes `net_processing.h`.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434912424)
Maybe change the argument from `uint64_t` to `std::chrono::seconds`, so that the units of it are more clear. The comment warrants an update:

```cpp
/** Set the height of the best block and its time (seconds since epoch). */
virtual void SetBestBlock(int height, std::chrono::seconds time) = 0;
```

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1434954442)
Not in this PR, unless someone else gives me the code for it :-)

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1434955493)
Will consider new wording if I need to touch / rebase.

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434962398)
The conditions under which `m_connman.SetTryNewOutboundPeer(false);` is executed are not the same before and after this change. Maybe it is ok, but is an unrelated change to the aim of this PR:

* before `m_connman.SetTryNewOutboundPeer(false)` would have been executed if this was true:
```cpp
(m_chainman.m_blockman.LoadingBlocks() || !m_connman.GetNetworkActive() || !m_connman.GetUseAddrmanOutgoing() || !TipMayBeStale()) && m_connman.GetTryNewOutboundPeer()
```

* after `m_connman.SetTry
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1867587043)
Rebased after #28880. Cleaned up the commits a bit. Doesn't work yet, because Qt doesn't build..

Also rethinking the best approach here a bit, as we've now essentially got to accomodate 3 different builds for macOS:
* Building for mac, on mac, using Apple Clang & ld64 (which also doesn't like `-fuse-ld=lld`)
* Building for mac, on Linux, using our downloaded LLVM (Clang + lld + tools)
* Building for mac, in Guix, using Guix's Clang + our downloaded lld + tools??
* Or shift further into
...