💬 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
...
💬 hebasto 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-2223971606)
> I think a simple fix would be to change the order in `Shutdown()`, stopping import block before the scheduler:
>
> ```diff
> diff --git a/src/init.cpp b/src/init.cpp
> index e4b65fbfa9..e0aae3699f 100644
> --- a/src/init.cpp
> +++ b/src/init.cpp
> @@ -298,8 +298,8 @@ void Shutdown(NodeContext& node)
>
> // After everything has been shut down, but before things get flushed, stop the
> // scheduler and load block thread.
> - if (node.scheduler) node.scheduler->stop();
...
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223971606)
> I think a simple fix would be to change the order in `Shutdown()`, stopping import block before the scheduler:
>
> ```diff
> diff --git a/src/init.cpp b/src/init.cpp
> index e4b65fbfa9..e0aae3699f 100644
> --- a/src/init.cpp
> +++ b/src/init.cpp
> @@ -298,8 +298,8 @@ void Shutdown(NodeContext& node)
>
> // After everything has been shut down, but before things get flushed, stop the
> // scheduler and load block thread.
> - if (node.scheduler) node.scheduler->stop();
...
💬 ryanofsky commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2224087427)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2223419766
> Unless there's way to halt processing of an IPC message mid-way, push out NewTemplate and then process the remainder that we need (much later) to reply to RequestTransactionData.Success.
I'm not sure if this is a question about the multiprocess IPC framework, or you're just deciding what the mining IPC interface should look like, but the framework is nonblocking, so there is no problem with the client making multip
...
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2224087427)
re: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2223419766
> Unless there's way to halt processing of an IPC message mid-way, push out NewTemplate and then process the remainder that we need (much later) to reply to RequestTransactionData.Success.
I'm not sure if this is a question about the multiprocess IPC framework, or you're just deciding what the mining IPC interface should look like, but the framework is nonblocking, so there is no problem with the client making multip
...