💬 Sjors commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3516135207)
Each commit should leave the code in a valid state, so it would make sense to move 000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
(https://github.com/bitcoin/bitcoin/pull/33665#issuecomment-3516135207)
Each commit should leave the code in a valid state, so it would make sense to move 000c685bcdd395d61a58ad9698563cab41e2d7d0 up.
💬 laanwj commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3516192532)
Concept ACK.
re "refactor: Use fixed size ints over (un)signed ints for serialized values"
i wonder if we can enforce this kind of thing? That only fixed-size values can be serialized, would make sense to avoid any kind of platform divergence. At least in theory, as we already make assumptions on these types it's not a pressing issue.
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3516192532)
Concept ACK.
re "refactor: Use fixed size ints over (un)signed ints for serialized values"
i wonder if we can enforce this kind of thing? That only fixed-size values can be serialized, would make sense to avoid any kind of platform divergence. At least in theory, as we already make assumptions on these types it's not a pressing issue.
📝 AminMemariani opened a pull request: "Gpt/asmap tests"
(https://github.com/bitcoin/bitcoin/pull/33849)
## Motivation
Add direct Boost unit tests for the asmap interpreter and validator so regressions in ASN bucketing are caught earlier. Existing coverage relied on indirect behavior through addrman tests; these new cases exercise `Interpret` and `SanityCheckASMap` without large fixtures.
## Changes
- Introduce `src/test/asmap_tests.cpp` with helpers to synthesize compact asmap programs and five focused tests:
- verifies match vs default routing
- rejects tables missing a `RETURN`
- r
...
(https://github.com/bitcoin/bitcoin/pull/33849)
## Motivation
Add direct Boost unit tests for the asmap interpreter and validator so regressions in ASN bucketing are caught earlier. Existing coverage relied on indirect behavior through addrman tests; these new cases exercise `Interpret` and `SanityCheckASMap` without large fixtures.
## Changes
- Introduce `src/test/asmap_tests.cpp` with helpers to synthesize compact asmap programs and five focused tests:
- verifies match vs default routing
- rejects tables missing a `RETURN`
- r
...
💬 Sjors commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513732380)
A more thorough approach would be to introduce `typedef uint8_t HashType`, but it requires changes all over the codebase.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513732380)
A more thorough approach would be to introduce `typedef uint8_t HashType`, but it requires changes all over the codebase.
👍 TheCharlatan approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447427213)
ACK fabf8e51083120883162399293c620f2afc0c5e2
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447427213)
ACK fabf8e51083120883162399293c620f2afc0c5e2
💬 Sjors commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513754984)
I would also prefer not breaking up this loop.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513754984)
I would also prefer not breaking up this loop.
💬 Sjors commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513763886)
Since we do an early return here, it's nicer to make `sighash` (or `sig_hash`) not an optional, so below you don't need dereference pointers.
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513763886)
Since we do an early return here, it's nicer to make `sighash` (or `sig_hash`) not an optional, so below you don't need dereference pointers.
💬 Sjors commented on pull request "wallet: remove redundant sighash calculation in Musig2 signing flow":
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513769931)
Why is this added?
(https://github.com/bitcoin/bitcoin/pull/33665#discussion_r2513769931)
Why is this added?
💬 Sjors commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3516337169)
ACK fa65a57a826192ffae48f62f10c216068e8236f4
I (lightly) checked that the vectors match BIP 328 and that the test fails if I sabotage the input data.
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3516337169)
ACK fa65a57a826192ffae48f62f10c216068e8236f4
I (lightly) checked that the vectors match BIP 328 and that the test fails if I sabotage the input data.
✅ fanquake closed a pull request: "Gpt/asmap tests"
(https://github.com/bitcoin/bitcoin/pull/33849)
(https://github.com/bitcoin/bitcoin/pull/33849)
🚀 fanquake merged a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181)
(https://github.com/bitcoin/bitcoin/pull/33181)
💬 laanwj commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2513829761)
Ideally we should not use `size_t` anywhere for on-disk sizes and offsets in the first place. It's meant for counters and in-memory sizes, at most. But it's probably not worth changing all of that in places where we don't actually expect to work with large files.
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2513829761)
Ideally we should not use `size_t` anywhere for on-disk sizes and offsets in the first place. It's meant for counters and in-memory sizes, at most. But it's probably not worth changing all of that in places where we don't actually expect to work with large files.
🤔 Sjors reviewed a pull request: "doc: update interface, --stdin flag, IPC-command signtx (#31005)"
(https://github.com/bitcoin/bitcoin/pull/33765#pullrequestreview-3447533985)
Good idea to update these docs.
(https://github.com/bitcoin/bitcoin/pull/33765#pullrequestreview-3447533985)
Good idea to update these docs.
💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513826354)
Afaik the use of quotes depends on the shell environment, I don't think it's worth documenting.
More important is to point out that Bitcoin Core will use `--stdin` here, in order to support large PSBTs.
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513826354)
Afaik the use of quotes depends on the shell environment, I don't think it's worth documenting.
More important is to point out that Bitcoin Core will use `--stdin` here, in order to support large PSBTs.
💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513819248)
The term "IPC" could be confused with our multiprocess integration, maybe just use the wording from `hwi --help`:
> `--stdin Enter commands and arguments via stdin`
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513819248)
The term "IPC" could be confused with our multiprocess integration, maybe just use the wording from `hwi --help`:
> `--stdin Enter commands and arguments via stdin`
💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513829451)
The original doc is wrong, there is _only_ `signtx`, no `signtransaction`.
`signtx` and its arguments, like all other commands, can optionally be passed via `--stdin`
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2513829451)
The original doc is wrong, there is _only_ `signtx`, no `signtransaction`.
`signtx` and its arguments, like all other commands, can optionally be passed via `--stdin`
📝 fanquake opened a pull request: "depends: drop qtbase_avoid_native_float16 qt patch"
(https://github.com/bitcoin/bitcoin/pull/33850)
There is no-longer a minimum required / max supported libgcc version, after https://github.com/bitcoin/bitcoin/pull/33181.
(https://github.com/bitcoin/bitcoin/pull/33850)
There is no-longer a minimum required / max supported libgcc version, after https://github.com/bitcoin/bitcoin/pull/33181.
🤔 hebasto reviewed a pull request: "depends: drop qtbase_avoid_native_float16 qt patch"
(https://github.com/bitcoin/bitcoin/pull/33850#pullrequestreview-3447568335)
Concept ACK. I was about to open the same PR :)
(https://github.com/bitcoin/bitcoin/pull/33850#pullrequestreview-3447568335)
Concept ACK. I was about to open the same PR :)
👍 hebasto approved a pull request: "build: add `-W*-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482#pullrequestreview-3447610254)
ACK e412d6d438d600267174d1281feae3954c1c1c5b, tested on Ubuntu 25.10 using GCC 15.
(https://github.com/bitcoin/bitcoin/pull/32482#pullrequestreview-3447610254)
ACK e412d6d438d600267174d1281feae3954c1c1c5b, tested on Ubuntu 25.10 using GCC 15.
💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2513885282)
nanonit: An extra empty line?
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2513885282)
nanonit: An extra empty line?