💬 MarcoFalke commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#issuecomment-1479385710)
@fanquake See https://github.com/bitcoin/bitcoin/issues/18623 ?

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1144656973)
Thanks for the thorough review Josi.
I will fix that.
This means I have to check in `bech32_to_bytes `also for a None version value after so as not to call bytearray(payload) with None.

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144661496)
Put the declaration in the header (eg `extern const std::string MAIN;`) and the definition in the corresponding cpp file (`const std::string MAIN{"main"};`)

📝 MarcoFalke opened a pull request: "test: Remove unused Check* default constructors"
(https://github.com/bitcoin/bitcoin/pull/27297)
They are no longer needed after the removal of `swap`, see https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693

Also, flatten a redundant `if` check.

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144667808)
Adding/moving headers is usually pretty benign -- if you have an extra header it just makes compilation slightly slower and can be caught by iwyu, if you miss a header, it just won't compile. But changing code is where security bugs can sneak in, and if you're touching 100 lines all over the place, having an automated check that nothing's going wrong seems like a big win.

💬 darosior commented on pull request "wallet: Migrate non-HD keys with single combo containing a list of keys":
(https://github.com/bitcoin/bitcoin/pull/26627#issuecomment-1479412726)
Wouldn't #20018 address most performance concerns of having a large number of `DescriptorSPKM`s? Albeit at the cost of a slightly higher memory footprint, especially in this case.

👍 hebasto approved a pull request: "test: Remove unused Check* default constructors"
(https://github.com/bitcoin/bitcoin/pull/27297)
ACK fae349076db03ddfbf23c5d828368d538b5d52d5

📝 MarcoFalke opened a pull request: "ci: Use TSan v2 (llvm-16, take 3)"
(https://github.com/bitcoin/bitcoin/pull/27298)
The previous two attempts failed:
* llvm-14: Failed in https://github.com/bitcoin/bitcoin/pull/24572
* llvm-15: Failed in https://github.com/bitcoin/bitcoin/pull/26775

However, now that the bug is known and fixed, it should be good to go. See also https://github.com/bitcoin/bitcoin/pull/26775#issuecomment-1380590669

💬 willcl-ark commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1479417856)
@MarcoFalke, I've implemented this in a branch but don't feel I understand the rationale for the change fully to open a PR yet. Is the idea that (mainly devs) running after building with `--enable-debug` would have this running by default to potentially catch consistency issues e.g. on mainnet?

(Otherwise it seems to me that we could just enable it via the flag in the two Ci tests which build in debug mode, native QT5 and i686 multiprocess)

💬 MarcoFalke commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1479453937)
> Is the idea that ...

Yes.

Otherwise it seems odd to have an `--enable-debug` setting that doesn't enable debug checks.

Maybe the `--enable-debug` setting can be made more clear that this enables stricter checks, which could terminate the program or throw additional errors, that otherwise wouldn't appear. See also https://github.com/bitcoin/bitcoin/issues/26338#issuecomment-1284320019

💬 MarcoFalke commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144709977)
> ... in the same commit. This makes it easier to see that for every changed file the new header is included as well.

For reviewers it is possible to squash two commits for review, if they believe it makes it easier for them to review.

💬 dergoegge commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1144723993)
I was making a copy here of the `CNetMessage`, fixed now!

Easy to review with `git range-diff 60c3f4918190900e5f79341abcc0878214656257...3566aa7d495bb783bbd135726238d9f2a9e9f80e`.

💬 MarcoFalke commented on pull request "github: Switch to yaml issue templates":
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1144724516)
If there is a duplicate entry, at least it could be moved to the first position, as some people seem to miss it?

⚠️ MarcoFalke opened an issue: "depends does not compile with clang-16 (fontconfig)"
(https://github.com/bitcoin/bitcoin/issues/27299)
Steps to reproduce on a fresh install of Ubuntu Lunar:

`export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && apt install libc++abi-16-dev libc++-16-dev clang-16 llvm-16 build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq make automake cmake curl g++-multilib libtool binutils bsdmainutils pkg-config python3 patch
...

💬 MarcoFalke commented on issue "depends does not compile with clang-16 (fontconfig)":
(https://github.com/bitcoin/bitcoin/issues/27299#issuecomment-1479487296)
Same for qrencode:

```
# ( cd depends && make CC=clang-16 CXX="clang++-16" qrencode )
Building qrencode...
make[1]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-f8b44fab047'
make all-recursive
make[2]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-f8b44fab047'
Making all in .
make[3]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-f8b44fab047'
/bin/bash ./lib
...

💬 MarcoFalke commented on issue "depends does not compile with clang-16 (fontconfig)":
(https://github.com/bitcoin/bitcoin/issues/27299#issuecomment-1479492752)
The qrencode issue also with clang-15:


```
# ( cd depends && make CC=clang-15 CXX="clang++-15" qrencode )
Building qrencode...
make[1]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-ba1c2aba6fa'
make all-recursive
make[2]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-ba1c2aba6fa'
Making all in .
make[3]: Entering directory '/bitcoin-core/depends/work/build/x86_64-pc-linux-gnu/qrencode/3.4.4-ba1c2ab
...

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1144759378)
there should be a space between function arguments and I'd return a tuple here to stay consistent with the other functions in the `test_framework/segwit_addr.py` file:

```suggestion
version, payload = decode_segwit_address(hrp, address)
if version is None:
return (None, None)
```

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1144777555)
:+1:

💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1479540400)
ACK https://github.com/bitcoin/bitcoin/commit/9e377f481bd0496d868ea0b21f53d2c9bd0ef85a

nice work!

💬 ryanofsky commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#issuecomment-1479542166)
I believe these names belong in the util library, not the kernel library. They are used by bitcoin-cli, and bitcoin-cli should not depend on the kernel because is an RPC client that should not contain validation code.