💬 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

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268054016)
ok will assert so future failure on regression is easy to see

💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-1642068632)
Rebased. Still based on https://github.com/bitcoin/bitcoin/pull/26567.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268066532)
Hm well, that'll certainly be faster but then we can't verify a successful reindex in the new run-as-root path. Size shouldn't be an issue though because no assumptions are made about it, the test calculates how much data it needs to generate. Let me know what you think about this again after the next push

💬 furszy commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1268077213)
> I mean `ComputeChangeParams` computes some of `CoinSelectionParams`, but it's not immediately obvious which ones without reading the code of the function

Ok. Added docs.
The function calculates all the `CoinSelectionParams` fields related to 'change'. No exceptions.

🤔 furszy reviewed a pull request: "wallet: don't duplicate change output if already exist"
(https://github.com/bitcoin/bitcoin/pull/27601#pullrequestreview-1537117279)
Updated per feedback. Thanks.

Added docs for the `ComputeChangeParams` new function.

📝 furszy converted_to_draft a pull request: "wallet: coverage for receiving txes with same id but different witness data"
(https://github.com/bitcoin/bitcoin/pull/25909)
Based on #11240 context, adding test coverage for the behavior introduced in #11225 and to the current wallet limitations.

This is the first step towards adding the ability to store multiple transactions with same tx id but different witness data in the wallet. Verifying and testing the current behavior before introducing the new features.

The following cases are covered:

1) Two p2wpkh spending transactions with the same hash are received:
The first one with segwit data stripped, and t
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268106550)
In the latest code it is:

```cpp
socklen_t len = sizeof(addrun);

if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
```

feel free to ditch the `len` variable and use `sizeof(addrun)` directly when calling `ConnectToSocket()`.

💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268107378)
I wouldn't use hardcode default values here, but instead use the globals (e.g. `DEFAULT_TXRECONCILIATION_ENABLE`), and update the logic in `peerman_args.cpp` to only override Options members _if_ they are set as cli args.