💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440372713)
Again, this should affect everyone on macOS today already on current master, using clang-15 (or later), so a separate issue and bugfix can be filed?

👍 vasild approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1801881895)
ACK 6c603490023317a84c7c96ef4b64f4f96ea03d1f

It would be nice to avoid duplicating the constant `NODE_NETWORK_LIMITED_MIN_BLOCKS` ([discussion](https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434938857)) but I do not see that as a blocker of this PR.

Thanks!

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1440386706)
No strong opinion. To me `net_processing.h` looks like the natural place to move/expose the constant(s) that were defined as `static` in `net_processing.cpp` if they need to be used outside of that `cpp` file. Do you somehow dislike that idea or have another one?

I dislike having them duplicated because then there is a chance that they will go out of sync. Also, in semantic completion tools one can point to a symbol and ask "show me all places where this is used". If what is used in the test
...

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440417362)
> Sure, but that happens today on current master, unrelated to the changes in this pull request.

No? The addition of -latomic here, is only needed with the changes in this PR. Otherwise CI on master would be failing.

💬 maflcko commented on issue "CI failure: `ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const`":
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1875350212)
> To be able to remove the suppression, all these these cout writes would need to be protect with a lock. We can lock `G_TEST_LOG_FUN` ourselves since it is our own code. It looks like we could synchronize the boost cout output by redirecting it with `boost::unit_test::unit_test_log.set_stream( std::ostream& );` https://www.boost.org/doc/libs/1_77_0/libs/test/doc/html/boost_test/test_output/logging_api/log_ct_output_stream_redirection.html with a custom `std::ostream` subclass that locks a mutex
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440439062)
Sorry, I meant fixing/changing the latomic build system check is unrelated, not the CI changes in this PR.

💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440456087)
We already assume the median can be rounded down, so I don't think extra rounding changes any assumptions, and therefore I wouldn't overcomplicate things, it's just not worth it imo. I'd be okay with casting to a double first, but it's not strictly better as we're trading off precision at the higher ranges (which I also don't think is important at all).

```cpp
if (m_index % 2 == 0) {
return (0.5 * sorted_copy[m_index / 2]) +
(0.5 * sorted_copy[m_index
...

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1875443279)
This is relevant and I am rewriting it as per the discussions above.

💬 maflcko commented on issue "Possibility to dump all runtime parameter values":
(https://github.com/bitcoin/bitcoin/issues/28790#issuecomment-1875457667)
To get all config options (and their default) you can run `-help`.

To get all interpreted config options, you can look a the debug log, as already explained.

🤔 furszy reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-1802244171)
Rebased, conflicts solved. Next PR in the line is #28987.

⚠️ Pissycat43 opened an issue: "In the future "
(https://github.com/bitcoin/bitcoin/issues/29168)
0xd8Ca987144CB163E6B2b1Edd6356622bEfD4d96E

✅ fanquake closed an issue: "In the future "
(https://github.com/bitcoin/bitcoin/issues/29168)

:lock: fanquake locked an issue: "In the future "
(https://github.com/bitcoin/bitcoin/issues/29168)

👍 dergoegge approved a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802291377)
utACK f761f0306b092c06cecd239060b36cd0ba45fa2c

👍 furszy approved a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1802295539)
reACK b093e5e

Only cleaned the IsMine && IsFromMe code dup from my last review.

✅ fanquake closed an issue: "Possibility to dump all runtime parameter values"
(https://github.com/bitcoin/bitcoin/issues/28790)

💬 fanquake commented on issue "Possibility to dump all runtime parameter values":
(https://github.com/bitcoin/bitcoin/issues/28790#issuecomment-1875492104)
Going to close this for now.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1875530051)
Rebased 717093f24b1ad65bc18b23eab492cfdcc2511e69 -> d63b2e88780dc78fd531b053653361a0bf3fcbea ([noGlobalSignals_0](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_0) -> [noGlobalSignals_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_0..noGlobalSignals_1))

* Fix conflict with #29013

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1440574231)
> To me `net_processing.h` looks like the natural place to move/expose the constant(s) that were defined as `static` in `net_processing.cpp` if they need to be used outside of that `cpp` file. Do you somehow dislike that idea or have another one?

I don't totally dislike it, but was pondering the option of moving them to a separate file for two reasons; I find the spread of constants across the sources hard to maintain, and I was also thinking about another testing scenario: what if we want to
...

🤔 glozow reviewed a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019#pullrequestreview-1802420294)
ACK f761f0306b092c06cecd239060b36cd0ba45fa2c given this brings us back to what we had before

It seems correct that none of the callsites need `GetSortedDepthAndScore`, though maybe topological is needed in wallet? I had the [same](https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845497136) question. I guess insertion order doesn't imply topological if a parent was inserted later than a child, i.e. if it's from a reorg?