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

🤔 furszy reviewed a pull request: "init: return error when block index is non-contiguous, fix feature_init.py file perturbation"
(https://github.com/bitcoin/bitcoin/pull/27823#pullrequestreview-1535880019)
Code ACK d27b9a224

💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1267289758)
Maybe could randomize (or restructure) this a bit in a follow-up. Perturbing different parts of the block content might let us detect and add coverage for different errors.

💬 furszy commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1268112906)
nit: couldn't this be copied outside of the for-loop only once?

💬 jamesob commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806)
Only downside with this is that CI won't report all lint failures, rather will bail on the first one. Not sure if this is going to be annoying for people. Maybe we could have a `FAIL_FAST` envvar that gates this, and right now is only enabled in the container entrypoint script?

💬 fanquake commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1642153266)
> So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?

We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.

> (I mean, obviously my preference would be to use clang for Windows cross-builds,

I wouldn't be opposed to that. Although note that in tha
...

📝 dergoegge opened a pull request: "rfc: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107)
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.

This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.

(Only t
...