💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2933483080)
re-ACK abd0749fae
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2933483080)
re-ACK abd0749fae
💬 maflcko commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-2933488025)
lgtm ACK 1e09c00239dafe7e9fa4f4d3f0d80c07bb7120bf
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-2933488025)
lgtm ACK 1e09c00239dafe7e9fa4f4d3f0d80c07bb7120bf
💬 maflcko commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-2933495508)
There are still some places where it is incorrectly suggested, but this seems unrelated:
```
[19:19:20.209] /ci_container_base/src/node/miner.cpp should add these lines:
[19:19:20.209] #include <boost/multi_index/detail/bidir_node_iterator.hpp> // for operator==
[19:19:20.209] #include <boost/operators.hpp> // for operator!=
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-2933495508)
There are still some places where it is incorrectly suggested, but this seems unrelated:
```
[19:19:20.209] /ci_container_base/src/node/miner.cpp should add these lines:
[19:19:20.209] #include <boost/multi_index/detail/bidir_node_iterator.hpp> // for operator==
[19:19:20.209] #include <boost/operators.hpp> // for operator!=
💬 maflcko commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2122723920)
Deleting the folder right away should be fine, too. (This is what this pull is doing?) An alternative would be to delay the deletion until the next major release and keep the old data for one release.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2122723920)
Deleting the folder right away should be fine, too. (This is what this pull is doing?) An alternative would be to delay the deletion until the next major release and keep the old data for one release.
💬 maflcko commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2122750882)
> If we really do want to drop the test coverage, that also seems ok, but dropping it doesn't seem required.
Yeah, I really want to drop this one. The other reason is that a corresponding non-type-safe catch needs to be maintained in `src/test/fuzz/rpc.cpp`, which was historically brittle and I expect will be in the future.
If someone cares about test coverage, a self-contained minimal unit test may be best here. Happy to review such a follow-up.
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2122750882)
> If we really do want to drop the test coverage, that also seems ok, but dropping it doesn't seem required.
Yeah, I really want to drop this one. The other reason is that a corresponding non-type-safe catch needs to be maintained in `src/test/fuzz/rpc.cpp`, which was historically brittle and I expect will be in the future.
If someone cares about test coverage, a self-contained minimal unit test may be best here. Happy to review such a follow-up.
💬 maflcko commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2933562293)
> It doesn't seem like this is catching the issue in [#29136 (comment)](https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120666028)? I tried making a similar change to a RPC in this PR and the test still passed.
Are you sure? What are the exact steps to reproduce?
Locally such a bug passes for me on current master:
```diff
$ git checkout bitcoin-core/master && git diff && cmakeb && ./bld-cmake/test/functional/rpc_help.py
M src/rpc/signmessage.cpp
HEAD is now at 88b22acc3d
...
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2933562293)
> It doesn't seem like this is catching the issue in [#29136 (comment)](https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120666028)? I tried making a similar change to a RPC in this PR and the test still passed.
Are you sure? What are the exact steps to reproduce?
Locally such a bug passes for me on current master:
```diff
$ git checkout bitcoin-core/master && git diff && cmakeb && ./bld-cmake/test/functional/rpc_help.py
M src/rpc/signmessage.cpp
HEAD is now at 88b22acc3d
...
💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2933563529)
nit: can drop "RFC:" from the second commit?
> I was able to reproduce the hash for the first commit [6998e93](https://github.com/bitcoin/bitcoin/commit/6998e933f935a379c3ad55c2fb16eca9b854f40b), but not for the second commit [20778eb](https://github.com/bitcoin/bitcoin/commit/20778eb0235df70397fc285f9e3b72270bd4aaf4).
Tested this again against a2bf3b03b19a0e668bf86eea7de3e3a592b4b23f and fc81a759413230daf3e05b448b357e62915bb3ba on my M4 machine running macOS 15.5 with Python 3.10.14 (via
...
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2933563529)
nit: can drop "RFC:" from the second commit?
> I was able to reproduce the hash for the first commit [6998e93](https://github.com/bitcoin/bitcoin/commit/6998e933f935a379c3ad55c2fb16eca9b854f40b), but not for the second commit [20778eb](https://github.com/bitcoin/bitcoin/commit/20778eb0235df70397fc285f9e3b72270bd4aaf4).
Tested this again against a2bf3b03b19a0e668bf86eea7de3e3a592b4b23f and fc81a759413230daf3e05b448b357e62915bb3ba on my M4 machine running macOS 15.5 with Python 3.10.14 (via
...
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2933572032)
re-ACK 61001085941fd6d78f4cd46c9b3d65914b6e961e
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2933572032)
re-ACK 61001085941fd6d78f4cd46c9b3d65914b6e961e
💬 l3x3l commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933592354)
@achow101 Thank you. I was able to get both private and public key dumps from your python program.
The version of the wallet is:
Wallet file version: version=210000
Now, can you help me find my funds in this wallet i recovered? I have an extensive series of private keys and I don't know which ones have funds.
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933592354)
@achow101 Thank you. I was able to get both private and public key dumps from your python program.
The version of the wallet is:
Wallet file version: version=210000
Now, can you help me find my funds in this wallet i recovered? I have an extensive series of private keys and I don't know which ones have funds.
💬 achow101 commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933609476)
> The version of the wallet is: Wallet file version: version=210000
Huh, that's really weird as the highest version number is 169900 and it's been that way since 0.17.0 was in development.
> Now, can you help me find my funds in this wallet i recovered? I have an extensive series of private keys and I don't know which ones have funds.
You don't necessarily need to import just the ones with funds; you can import all of those private keys and your funds should show up. While your wallet will ha
...
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933609476)
> The version of the wallet is: Wallet file version: version=210000
Huh, that's really weird as the highest version number is 169900 and it's been that way since 0.17.0 was in development.
> Now, can you help me find my funds in this wallet i recovered? I have an extensive series of private keys and I don't know which ones have funds.
You don't necessarily need to import just the ones with funds; you can import all of those private keys and your funds should show up. While your wallet will ha
...
💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122844417)
Unfortunately `has_nx` only includes stack nx checks in 0.16.6 (and main):
https://github.com/lief-project/LIEF/tree/0.16.6/include/LIEF/MachO/Binary.hpp#L579-L593
```cpp
/// Check if the binary uses `NX` protection
bool has_nx() const override {
return !has_nx_stack();
}
/// Return True if the **heap** is flagged as non-executable. False otherwise
bool has_nx_stack() const {
return !header().has(Header::FLAGS::ALLOW_STACK_EXECUTION);
}
/// Return True
...
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122844417)
Unfortunately `has_nx` only includes stack nx checks in 0.16.6 (and main):
https://github.com/lief-project/LIEF/tree/0.16.6/include/LIEF/MachO/Binary.hpp#L579-L593
```cpp
/// Check if the binary uses `NX` protection
bool has_nx() const override {
return !has_nx_stack();
}
/// Return True if the **heap** is flagged as non-executable. False otherwise
bool has_nx_stack() const {
return !header().has(Header::FLAGS::ALLOW_STACK_EXECUTION);
}
/// Return True
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2933706882)
only changes are addressed nits
re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tr
...
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2933706882)
only changes are addressed nits
re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tr
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2122849568)
Doesn't matter much, but the comment could also say "... so skip the patch because it is not needed and to avoid issues ...".
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2122849568)
Doesn't matter much, but the comment could also say "... so skip the patch because it is not needed and to avoid issues ...".
💬 fanquake commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122854382)
Ah, right, it was the old logic that included both.. I wonder why it was split out. I guess we can just document here then, or switch to both for all (relevant) platforms.
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122854382)
Ah, right, it was the old logic that included both.. I wonder why it was split out. I guess we can just document here then, or switch to both for all (relevant) platforms.
💬 l3x3l commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933724050)
I see! What wallet should I use to import the private keys? I’ve had a
problem using Bitcoin wallet. I’m using iOS.
l3x3l
>--o--<
On Tue, Jun 3, 2025 at 12:04 AM Ava Chow ***@***.***> wrote:
> *achow101* left a comment (bitcoin/bitcoin#32548)
> <https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933609476>
>
> The version of the wallet is: Wallet file version: version=210000
>
> Huh, that's really weird as the highest version number is 169900 and it's
> been that
...
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933724050)
I see! What wallet should I use to import the private keys? I’ve had a
problem using Bitcoin wallet. I’m using iOS.
l3x3l
>--o--<
On Tue, Jun 3, 2025 at 12:04 AM Ava Chow ***@***.***> wrote:
> *achow101* left a comment (bitcoin/bitcoin#32548)
> <https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933609476>
>
> The version of the wallet is: Wallet file version: version=210000
>
> Huh, that's really weird as the highest version number is 169900 and it's
> been that
...
💬 l3x3l commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933733924)
@achow101
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2933733924)
@achow101
🤔 maflcko reviewed a pull request: "doc: miscellaneous changes"
(https://github.com/bitcoin/bitcoin/pull/32644#pullrequestreview-2890916359)
review ACK c33e7d20b39efaae33625a061396fe065ad3cfc7 🔬
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c33e7d20b39e
...
(https://github.com/bitcoin/bitcoin/pull/32644#pullrequestreview-2890916359)
review ACK c33e7d20b39efaae33625a061396fe065ad3cfc7 🔬
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c33e7d20b39e
...
💬 maflcko commented on pull request "doc: miscellaneous changes":
(https://github.com/bitcoin/bitcoin/pull/32644#discussion_r2122875951)
should be https://app.transifex.com/signup/open-source/ instead?
(https://github.com/bitcoin/bitcoin/pull/32644#discussion_r2122875951)
should be https://app.transifex.com/signup/open-source/ instead?
💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122900028)
Opened https://github.com/lief-project/LIEF/issues/1221 and added a note for this.
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2122900028)
Opened https://github.com/lief-project/LIEF/issues/1221 and added a note for this.
💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2933782962)
Rebased to address @hebasto and @fanquake feedback.
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2933782962)
Rebased to address @hebasto and @fanquake feedback.