💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1480921094)
Concept ACK

The code as of 8bdea6316c looks ok, but it seems to be mixing the fix with an optimization to only parse once (instead of every time a parameter is retrieved). It would be better to have two separate commits - one with the non-functional optimization and one with the fix.

Idea: I think it would be better to disallow the creation of `HTTPRequest` if the request is invalid/unparsable. This would avoid having a `nullptr` member `m_uri_parsed` and having to handle the nullness. Wou
...

🚀 fanquake merged a pull request: "refactor: Replace GetTimeMicros by SystemClock"
(https://github.com/bitcoin/bitcoin/pull/27233)

💬 fanquake commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1145966325)
@john-moffett want to open a follow up?

📝 fanquake opened a pull request: "depends: qrencode 4.1.1"
(https://github.com/bitcoin/bitcoin/pull/27312)
Upgrade to the latest qrencode, and disable some warnings that cause compile failures with newer compilers (clang-15+).

I haven't tested this (from a GUI perspective) at all. This is just "good enough" to keep things compiling, and uses some similar work-arounds as we have with other older packages, i.e bdb.

Note that upstream, libqrencode is effectively unmaintained. No code changes for > 2 years. No responses to issues/PRs. Seems like the author has mostly dropped off of GitHub as well.
...

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145996614)
That will work also, thanks @josibake

💬 MarcoFalke commented on pull request "depends: fontconfig 2.14.2":
(https://github.com/bitcoin/bitcoin/pull/27301#issuecomment-1480974024)
unrelated: on master it looks like @DrahtBot can't build for Windows:


```
INFO: Building 6e69fead2baf for platform triple x86_64-w64-mingw32:
...using reference timestamp: 1679478521
...running at most 4 jobs
...from worktree directory: '/root/temp/scratch/guix/bitcoin/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/root/temp/scratch/guix/bitcoin/bitcoin/guix-build-6e69fead2baf/distsrc-6e69fead2baf-x86_64-w64-mingw32'

...

💬 MarcoFalke commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1146019853)
gmtime can also be removed once we have C++20, but not sure if and when that happens

💬 willcl-ark commented on issue "tracepoints: gnu-zero-variadic-macro-arguments warnings":
(https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1480997053)
This is still present with https://github.com/bitcoin/bitcoin/pull/26593 as it also uses an underlying variadic macro with 0 arguments via `_SDT_ASM_BODY` -> `_SDT_PROBE` -> `_SDT_PROBE_N` -> `STAP_PROBEV` -> `TRACEPOINT`.

I tried to directly call the `STAP_PROBE1` macro for the single argument case (on top of #26593), which would mean that all our remaining `TRACEPOINT` macros were invoked with >= 1 argument:

```cpp
// A USDT tracepoint with one argument.
#define TRACEPOINT1(context, ev
...

💬 dergoegge commented on pull request "ci: Use clang-15 in "tidy" task":
(https://github.com/bitcoin/bitcoin/pull/27311#issuecomment-1480997560)
clang-16 was released recently, any reason not to switch to that directly while we're at it?

💬 fanquake commented on pull request "depends: fontconfig 2.14.2":
(https://github.com/bitcoin/bitcoin/pull/27301#issuecomment-1481002005)
> unrelated: on master it looks like @DrahtBot can't build for Windows:

Can @DrahtBot do a `guix pull` and try buildign again? iirc this is a bug in Guix that I've seen previously

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

I tested this by making sure the `unittests` run and also modifying `test/functional/wallet_fast_rescan.py` to use `address_to_scriptpubkey`, per @theStack 's suggestion. Works great!

I'd love to see a follow-up PR targeting areas where we are a) only testing base58 addresses but should be testing bech32 as well and b) anywhere we are making a `getaddressinfo` RPC call to get the spk to inste
...

💬 fanquake commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1146040976)
Seems kind of weird to change the CLI to use a "test-only" RPC in any case

💬 MarcoFalke commented on pull request "ci: Use clang-15 in "tidy" task":
(https://github.com/bitcoin/bitcoin/pull/27311#issuecomment-1481020371)
lgtm ACK 8fe27fbed85311bbdcd75136ca13370d368a6f2f

> clang-16 was released recently, any reason not to switch to that directly while we're at it?

Sounds good to do this, once iwyu has a release with clang-16 support. Maybe in a follow-up?

💬 hebasto commented on pull request "ci: Use clang-15 in "tidy" task":
(https://github.com/bitcoin/bitcoin/pull/27311#issuecomment-1481022908)
> clang-16 was released recently, any reason not to switch to that directly while we're at it?

Well, it is available as a package in Ubuntu Lunar, but I'm not sure if we want to bring a short-lived distro into our CI.

💬 hebasto commented on pull request "depends: qrencode 4.1.1":
(https://github.com/bitcoin/bitcoin/pull/27312#issuecomment-1481028288)
Concept ACK.

> Note that upstream, libqrencode is effectively unmaintained. No code changes for > 2 years. No responses to issues/PRs. Seems like the author has mostly dropped off of GitHub as well.

Yeah, this situation is a real concern, unfortunately.

💬 darosior commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1146013592)
Making sure i got this one right. `use_anti_fee_sniping` may only be `false` if this is called from `FundTransaction`. And `FundTransaction` diregards the value of the `nLockTime` that may be set here anyways.

💬 darosior commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1481033569)
People interested in seeing this move forward may be interested in giving https://github.com/bitcoin/bitcoin/pull/25273 a look. It was recently rebased and reviewed twice.

💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1146057675)
The `_POSIX_C_SOURCE` should be moved here too.

💬 jamesob commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481055451)
I probably won't have time in the next few days to attend to this, so feel free to open a replacement PR with @achow101's branch if I'm holding anything up.

💬 theStack commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1146099889)
@josibake: yes, agree that considering on what we need in `address_to_scriptpubkey` returning `(None, None)` is better than asserting. LGTM!

👍 theStack approved a pull request: "test: Support decoding segwit address in address_to_scriptpubkey()"
(https://github.com/bitcoin/bitcoin/pull/27269)
ACK d178082996dc3000f42816f89afcf3fa4d31e159 ✔️

> I'd love to see a follow-up PR targeting areas where we are a) only testing base58 addresses but should be testing bech32 as well and b) anywhere we are making a getaddressinfo RPC call to get the spk to instead use address_to_scriptpubkey

👍 If anyone wants to work on this, feel free to tag me as reviewer.