💬 theuni commented on pull request "depends: bump boost to 1.85.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2223618172)
Ping @hebasto. Tested on `cmake-staging` with `Boost_NO_BOOST_CMAKE` removed. Seems to work as expected. Though ofc we wouldn't be able to remove that until we require boost 1.82 for non-depends builds.
(https://github.com/bitcoin/bitcoin/pull/30434#issuecomment-2223618172)
Ping @hebasto. Tested on `cmake-staging` with `Boost_NO_BOOST_CMAKE` removed. Seems to work as expected. Though ofc we wouldn't be able to remove that until we require boost 1.82 for non-depends builds.
💬 theuni commented on pull request "depends: update doc in Qt pwd patch":
(https://github.com/bitcoin/bitcoin/pull/30336#issuecomment-2223626277)
Nice, utACK. Any reason not to just take their full patch, though? Guessing it doesn't apply cleanly?
(https://github.com/bitcoin/bitcoin/pull/30336#issuecomment-2223626277)
Nice, utACK. Any reason not to just take their full patch, though? Guessing it doesn't apply cleanly?
💬 fanquake commented on pull request "depends: update doc in Qt pwd patch":
(https://github.com/bitcoin/bitcoin/pull/30336#issuecomment-2223639910)
Yea, iirc. Also didn't want to change anything else here, and possibly break anything else, given we don't need any other changes.
(https://github.com/bitcoin/bitcoin/pull/30336#issuecomment-2223639910)
Yea, iirc. Also didn't want to change anything else here, and possibly break anything else, given we don't need any other changes.
💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674506073)
More seriously: i considered doing it but it would not match the existing style for `HasDeepDerivPath` and would require duplicating the `MAX_SUBS` and `MAX_WRAPPERS` args (once in descriptor and once in descriptor_parse). So IMO it would be less neat and therefore i didn't do it.
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674506073)
More seriously: i considered doing it but it would not match the existing style for `HasDeepDerivPath` and would require duplicating the `MAX_SUBS` and `MAX_WRAPPERS` args (once in descriptor and once in descriptor_parse). So IMO it would be less neat and therefore i didn't do it.
🤔 theuni reviewed a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423#pullrequestreview-2172750410)
Nice, concept ACK. I had the same thought when looking at this for #30387.
I think it should be possible (or maybe this is enough?) to avoid making toolchain assumptions and have this work outside of guix.
(https://github.com/bitcoin/bitcoin/pull/30423#pullrequestreview-2172750410)
Nice, concept ACK. I had the same thought when looking at this for #30387.
I think it should be possible (or maybe this is enough?) to avoid making toolchain assumptions and have this work outside of guix.
👍 theuni approved a pull request: "depends: update doc in Qt pwd patch"
(https://github.com/bitcoin/bitcoin/pull/30336#pullrequestreview-2172752965)
ACK f170fe04ca03fe4021cbff7c5450ce3cc7fda17f
(https://github.com/bitcoin/bitcoin/pull/30336#pullrequestreview-2172752965)
ACK f170fe04ca03fe4021cbff7c5450ce3cc7fda17f
💬 achow101 commented on pull request "test: fix inconsistency in fundrawtransaction weight limits test":
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2223661762)
ACK 00b8e26bd6a47a8207c7571ca7ef9643f0ee2668
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2223661762)
ACK 00b8e26bd6a47a8207c7571ca7ef9643f0ee2668
💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#issuecomment-2223670550)
Thank you both for the review. I addressed your comments and heavily documented the code. I think it can be helpful as it indeed relies on being familiar with the descriptor/miniscript syntax. If only to make the assumptions in the counting logic explicit, or for when we'll all have forgotten about the details of this syntax.
I also modified two small things:
- I introduced a limit to the number of nested sub-fragments a fragment may have. Since we are pushing an element on top of a stack fo
...
(https://github.com/bitcoin/bitcoin/pull/30197#issuecomment-2223670550)
Thank you both for the review. I addressed your comments and heavily documented the code. I think it can be helpful as it indeed relies on being familiar with the descriptor/miniscript syntax. If only to make the assumptions in the counting logic explicit, or for when we'll all have forgotten about the details of this syntax.
I also modified two small things:
- I introduced a limit to the number of nested sub-fragments a fragment may have. Since we are pushing an element on top of a stack fo
...
💬 maflcko commented on pull request "rpc: Use CHECK_NONFATAL over Assert":
(https://github.com/bitcoin/bitcoin/pull/30429#discussion_r1674529679)
> I think assuming no whitespace between a macro and the parenthesis is a regression, since that's allowed?
Yes, also a newline, but both are "forbidden" by clang-format. If you want to be more accurate you'll have to write a clang-tidy plugin, but I won't be doing this myself.
> I think because of the `\<` prefix?
Keep in mind that macOS is not supported. What is `grep --version | grep GNU` on your machine?
(https://github.com/bitcoin/bitcoin/pull/30429#discussion_r1674529679)
> I think assuming no whitespace between a macro and the parenthesis is a regression, since that's allowed?
Yes, also a newline, but both are "forbidden" by clang-format. If you want to be more accurate you'll have to write a clang-tidy plugin, but I won't be doing this myself.
> I think because of the `\<` prefix?
Keep in mind that macOS is not supported. What is `grep --version | grep GNU` on your machine?
💬 mzumsande commented on issue "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever":
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223676434)
I now have a hypothesis what might be happening. In `Shutdown()`, we first stop the scheduler thread and then the import blocks thread [here](https://github.com/bitcoin/bitcoin/blob/00feabf6c59c954c1e3a5a46e58cb80cd8d55911/src/init.cpp#L301-L302).
When the import blocks thread calls `ActivateBestChain()`, this can call `LimitValidationInterfaceQueue` [here](https://github.com/bitcoin/bitcoin/blob/00feabf6c59c954c1e3a5a46e58cb80cd8d55911/src/validation.cpp#L3487). In a case of unfortunate timing
...
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223676434)
I now have a hypothesis what might be happening. In `Shutdown()`, we first stop the scheduler thread and then the import blocks thread [here](https://github.com/bitcoin/bitcoin/blob/00feabf6c59c954c1e3a5a46e58cb80cd8d55911/src/init.cpp#L301-L302).
When the import blocks thread calls `ActivateBestChain()`, this can call `LimitValidationInterfaceQueue` [here](https://github.com/bitcoin/bitcoin/blob/00feabf6c59c954c1e3a5a46e58cb80cd8d55911/src/validation.cpp#L3487). In a case of unfortunate timing
...
💬 maflcko commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-2223678147)
One-line force-push to relax the v2 assumption to allow v3/TRUC transactions, to avoid having to touch the code again in the future.
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-2223678147)
One-line force-push to relax the v2 assumption to allow v3/TRUC transactions, to avoid having to touch the code again in the future.
🚀 achow101 merged a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353)
(https://github.com/bitcoin/bitcoin/pull/30353)
✅ achow101 closed an issue: "ci: failure in `wallet_fundrawtransaction.py --legacy-wallet`"
(https://github.com/bitcoin/bitcoin/issues/30432)
(https://github.com/bitcoin/bitcoin/issues/30432)
👋 achow101's pull request is ready for review: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328)
(https://github.com/bitcoin/bitcoin/pull/30328)
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#issuecomment-2223723973)
Ready for review now that #26596 has been merged.
(https://github.com/bitcoin/bitcoin/pull/30328#issuecomment-2223723973)
Ready for review now that #26596 has been merged.
👍 hebasto approved a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387#pullrequestreview-2172893390)
ACK 9010b1343b9f931f771d3d49dd03b57868c24d5d.
My Guix build:
```
x86_64
77365de6a5a0dd40d79f2c4ba8ac8f105c44348076ef64eb36c9ae030ce833f8 guix-build-9010b1343b9f/output/aarch64-linux-gnu/SHA256SUMS.part
af1b2f2cd45ce3b510903c674d68545e0ec0ad4cb5b7e65b7e6e8f17eabb4f4f guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin-9010b1343b9f-aarch64-linux-gnu-debug.tar.gz
a88c00e179e1302a3ffd438c175b2f6331b58f0484757c1f843ca2d2a386fd59 guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/30387#pullrequestreview-2172893390)
ACK 9010b1343b9f931f771d3d49dd03b57868c24d5d.
My Guix build:
```
x86_64
77365de6a5a0dd40d79f2c4ba8ac8f105c44348076ef64eb36c9ae030ce833f8 guix-build-9010b1343b9f/output/aarch64-linux-gnu/SHA256SUMS.part
af1b2f2cd45ce3b510903c674d68545e0ec0ad4cb5b7e65b7e6e8f17eabb4f4f guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin-9010b1343b9f-aarch64-linux-gnu-debug.tar.gz
a88c00e179e1302a3ffd438c175b2f6331b58f0484757c1f843ca2d2a386fd59 guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin
...
💬 luke-jr commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1674637608)
Will this override the user if he sets a stronger mode?
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1674637608)
Will this override the user if he sets a stronger mode?
💬 luke-jr commented on pull request "build: add `standard branch-protection` to hardening flags for aarch64-linux":
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1674641005)
Could this be an issue?
https://sourceware.org/bugzilla/show_bug.cgi?id=26312
(https://github.com/bitcoin/bitcoin/pull/30433#discussion_r1674641005)
Could this be an issue?
https://sourceware.org/bugzilla/show_bug.cgi?id=26312
💬 furszy commented on pull request "test: fix inconsistency in fundrawtransaction weight limits test":
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2223886638)
A bit late but here @ismaelsadeeq.
> > Only for send()
>
> How is this an issue for `send()`?
Mainly because of what users would expect from the `send()` command. Which, due to its description, is to actually craft a sendable transaction. As is now, if this happens, the command will return the incomplete transaction without telling the user what happened. But.. I don't think will follow-up this thought. No one complained about it so far.
> should we consider returning this error ea
...
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2223886638)
A bit late but here @ismaelsadeeq.
> > Only for send()
>
> How is this an issue for `send()`?
Mainly because of what users would expect from the `send()` command. Which, due to its description, is to actually craft a sendable transaction. As is now, if this happens, the command will return the incomplete transaction without telling the user what happened. But.. I don't think will follow-up this thought. No one complained about it so far.
> should we consider returning this error ea
...
💬 hebasto commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2223943553)
This PR introduced a new `-Wmaybe-uninitialized` warning when building with GCC < 13:
- GCC 11:
```
CXX rpc/libbitcoin_node_a-blockchain.o
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1742:38: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<un
...
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2223943553)
This PR introduced a new `-Wmaybe-uninitialized` warning when building with GCC < 13:
- GCC 11:
```
CXX rpc/libbitcoin_node_a-blockchain.o
rpc/blockchain.cpp: In function ‘getchaintxstats()::<lambda(const RPCHelpMan&, const JSONRPCRequest&)>’:
rpc/blockchain.cpp:1742:38: warning: ‘*(std::_Optional_payload_base<unsigned int>::_Storage<unsigned int, true>*)((char*)&window_tx_count + offsetof(const std::optional<unsigned int>,std::optional<unsigned int>::<unnamed>.std::_Optional_base<un
...