💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(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.

👍 hebasto approved a pull request: "build: Bump g++ minimum supported version to 12"
(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.

👍 hebasto approved a pull request: "build: add `-W*-whitespace`"
(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.

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


...

💬 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);
```

💬 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]
```

💬 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 :)

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

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

👍 laanwj approved a pull request: "refactor: Return uint64_t from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/33724#pullrequestreview-3447733525)
Code review ACK fa6c0bedd33ac7ad27454adaf9522fd27bef6ea3
Changes LGTM
- `test: Remove outdated comment`: trivial, no risk
- `refactor: [rpc] Remove cast when reporting serialized size`: obvious ACK. probably was a workaround for an old univalue version
- `move-only: Move CBlockFileInfo to kernel namespace`: good catch! verified move-only
- `refactor: Use fixed size ints over (un)signed ints for serialized values`: long-overdue change. There should be no difference in generated code (but i did no
...

💬 fanquake commented on pull request "build: Bump g++ minimum supported version to 12":
(https://github.com/bitcoin/bitcoin/pull/33842#issuecomment-3516552057)
Are we keeping `gccbug_90348` (for `-fstack-reuse` bugs), even though it will no-longer reproduce with our supported versions of GCC?

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3516563445)
> Should probably be based on #33775.

Sure. Rebased on on #33775 and drafted for now.

📝 hebasto converted_to_draft a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764)
This PR is part of the ongoing effort to migrate to the modern UCRT runtime for cross-compiled Windows binaries, including release builds.

For more details about this migration, see:
- https://github.com/bitcoin/bitcoin/issues/30210
- https://github.com/bitcoin/bitcoin/pull/33593

A few items are outside the scope of this PR and are left for follow-up work:
1. The version of Debian's [cross-compiler](https://packages.debian.org/trixie/g++-mingw-w64-ucrt64) is 14.2.0, which differs from v
...

💬 pinheadmz commented on issue "ci: failure in `wallet_basic.py` KeyError: 'ischange'":
(https://github.com/bitcoin/bitcoin/issues/33848#issuecomment-3516579110)
This is the only such failure so far since merge?

👍 laanwj approved a pull request: "depends: drop qtbase_avoid_native_float16 qt patch"
(https://github.com/bitcoin/bitcoin/pull/33850#pullrequestreview-3447771251)
Code review ACK 169f93d2ac887a7295e87f0a69c2407cdd0a936e
Good to get rid of this patch. Using native float16 should be fine (and might even be a speed-up) on platforms that have this. If not, it's a good cleanup.

👍 rkrux approved a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3447791811)
crACK e346ecae830e10310979e5f64de63e043a383ff1

This advisory note is helpful.

💬 fanquake commented on pull request "guix: produce a `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-3516621838)
> Binary would be bigger

Yes ~2mb. I've added a comparison of bitcoind size for master vs this change, to the PR description.