💬 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

📝 fanquake opened a pull request: "depends: fontconfig 2.14.2"
(https://github.com/bitcoin/bitcoin/pull/27301)
We need to bump this as the current version doesn't compile under `clang-16`, which is blocking upgrading sanitizer/fuzzing infrastructure (see #27298).

Untested. Need to double-check the gperf/patch dropping.