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

💬 fanquake commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1479559438)
Imagine how much nicer would life be if we could upgrade the sanitizer infrastructure for the bitcoin consensus code without having to make some random gui sub dependecy compile with a newer version of Clang. I'll take a look. Maybe we can just drop a patch into depends.

💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1479580979)
@nopara73 I'm aware of `estimatesmartfee`'s limitations, and understand the desire for something based on current mempool composition that can react faster to current conditions. The problem is that looking your own mempool only gives a reliable result in case you're running with policies that more or less match what miners are doing, and if not, becomes easily gameable. That is the reason why `estimatesmartfee` intentionally does not use mempool composition.

As I said, I think an RPC like th
...

👍 stickies-v approved a pull request: "util: improve FindByte() performance"
(https://github.com/bitcoin/bitcoin/pull/19690)
ACK 0fe832c4a

💬 stickies-v commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#discussion_r1143742695)
Given that it's part of the interface, I think this needs to be documented on the function level so devs wanting to use FindByte know how it behaves when the byte isn't found - they shouldn't need to dive into the implementation.

💬 stickies-v commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#discussion_r1143741383)
Instead of changing the callsites of `FindByte()`, how about adding a `uint8_t` overload? I think it keeps the implementation clean, but since it can easily be argued that `uint8_t` also is a byte, this keeps the callsites straightforward and reduces the diff.

<details>
<summary>git diff</summary>

```diff
diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp
index 175564fe9..7b2e65da2 100644
--- a/src/bench/streams_findbyte.cpp
+++ b/src/bench/streams_findbyte.c
...

💬 stickies-v commented on pull request "util: improve FindByte() performance":
(https://github.com/bitcoin/bitcoin/pull/19690#discussion_r1143747292)
nit
```suggestion
// The modulus operation is much more expensive than byte
// comparison and addition, so we keep it out of the loop to
// improve performance (see #19690 for discussion).
```

💬 hebasto commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1479583846)
Friendly ping @glozow :)

💬 willcl-ark commented on pull request "github: Switch to yaml issue templates":
(https://github.com/bitcoin/bitcoin/pull/27025#discussion_r1144830646)
@MarcoFalke so it seems that it is not possible to move this particular entry (nor the bottom two) to the top... Apparently we can re-order our custom templates by changing the filenames to `01_xxx`, `02_xxx` etc., but the two bottom entries, and the security report are just part of the GitHub UI :(

There is a GitHub beta setting to enable the `Report a Vulnerability` big green button for _all_ users (changing it from a duplicate `View Policy` big green button), as I've done now for [my fork]
...

📝 willcl-ark opened a pull request: "build: debug enable addrman consistency checks"
(https://github.com/bitcoin/bitcoin/pull/27300)
The `--enable-debug` flag should default to enabling debug checks, of which
`addrman` consitency checking is one. This PR enables that behaviour.

To opt out of `addrman` consistency checks when running a debug build use the
`-checkaddrman=0` runtime option for `bitcoind` as described in `developer-notes.md`.

fixes #24709

💬 dergoegge commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1479606895)
Concept ACK