💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675133144)
The `x86_64-w64-mingw32-codesigned` component—including the installer and archived binaries—has been tested on Windows 11 Pro 24H2. All signatures appear correct.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675133144)
The `x86_64-w64-mingw32-codesigned` component—including the installer and archived binaries—has been tested on Windows 11 Pro 24H2. All signatures appear correct.
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675167050)
> But it seems that this only picks the binaries. But if I understand the Apple priests correctly, you have to send the whole package.
The directory is passed to `signapple` which will proceed to zip it. Binaries by themselves cannot be uploaded (I tried).
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675167050)
> But it seems that this only picks the binaries. But if I understand the Apple priests correctly, you have to send the whole package.
The directory is passed to `signapple` which will proceed to zip it. Binaries by themselves cannot be uploaded (I tried).
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675183164)
> Note that this only affects code signers so I will hold off on updating signapple in guix for now.
Do you want to include these and then have us ACK-away at the PR?
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2675183164)
> Note that this only affects code signers so I will hold off on updating signapple in guix for now.
Do you want to include these and then have us ACK-away at the PR?
💬 fanquake commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2675183982)
https://github.com/bitcoin/bitcoin/actions/runs/13458258095/job/37607048581?pr=31923:
```bash
65/139 Test #9: bench_sanity_check_high_priority .....***Failed 78.91 sec
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
/home/runner/work/_temp/src/logging.cpp:337:30: runtime error: implicit conversion from type 'char' of value -20 (8-bit, signed) to type 'unsigned char' changed the value to 236 (8-bit, unsigned)
#0 0x5596549396a8 in
...
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2675183982)
https://github.com/bitcoin/bitcoin/actions/runs/13458258095/job/37607048581?pr=31923:
```bash
65/139 Test #9: bench_sanity_check_high_priority .....***Failed 78.91 sec
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
/home/runner/work/_temp/src/logging.cpp:337:30: runtime error: implicit conversion from type 'char' of value -20 (8-bit, signed) to type 'unsigned char' changed the value to 236 (8-bit, unsigned)
#0 0x5596549396a8 in
...
💬 fanquake commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2675190371)
@fjahr you might want to have a look here?
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2675190371)
@fjahr you might want to have a look here?
💬 theuni commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675264666)
> I've removed the `MAIN_DEPENDENCY` options (I don't recall the motivation behind their introduction). A comparison of the resulting `build.ninja` files reveals no changes in dependencies.
I believe `MAIN_DEPENDENCY` is MSVC related, so it wouldn't show up in the `build.ninja`. Did it help with something there?
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675264666)
> I've removed the `MAIN_DEPENDENCY` options (I don't recall the motivation behind their introduction). A comparison of the resulting `build.ninja` files reveals no changes in dependencies.
I believe `MAIN_DEPENDENCY` is MSVC related, so it wouldn't show up in the `build.ninja`. Did it help with something there?
📝 pablomartin4btc opened a pull request: "doc: Update translation generation instructions"
(https://github.com/bitcoin/bitcoin/pull/31930)
This is a follow-up of #31731.
The process [requires](https://github.com/bitcoin/bitcoin/pull/31899#issue-2861109641) to build `depends` with multiprocess as at some point the multiprocess-specific sources will contain translatable strings.
Technically this change [fixes](https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1928888001) the preset configuration as it needs multiprocess to be enabled.
(https://github.com/bitcoin/bitcoin/pull/31930)
This is a follow-up of #31731.
The process [requires](https://github.com/bitcoin/bitcoin/pull/31899#issue-2861109641) to build `depends` with multiprocess as at some point the multiprocess-specific sources will contain translatable strings.
Technically this change [fixes](https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1928888001) the preset configuration as it needs multiprocess to be enabled.
💬 theuni commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675317304)
> @theuni
>
> > Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
Ok, Ignore this. I believe I was testing with make, where the stall is expected. Ninja is indeed unchanged.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675317304)
> @theuni
>
> > Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
Ok, Ignore this. I believe I was testing with make, where the stall is expected. Ninja is indeed unchanged.
👍 theuni approved a pull request: "cmake: Revamp handling of data files"
(https://github.com/bitcoin/bitcoin/pull/30901#pullrequestreview-2633976453)
ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
(https://github.com/bitcoin/bitcoin/pull/30901#pullrequestreview-2633976453)
ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2675329745)
> > Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment
>
> On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?
>
> `getdescriptorinfo` and `importdescriptors` with both public key and private keys on master
@rkrux Some examples I just tried:
```
./build/s
...
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2675329745)
> > Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment
>
> On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?
>
> `getdescriptorinfo` and `importdescriptors` with both public key and private keys on master
@rkrux Some examples I just tried:
```
./build/s
...
💬 theuni commented on pull request "build: create Depends build type for depends and use it by default for depends builds":
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-2675463585)
> Current integration between depends and cmake builds definitely does not seem ideal, but it's hard for me to figure out if this PR makes the situation better. Maybe the PR description can be updated to state some advantages of this new approach? Or some problems with the current approach that this new approach solves?
>
> I listed a few aspects of the cmake/depends integration that I thought seemed broken and counterintuitive in [#30813 (comment)](https://github.com/bitcoin/bitcoin/issues
...
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-2675463585)
> Current integration between depends and cmake builds definitely does not seem ideal, but it's hard for me to figure out if this PR makes the situation better. Maybe the PR description can be updated to state some advantages of this new approach? Or some problems with the current approach that this new approach solves?
>
> I listed a few aspects of the cmake/depends integration that I thought seemed broken and counterintuitive in [#30813 (comment)](https://github.com/bitcoin/bitcoin/issues
...
💬 fjahr commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966033115)
In 0a0c803785782e40f4fe8055383565e3d24b59cc : That is weird but also I seem to be unable to parse this commit message (and the next one). Can this be made more understandable?
What I can gather is this seems to make the test less tight around the actually expected error (?) and I don't understand how that would make the test better.
"as the snapshot data may one day be so that a coin is correctly deserialized and none of these errors is hit". I don't understand how we are supposed to write
...
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966033115)
In 0a0c803785782e40f4fe8055383565e3d24b59cc : That is weird but also I seem to be unable to parse this commit message (and the next one). Can this be made more understandable?
What I can gather is this seems to make the test less tight around the actually expected error (?) and I don't understand how that would make the test better.
"as the snapshot data may one day be so that a coin is correctly deserialized and none of these errors is hit". I don't understand how we are supposed to write
...
💬 fjahr commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966132022)
I'm in favor of adding varint and compression implementation here since it's a more meaningful improvement and the code is already there. Also not sure why we need to add `VARINT(0)`?
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1966132022)
I'm in favor of adding varint and compression implementation here since it's a more meaningful improvement and the code is already there. Also not sure why we need to add `VARINT(0)`?
💬 fjahr commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675494080)
re-ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
Only removed `MAIN_DEPENDENCY` since last review.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2675494080)
re-ACK ecf54a32ed26a50e861fca559e43ec1f9dee93b7
Only removed `MAIN_DEPENDENCY` since last review.
🤔 mzumsande reviewed a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2634176605)
ACK 301017c6217378ca868e17c7cb8ffe3447abb11d (for v30)
I did a sanity check of the code paths leading to `AddToBlockIndex`, to verify that it should be impossible for us to accept a low-work header from an untrusted p2p source.
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2634176605)
ACK 301017c6217378ca868e17c7cb8ffe3447abb11d (for v30)
I did a sanity check of the code paths leading to `AddToBlockIndex`, to verify that it should be impossible for us to accept a low-work header from an untrusted p2p source.
💬 giahuy98 commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31912#issuecomment-2675501908)
2498dd8dbd5acdede03e1a57c5873ca022903023 works on Samsung S25 with the log: `Using RNDR as an additional entropy source`.
(https://github.com/bitcoin/bitcoin/pull/31912#issuecomment-2675501908)
2498dd8dbd5acdede03e1a57c5873ca022903023 works on Samsung S25 with the log: `Using RNDR as an additional entropy source`.
💬 sipa commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2675509389)
If we accept the fact that we have assembly RNDR/RNDRRS code in our project, I don't think it's unreasonable that this also means taking on the responsibility of dealing with weird hardware incompatibility issues it causes, in general. If we don't want that, we can choose to not have this support at all and just rely on the kernel's RNG.
But having merged something which doesn't compile on the platform it's support to fix things for is a red flag, and makes me feel that if we want to spend ef
...
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2675509389)
If we accept the fact that we have assembly RNDR/RNDRRS code in our project, I don't think it's unreasonable that this also means taking on the responsibility of dealing with weird hardware incompatibility issues it causes, in general. If we don't want that, we can choose to not have this support at all and just rely on the kernel's RNG.
But having merged something which doesn't compile on the platform it's support to fix things for is a red flag, and makes me feel that if we want to spend ef
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1966157404)
Indeed. Empty clusters can only exist in after an `ApplyRemovals` but before a `SplitAll()`. After oversizedness caching, `ApplyDependencies` may end up not calling `SplitAll()`, though, if the result is known to be oversized already.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1966157404)
Indeed. Empty clusters can only exist in after an `ApplyRemovals` but before a `SplitAll()`. After oversizedness caching, `ApplyDependencies` may end up not calling `SplitAll()`, though, if the result is known to be oversized already.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1966186414)
I think the most accurate description is "All entries which have an (R) removed locator at this level, plus any transactions in m_unlinked". Would that help?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1966186414)
I think the most accurate description is "All entries which have an (R) removed locator at this level, plus any transactions in m_unlinked". Would that help?
💬 mzumsande commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1966231509)
I'd prefer to call this wrapper around `ProcessNewBlock` some other name (e.g. `ProcessBlock()` like `net_processing` does) to avoid confusion with the existing `ConnectBlock` in validation. The added function doesn't necessarily attempt to connect the block, for example if a conflicting block arrived / was connected in between mining and calling `ProcessNewBlock` the real `ConnectBlock` may never be called.
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1966231509)
I'd prefer to call this wrapper around `ProcessNewBlock` some other name (e.g. `ProcessBlock()` like `net_processing` does) to avoid confusion with the existing `ConnectBlock` in validation. The added function doesn't necessarily attempt to connect the block, for example if a conflicting block arrived / was connected in between mining and calling `ProcessNewBlock` the real `ConnectBlock` may never be called.