💬 S3RK commented on pull request "wallet: Turn `destdata` entries into `CAddressBookData` fields":
(https://github.com/bitcoin/bitcoin/pull/27215#issuecomment-1457734909)
Concept ACK

💬 MarcoFalke commented on pull request "wallet: Turn `destdata` entries into `CAddressBookData` fields":
(https://github.com/bitcoin/bitcoin/pull/27215#discussion_r1127555196)
nit: Returning a const value is not possible. You can either return a (mutable) value or a const reference. See also:


`�[1m/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./wallet/wallet.h:230:5: �[0m�[0;1;31merror: �[0m�[1mreturn type 'const std::optional<std::pair<int64_t, std::string>>' (aka 'const optional<pair<long, basic_string<char>>>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const
...

💬 fanquake commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1457806376)
ACKs here, but these two comments should be addressed:
> assuming this shouldn't use emplace_back https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784 or avoid temporary vector allocations https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123429117.

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127561494)
nit:

```
test/functional/wallet_labels.py:76:132: E703 statement ends with a semicolon

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127566183)
```suggestion
// Any other string is a programming error or disk corruption
```

nit: This could also happen on disk corruption, no?

💬 MarcoFalke commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1127564320)
```suggestion
} // no default case so the compiler will warn when a new enum is added

```

nit: typo

💬 fanquake commented on issue "Issue in `p2p_ibd_stalling.py` under Valgrind":
(https://github.com/bitcoin/bitcoin/issues/27208#issuecomment-1457879348)
> What did you set the timeout factor to?

I didn't change it at all. Will get you a log.

📝 MarcoFalke opened a pull request: "util: Work around ParseHex gcc cross compiler bug"
(https://github.com/bitcoin/bitcoin/pull/27218)
I fail to see how an explicit `ParseHex` template instantiation fails to also instantiate `TryParseHex`.

Nonetheless, to work around a compiler bug, change the explicit instantiation from `ParseHex` to `TryParseHex`. (`ParseHex` is inline anyway and will be instantiated by the compiler either way).

Fixes https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757 :

```
CXXLD bitcoind
/usr/lib/gcc-cross/powerpc64le-linux-gnu/11/../../../../powerpc64le-linux-gnu/bin/ld: l
...

💬 MarcoFalke commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1457941463)
To test on a fresh install of Ubuntu/Debian with gcc-11 or later: https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456215491

💬 MarcoFalke commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1457953182)
> I guess it's having issues with the template for the std::optional<std::vector<uint8_t>> instantiation, since it's not available to the linker?

It might be a compiler bug where the compiler skips the explicit instantiation of a template that has a function body (and thus is already required to be implicitly instantiated)?

> Adding explicit instantiations should fix it, I think:

Thanks. Done something like that in #https://github.com/bitcoin/bitcoin/pull/27218

> Alternatively, I su
...

⚠️ 1440000bytes opened an issue: "`combinepsbt` RPC does not work with P2TR inputs"
(https://github.com/bitcoin/bitcoin/issues/27219)
[`combinepsbt`](https://bitcoincore.org/en/doc/24.0.0/rpc/rawtransactions/combinepsbt/) allows to distribute identical PSBTs to peers for signature and consolidate the resulting PSBT. However, this does not work with taproot UTXOs for me.

**Expected behavior**

Combined PSBT should return `'complete': True` if all inputs are signed in different PSBTs and finalized with `finalizepsbt`

**Actual behavior**

Unable to finalize the transaction that was combined from multiple signed PSBTs.

...

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1458015010)
Concept ACK.

This was somewhere on my TODO, thanks for picking it up!

> The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration t
...

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127730689)
```suggestion
const auto& address = addrman.Select(/*new_only=*/false, NET_I2P);
```

💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1127731998)
Yes, can leave this way. This suggestion I made while reviewing the first commit but considering the whole context, you can leave it.

💬 MarcoFalke commented on pull request "ci: Use clang-15 and IWYU v0.19 in "tidy" task":
(https://github.com/bitcoin/bitcoin/pull/26766#discussion_r1127740280)
Another alternative would be to use https://apt.llvm.org/

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1458076661)
Added myself as co-author to some commits per invitation of @glozow

👍 hebasto approved a pull request: "util: Work around ParseHex gcc cross compiler bug"
(https://github.com/bitcoin/bitcoin/pull/27218)
ACK fa8481b05fae6a7e44ca526d930993f02549234e, tested on Ubuntu 22.04, gcc 11.3 for the `riscv64-linux-gnu` host.,

💬 furszy commented on pull request "wallet: coin selection, don't return results that exceed the max allowed weight":
(https://github.com/bitcoin/bitcoin/pull/26720#issuecomment-1458118175)
rebased, conflicts solved.

👍 gruve-p approved a pull request: "util: Work around ParseHex gcc cross compiler bug"
(https://github.com/bitcoin/bitcoin/pull/27218)

💬 gruve-p commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1458151432)
ACK https://github.com/bitcoin/bitcoin/pull/27218/commits/fa8481b05fae6a7e44ca526d930993f02549234e

Tested cross compiling through depends on Ubuntu Jammy (22.04) with GCC 11 on these hosts:
`i686-pc-linux-gnu`
`x86_64-pc-linux-gnu`
`x86_64-w64-mingw32`
`x86_64-apple-darwin`
`arm64-apple-darwin`
`arm-linux-gnueabihf`
`aarch64-linux-gnu`
`powerpc64-linux-gnu`
`powerpc64le-linux-gnu`
`riscv32-linux-gnu`
`riscv64-linux-gnu`
`s390x-linux-gnu`