💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1867422517)
Rebased and taken out of draft after the dependency https://github.com/bitcoin/bitcoin/pull/29056 was merged

💬 maflcko commented on issue "failure in wallet_basic.py --descriptors":
(https://github.com/bitcoin/bitcoin/issues/27249#issuecomment-1867447005)
Reproduced locally: https://drahtbot.space/temp_scratch/wallet_basic_coins_2.tar.xz

💬 maflcko commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1867457231)
It may be good to check the upstream source code inside macOS for recently introduced issues.

💬 naumenkogs commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1434889375)
I've spent a ridiculous amount of time understanding what's going on here, and I'm still far from being 100% sure. Perhaps if you touch this comment, you can expand it even further? E.g., connecting to each of the conditions in `else if`

💬 naumenkogs commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1434899330)
8d8e0b796dabba601742a9585237be5892c5f05a

maybe expand `for every reachable network`

💬 naumenkogs commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1434903032)
dfd635bcc9b3f3615cabe115af302df33efba6a1

nit: The first sentence talks about `might`, while the second makes a reader think this is unconditionally a fallback (which is not true). I'd suggest aligning the two

💬 Sjors commented on pull request "rpc: add path to gethdkey":
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1867496111)
I plan to re-work this on top of #29130 with a similar interface to `createwalletdescriptor`. That is, if you have a regular wallet with normal descriptors, it will Just Work(tm). Otherwise you need to specify which master key to use.

Initially I'll make a separate PR. If #26728 is closed in favor of the new approach in #29130 then this PR can be closed too.

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