💬 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:
👍 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
...
(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?
(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.
(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
...
(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?
(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.
(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.
(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.
(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.
🤔 rkrux reviewed a pull request: "Musig2 tests"
(https://github.com/bitcoin/bitcoin/pull/32724#pullrequestreview-3447877705)
> but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.
There isn't a MuSig2 class per-se.
Concept ACK fa65a57a826192ffae48f62f10c216068e8236f4
(https://github.com/bitcoin/bitcoin/pull/32724#pullrequestreview-3447877705)
> but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.
There isn't a MuSig2 class per-se.
Concept ACK fa65a57a826192ffae48f62f10c216068e8236f4
💬 rkrux commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514091946)
Nit: s/expected_xpub/expected_aggregate_xpub
```diff
- std::string expected_xpub;
+ std::string expected_aggregate_xpub;
```
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514091946)
Nit: s/expected_xpub/expected_aggregate_xpub
```diff
- std::string expected_xpub;
+ std::string expected_aggregate_xpub;
```
💬 rkrux commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514088966)
There's a utility function in the master branch that can be used instead of hardcoding.
```diff
--- a/src/test/musig2_tests.cpp
+++ b/src/test/musig2_tests.cpp
@@ -81,12 +81,7 @@ BOOST_AUTO_TEST_CASE(bip328)
BOOST_CHECK_MESSAGE(combined_keys == test.expected_aggregate_pubkey, "Test vector " << i << ": Aggregate pubkey mismatch");
// Create extended public key
- CExtPubKey extpub;
- extpub.nDepth = 0;
- std::fill(std::begin(extpub.vchFingerprint), std::e
...
(https://github.com/bitcoin/bitcoin/pull/32724#discussion_r2514088966)
There's a utility function in the master branch that can be used instead of hardcoding.
```diff
--- a/src/test/musig2_tests.cpp
+++ b/src/test/musig2_tests.cpp
@@ -81,12 +81,7 @@ BOOST_AUTO_TEST_CASE(bip328)
BOOST_CHECK_MESSAGE(combined_keys == test.expected_aggregate_pubkey, "Test vector " << i << ": Aggregate pubkey mismatch");
// Create extended public key
- CExtPubKey extpub;
- extpub.nDepth = 0;
- std::fill(std::begin(extpub.vchFingerprint), std::e
...
💬 laanwj commented on pull request "guix: produce a more static `bitcoin-qt`":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3516832333)
i think making libxcb static is fine, as (as far as i know) no Linux distributions that make breaking interface changes to these. Nor does xcb have global hardcoded configuration such as paths, like the font libraries.
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3516832333)
i think making libxcb static is fine, as (as far as i know) no Linux distributions that make breaking interface changes to these. Nor does xcb have global hardcoded configuration such as paths, like the font libraries.
💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3516832835)
The following in the PR description had me confused:
> Client requests for block templates will specify the maximum age of the template in seconds; that is, how fresh they want the template to be:
I thought it meant that templates have an expiration time, but from 8af9ede0608f5bb4fe3eddf7d3fe4fbb57a4649d it's clear that the caller specifies how old cached templates can be (via `max_template_age`).
If you're having trouble with mock time in the tests / fuzzer, it might help to add some d
...
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3516832835)
The following in the PR description had me confused:
> Client requests for block templates will specify the maximum age of the template in seconds; that is, how fresh they want the template to be:
I thought it meant that templates have an expiration time, but from 8af9ede0608f5bb4fe3eddf7d3fe4fbb57a4649d it's clear that the caller specifies how old cached templates can be (via `max_template_age`).
If you're having trouble with mock time in the tests / fuzzer, it might help to add some d
...
💬 Sjors commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514084547)
In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf _miner: add `SimilarOptions` function_: both these comments should be in the header, as they explain both what this function does and why it's useful.
Maybe call it `ShareableOptions`, described as:
> Limit comparison to assembly options that impact the final block (without dummy coinbase scriptPubKey, with solution). Ignore options that can safely be shared between different callers.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514084547)
In a620c0a69e6d2b0d29f0714f2c6c0bff33e2b4bf _miner: add `SimilarOptions` function_: both these comments should be in the header, as they explain both what this function does and why it's useful.
Maybe call it `ShareableOptions`, described as:
> Limit comparison to assembly options that impact the final block (without dummy coinbase scriptPubKey, with solution). Ignore options that can safely be shared between different callers.