🚀 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?
💬 fanquake commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2513914592)
Dropped.
(https://github.com/bitcoin/bitcoin/pull/32482#discussion_r2513914592)
Dropped.
👍 hebasto approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447658512)
ACK fabf8e51083120883162399293c620f2afc0c5e2.
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447658512)
ACK fabf8e51083120883162399293c620f2afc0c5e2.
🤔 janb84 reviewed a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447665061)
lgtm ACK fabf8e51083120883162399293c620f2afc0c5e2
With the note added I can't think of a reason not to bump the g++ version to version 12.
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3447665061)
lgtm ACK fabf8e51083120883162399293c620f2afc0c5e2
With the note added I can't think of a reason not to bump the g++ version to version 12.
👍 hebasto approved a pull request: "build: add `-W*-whitespace`"
(https://github.com/bitcoin/bitcoin/pull/32482#pullrequestreview-3447669440)
re-ACK 40dcbf580d8eb31a067b62bf9676099919b9841e.
(https://github.com/bitcoin/bitcoin/pull/32482#pullrequestreview-3447669440)
re-ACK 40dcbf580d8eb31a067b62bf9676099919b9841e.
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3516508624)
Rebased to resolve a conflict with the merged bitcoin/bitcoin#33181.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3516508624)
Rebased to resolve a conflict with the merged bitcoin/bitcoin#33181.
👍 l0rinc approved a pull request: " test: [refactor] Use reference over ptr to chainman "
(https://github.com/bitcoin/bitcoin/pull/33840#pullrequestreview-3447477940)
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3
I left some nits for the second commit, but I'm fine merging as is:
* `mtype` param can be `std::string_view`
* `mtype.size()` should be documented to be less or equal to the available space
* `std::ranges::copy` would simplify the copy and clarify that we jump more than the number of inserted bytes before, something like
* to document the leading `0` (the `1 + ` part of the offsets) we could redundantly add populate the first byte as well.
...
(https://github.com/bitcoin/bitcoin/pull/33840#pullrequestreview-3447477940)
ACK 7a4901c9029687ad2b37f6d929d4c8fe96c15db3
I left some nits for the second commit, but I'm fine merging as is:
* `mtype` param can be `std::string_view`
* `mtype.size()` should be documented to be less or equal to the available space
* `std::ranges::copy` would simplify the copy and clarify that we jump more than the number of inserted bytes before, something like
* to document the leading `0` (the `1 + ` part of the offsets) we could redundantly add populate the first byte as well.
...
💬 l0rinc commented on pull request " test: [refactor] Use reference over ptr to chainman ":
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2513773820)
To clarify that the second one doesn't actually copy where the first left off:
```suggestion
std::ranges::copy(mtype, contents.begin() + 1);
std::ranges::copy(payload, contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
```
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2513773820)
To clarify that the second one doesn't actually copy where the first left off:
```suggestion
std::ranges::copy(mtype, contents.begin() + 1);
std::ranges::copy(payload, contents.begin() + 1 + CMessageHeader::MESSAGE_TYPE_SIZE);
```
💬 l0rinc commented on pull request " test: [refactor] Use reference over ptr to chainman ":
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2513944363)
We should mention that the fixed-size message is right-padded with zeros:
```suggestion
// Layout: [0x00][12-byte message type, zero padded on right][payload]
```
(https://github.com/bitcoin/bitcoin/pull/33840#discussion_r2513944363)
We should mention that the fixed-size message is right-padded with zeros:
```suggestion
// Layout: [0x00][12-byte message type, zero padded on right][payload]
```
💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516517123)
Friendly ping @willcl-ark @m3dwards @davidgumberg @hodlinator :)
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516517123)
Friendly ping @willcl-ark @m3dwards @davidgumberg @hodlinator :)
💬 fanquake commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516522272)
> You https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480170953 was parsed by @DrahtBot
Concept ACK. Should probably be based on #33775.
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516522272)
> You https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480170953 was parsed by @DrahtBot
Concept ACK. Should probably be based on #33775.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3516542540)
`29ef3c62de...875795ef0f`: bugfix: transactions to be broadcast are sorted by priority (number of times broadcast, last broadcast timestamp). The timestamp should only be compared for transactions that have been broadcast the same number of times. The previous code would compare `(3, 100) < (5, 90)` and `(5, 90) < (3, 100)` which was nonsensical :face_with_head_bandage:
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3516542540)
`29ef3c62de...875795ef0f`: bugfix: transactions to be broadcast are sorted by priority (number of times broadcast, last broadcast timestamp). The timestamp should only be compared for transactions that have been broadcast the same number of times. The previous code would compare `(3, 100) < (5, 90)` and `(5, 90) < (3, 100)` which was nonsensical :face_with_head_bandage: