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

💬 MarcoFalke commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307)
A fail-fast setting would also need to be picked up by lint-all to abort early, because this one does collect the failures.

But I won't be working on that soon, so should I just close this pull?

💬 MarcoFalke commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642175486)
How is this different from `GenTxid`?

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268148641)
We have already checked if UNIX sockets are supported.

```cpp
#if HAVE_SOCKADDR_UN
argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:' if UNIX domain sockets are supported by the platform.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
#else
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion
...

💬 dergoegge commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642196812)
> How is this different from GenTxid?

I think mostly compile-time vs. runtime checks? `GenTxid` can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. `assert(gtxid.IsWtxid())`. Also nothing prevents passing the wrong txid type when constructing a `GenTxid`.

(I guess with this PR we could consider making `GenTxid` a `std::variant<Txid, Wtxid>`)