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

💬 fanquake commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1458200190)
Does anyone have a link to an upstream issue? Is this a regression in GCC 11, that has been fixed in 12/13?

💬 MarcoFalke commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1458210175)
It is not fixed in gcc-12, see https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456215491. gcc-13 exists, so it may be possible to test. I have not attempted to reduce the test case, nor did I dig deeper to find out if this is a compiler bug or bug in our code, because I don't think we have a choice here: If there are two equivalent versions of the same code, but one doesn't compile, we should pick the one that compiles.

💬 MarcoFalke commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1458213858)
To clarify, even if the workaround is not needed in gcc-13 or gcc-14, I think we shouldn't revert it. I see it as a permanent workaround.

💬 fanquake commented on pull request "util: Work around ParseHex gcc cross compiler bug":
(https://github.com/bitcoin/bitcoin/pull/27218#issuecomment-1458234945)
> gcc-13 exists, so it may be possible to test.

Still broken in GCC 13.0.1. `riscv64-linux-gnu-g++-13 13.0.1 20230215`

🚀 fanquake merged a pull request: "util: Work around ParseHex gcc cross compiler bug"
(https://github.com/bitcoin/bitcoin/pull/27218)

💬 fanquake commented on pull request "Add wallet method to detect if a key is "fresh"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1127978416)
https://cirrus-ci.com/task/6502838134636544:
```bash
wallet/test/scriptpubkeyman_tests.cpp:142:48: error: argument name 'index' in comment does not match parameter name 'pos' [bugprone-argument-comment,-warnings-as-errors]
descriptor.descriptor->ExpandFromCache(/*index=*/10, descriptor.cache, scripts3, provider);
^
./script/descriptor.h:137:38: note: 'pos' declared here
virtual bool ExpandFromCache(int pos, const DescriptorCache& r
...

💬 fanquake commented on pull request "Add wallet method to detect if a key is "fresh"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1458300719)
https://github.com/bitcoin/bitcoin/pull/27216/checks?check_run_id=11804958852
```bash
Assertion failed: lock cs_wallet not held in ./wallet/wallet.h:444; locks held:
'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1185 (in thread 'test')
```

💬 fanquake commented on pull request "wallet: 26032 followups":
(https://github.com/bitcoin/bitcoin/pull/27180#issuecomment-1458303075)
> Followups for https://github.com/bitcoin/bitcoin/pull/26032. So far nothing major.

Are you expecting there to be more? I really only see the one followup comment in #26032.