📝 hebasto opened a pull request: "ci: Use clang-15 in "tidy" task"
(https://github.com/bitcoin/bitcoin/pull/27311)
Newer tools usually are better in terms of features and bug fixes.
Requested in https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1440230390.
Split from https://github.com/bitcoin/bitcoin/pull/26766.
(https://github.com/bitcoin/bitcoin/pull/27311)
Newer tools usually are better in terms of features and bug fixes.
Requested in https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1440230390.
Split from https://github.com/bitcoin/bitcoin/pull/26766.
💬 hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1480832579)
> I don't know much about how the CI works, maybe it's possible to install a newer clang version? I'm using 15.0.7
Done in https://github.com/bitcoin/bitcoin/pull/27311.
Making this PR a draft for now.
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1480832579)
> I don't know much about how the CI works, maybe it's possible to install a newer clang version? I'm using 15.0.7
Done in https://github.com/bitcoin/bitcoin/pull/27311.
Making this PR a draft for now.
📝 hebasto converted_to_draft a pull request: "clang-tidy: Add more `performance-*` checks and related fixes"
(https://github.com/bitcoin/bitcoin/pull/26642)
null
(https://github.com/bitcoin/bitcoin/pull/26642)
null
💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145886593)
```suggestion
version, payload = bech32_to_bytes(address)
```
@josibake this will call will assert to false if the address is a base58 address.
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145886593)
```suggestion
version, payload = bech32_to_bytes(address)
```
@josibake this will call will assert to false if the address is a base58 address.
💬 MarcoFalke commented on pull request "Remove almost all blockstorage globals":
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1145910136)
> (also: https://github.com/llvm/llvm-project/issues/56195)
Thanks, fixed in https://github.com/llvm/llvm-project/commit/8c10256734cd47274671fcabe94f24f15ecd6209
(https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1145910136)
> (also: https://github.com/llvm/llvm-project/issues/56195)
Thanks, fixed in https://github.com/llvm/llvm-project/commit/8c10256734cd47274671fcabe94f24f15ecd6209
💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145918186)
`bech32_to_bytes` will be updated to handle all segwit addresses and check if hrp is valid.
```python
def bech32_to_bytes(address):
hrp = address.split('1')[0]
assert hrp in ['bc', 'tb', 'bcrt'], f"hrp should be 'bc', 'tb', or 'bcrt', not '{hrp}'"
version, payload = decode_segwit_address(hrp, address)
if version is None:
return (None, None)
return version, bytearray(payload)
```
So calling address_to_scriptpubkey with a base58 address will assert to false,
...
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145918186)
`bech32_to_bytes` will be updated to handle all segwit addresses and check if hrp is valid.
```python
def bech32_to_bytes(address):
hrp = address.split('1')[0]
assert hrp in ['bc', 'tb', 'bcrt'], f"hrp should be 'bc', 'tb', or 'bcrt', not '{hrp}'"
version, payload = decode_segwit_address(hrp, address)
if version is None:
return (None, None)
return version, bytearray(payload)
```
So calling address_to_scriptpubkey with a base58 address will assert to false,
...
💬 josibake commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145925672)
ah, i see. I think id approach this in a different way: if the HRP doesn't match a bech32 HRP, return `(None, None)`. that way, `address_to_scriptpubkey` can continue and try base58 next. if the address is indeed malformed (meaning not bech32 or base58), eventually `address_to_scriptpubkey` will return false.
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1145925672)
ah, i see. I think id approach this in a different way: if the HRP doesn't match a bech32 HRP, return `(None, None)`. that way, `address_to_scriptpubkey` can continue and try base58 next. if the address is indeed malformed (meaning not bech32 or base58), eventually `address_to_scriptpubkey` will return false.
💬 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
...
(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)
(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?
(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.
...
(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
(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'
...
(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
(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
...
(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?
(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
(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
...
(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
(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?
(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?