💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1459853005)
> I think that scenario would already be problematic ... and then we wouldn't have a peer to evict.

"... then start disconnecting from `m_protected`"

> the math

I think it goes like this: (with 5 networks) the chance of not selecting IPv4 for the first peer is `4/5`, for the first and second peer is `(4/5)^2`. The chance of not selecting IPv4 in all `8` choices is `p=(4/5)^8`. So, the chance of not selecting IPv4 or not selecting IPv6 is `2*p`. The chance of not selecting IPv4, or IPv6
...

💬 kristapsk commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1459864966)
> This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

What's there?


💬 willcl-ark commented on issue "Small unspent can get removed from OutputGroup for being uneconomical probably leading to later partial spending":
(https://github.com/bitcoin/bitcoin/issues/20287#issuecomment-1459925538)
Even though `OutputGroup::GetPositiveOnlyGroup` has been refactored away, it looks to me that this is still an issue as `GroupOutputs()` is still filtering by `EffectiveValue() >0`:

https://github.com/bitcoin/bitcoin/blob/8d12127a9c19cb218d661a88ab9b6871c9d853b9/src/wallet/spend.cpp#L470-L473

If we remove the dust filter here then we become liable to a dust attack where an attacker could send us many dust outputs which would always be selected, causing our fee requirements to (significantl
...

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1129220513)
Yes, if it makes sense for you I'd appreciate your review there btw.

👍 brunoerg approved a pull request: "addrman: Enable selecting addresses by network"
(https://github.com/bitcoin/bitcoin/pull/27214)
crACK 25a64a20749f10ce84060f3570ad76d1a4776948

📝 MarcoFalke opened a pull request: "test: Use self.wait_until over wait_until_helper"
(https://github.com/bitcoin/bitcoin/pull/27226)
`wait_until_helper` is a "private" helper, not intended to be used directly, because it doesn't scale the timeout with the timeout factor. Fix this by replacing it with a call to `self.wait_until`, which does the scaling.

💬 MarcoFalke commented on issue "Issue in `p2p_ibd_stalling.py` under Valgrind":
(https://github.com/bitcoin/bitcoin/issues/27208#issuecomment-1459956694)
Ok, see https://github.com/bitcoin/bitcoin/pull/27226

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1459958764)
Updated f9e3c91f4d482b0c74ef899056d5a2c63e67e1fb -> c44983faa02b4845cd7605abe99c332980a07af1 ([tc/2022-09-libbitcoinkernel-chainparams-args_3](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_3) -> [tc/2022-09-libbitcoinkernel-chainparams-args_4](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_3..tc/202
...

💬 MarcoFalke commented on pull request "test: Default timeout factor to 4 under --valgrind":
(https://github.com/bitcoin/bitcoin/pull/27221#issuecomment-1459961347)
Yeah, this doesn't affect the valgrind CI system. Only test runs with `--valgrind` passed (and no timeout factor).

💬 MarcoFalke commented on issue "Rescanning use only 1 core":
(https://github.com/bitcoin/bitcoin/issues/19992#issuecomment-1459970293)
I think rescan is still single threaded, but there shouldn't be any reason to call it often, so I am not sure if there is a use case.

💬 MarcoFalke commented on issue "Rescanning use only 1 core":
(https://github.com/bitcoin/bitcoin/issues/19992#issuecomment-1459971651)
The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

✅ MarcoFalke closed an issue: "Rescanning use only 1 core"
(https://github.com/bitcoin/bitcoin/issues/19992)

💬 MarcoFalke commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1129250725)
```

src/wallet/walletdb.cpp:616:58: error: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg,-warnings-as-errors]
data.SetReceiveRequest(strKey.substr(2), std::move(strValue));

💬 fanquake commented on pull request "test: Default timeout factor to 4 under --valgrind":
(https://github.com/bitcoin/bitcoin/pull/27221#issuecomment-1459979548)
> Yeah, this doesn't affect the valgrind CI system.

Ok. Can we consolidate this at all? The disparity is somewhat confusing because you would assume that the Valgrind CI job would use the dedicated `--valgrind` option provided by the test framework.

💬 fanquake commented on pull request "doc: docment json rpc endpoints":
(https://github.com/bitcoin/bitcoin/pull/27225#issuecomment-1459981209)
Concept ACK

💬 fanquake commented on pull request "test: Use self.wait_until over wait_until_helper":
(https://github.com/bitcoin/bitcoin/pull/27226#issuecomment-1459982082)
Concept ACK. Will test under the CI

💬 willcl-ark commented on issue "Option to ignore small inputs when internal wallet is building TXes?":
(https://github.com/bitcoin/bitcoin/issues/20870#issuecomment-1459986236)
@Crypto2 can you not just do the inverse and use coin selection/manual construction when you want to exclude small outputs?

💬 MarcoFalke commented on pull request "test: Default timeout factor to 4 under --valgrind":
(https://github.com/bitcoin/bitcoin/pull/27221#issuecomment-1459987786)
Not sure if that makes sense. This would require special casing the non-test binaries `bitcoind/-cli/-tx/-util` etc in the valgrind wrapper. Also, if someone wants to interactively re-run a single test inside the CI pod, they'd have to pass `--valgrind`. Maybe open a new discussion thread or pull request?

💬 fanquake commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1459992955)
> This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

It will have been #27118 that triggered it. I can't look at this in mroe detail right now, but will by next week.

> On my Gentoo system with GCC 11.3.1 and glibc 2.36 this now gives me these warnings for each .cpp file when building Bitcoin Core

Looks like that's because you're using GCC 11.x, which doesn't support >=3 (it is supported with Clang >= 9). Somewhat annoying that glibc is going to warn like that,
...

💬 fanquake commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1129269455)
In 102b2033493f0d61e9763d094cb8a0017f7e3a10 bench: order the logging benchmark code by output:

Given that the code-movement, and re-ordering the `BENCHMARK(` calls doesn't change the output, this seems like a no-op?

In general, is (re-)ordering code in a `.cpp` file based on the order functions are called something we do?