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

💬 hebasto commented on pull request "depends: fontconfig 2.14.2":
(https://github.com/bitcoin/bitcoin/pull/27301#issuecomment-1479618243)
Concept ACK.

💬 fanquake commented on pull request "depends: fontconfig 2.14.2":
(https://github.com/bitcoin/bitcoin/pull/27301#issuecomment-1479622034)
Looks like we can't drop the patch, as we'd still end up needing gperf, however just adapting our current patch to the new code also doens't build, so we'll need to do more there. In any case, even if fontconfig builds, Qt doesn't work (out of the box) with clang-16, so this seems like a waste of time.

💬 MarcoFalke commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#discussion_r1144866456)
```suggestion
static constexpr int32_t DEFAULT_ADDRMAN_CONSISTENCY_CHECKS{
#ifdef DEBUG_ADDRMAN
1
#else
0
#endif
};
```

style nit (feel free to ignore, if you don't like it)

💬 dergoegge commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1479628226)
Could we drop the GUI from the TSan task?

💬 instagibbs commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144872917)
Hmmm, traced out a package size of 4, looks like I'm referencing non-existent outputs, not re-using them(maybe both :) ).

Clearly this is confusing and broken either way.

To take a step back, after doing this I realized https://github.com/bitcoin/bitcoin/blob/master/src/bench/mempool_stress.cpp exists. I'm fine if this doesn't get merged if it doesn't add anything new? Helped me learn about the benchmarks if nothing else. Thoughts?