💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1641941150)
> My suggestion, for future reference:

Thank you, will have a look.

📝 MarcoFalke converted_to_draft a pull request: "ci: Use qemu-user through container engine"
(https://github.com/bitcoin/bitcoin/pull/28087)
Currently the CI containers always run on the host architecture, and only wrap `bitcoind` into `qemu-user` when needed. This has many issues:

* The `i386` tasks can not be run on non-x86 hosts.
* `config.guess` isn't present when building the CI image, which is fine. But it prints a warning, see https://github.com/bitcoin/bitcoin/pull/27739#pullrequestreview-1446580353
* The python tests are run on the host architecture, making it harder to find architecture specific bugs. See for example h
...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1267962992)
thanks, that is leftover from older version

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1267964112)
sure thanks

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1641960701)
> this unexpected behavior causes Bitcoin Core to log way too many messages.

The log flooding rate is very high, but perhaps the more severe issue fixed here is that the I2P connections remain down until the router is manually stopped and then restarted, during which time the log flooding continues, and for anyone running `onlynet=i2p`, it would mean their node is effectively offline until they notice it and take action.

With this fix, the connections recover on their own, like they do for
...

💬 jonatack commented on pull request "validation: use noexcept instead of deprecated throw()":
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1641966462)
Post-merge ack

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1267981369)
done thanks. agreed about the smell. lets lock down the expected behavior with tests and then we can clean up in future PR.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1267987502)
As discussed offline, what I meant with "leave it as is" is to not move banman in this PR, i.e. reverting the change in this PR.

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#issuecomment-1641982667)
Rebased.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1267996774)
To summarize offline discussion: `CNode` [uses](https://github.com/bitcoin/bitcoin/blob/78a983f597af224e017f522443112cec81422fe6/src/net.h#L556) move semantics for this, but as you pointed out we construct many more `CNode` instances than we do `PeerManager`s. I'm not sure if the decreased readability trade-off is worth the benefits/sticking to best practices, so happy to have this marked as resolved.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268004838)
Removed banman from the options on latest push

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1641997568)
https://geti2p.net/en/download has just updated their available macOS version from 1.9 to 2.3 -- will retest.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268013582)
What's the rationale for using `m_node.banman.get()` instead of `nullptr` or instead of the locally constructed `banman.get()`? This comment applies to all 4 changes in this file.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268014256)
Probably no need to switch the parameter placement of `banman`?

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268016035)
yeah agreed, will update

👍 vasild approved a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1536990151)
ACK c69a74229da514228d3388e9652d2ea2e89224d7

Good changes, easy to review. Thanks!

💬 vasild commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268034069)
This is ok as it is and I do not insist to change it. My personal preference, feel free to ignore it, is to separate declaration (interface) from definition (implementation). This makes the former easier to read by an user who only needs to call the interface without being bothered with implementation details.

Inlining is desirable for performance purposes, does this change have any impact on the performance?

Both out-of-line definition and inlining are achievable:

```cpp
class CNetAdd
...

💬 vasild commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268010664)
For remote peers we really need to combine the `IsPrivacyNet()` call with `|| peer.m_inbound_onion` which looks easy to miss in the future, what about introducing also `CNode::IsPrivacyNet()` which does `return addr.IsPrivacyNet() || m_inbound_onion;`? Or at least a mention in the `CNetAddr::IsPrivacyNet()` doc/comment.

📝 MarcoFalke opened a pull request: "ci: Set DEBUG=1 for valgrind tasks"
(https://github.com/bitcoin/bitcoin/pull/28106)
Currently this is only done by OSS-Fuzz. As a fallback add it to the two valgrind tasks as well.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268045442)
That was a mistake on my side. Reverted in the latest push