💬 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
...
💬 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-2224098060)
Thanks!
Just saw that #23234 describes the same issue (and suggests the same fix).
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2224098060)
Thanks!
Just saw that #23234 describes the same issue (and suggests the same fix).
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2224112679)
re-ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2224112679)
re-ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953
💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1674830641)
Good thought. At least from an initial look, what makes sense to me would be checking if it's already null and if not call `freeaddrinfo()` (`getaddrinfo()` handles allocation and `freeaddrinfo()` handles cleanup). I suspect this could be somewhat overkill (or not happen if unsuccessful `getaddrinfo()` never sets ai_res to non-null), but we don't want a memory leak. I'm still looking for guarantees in documentation (and we'd need to potentially compare guarantees for different platforms).
...
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1674830641)
Good thought. At least from an initial look, what makes sense to me would be checking if it's already null and if not call `freeaddrinfo()` (`getaddrinfo()` handles allocation and `freeaddrinfo()` handles cleanup). I suspect this could be somewhat overkill (or not happen if unsuccessful `getaddrinfo()` never sets ai_res to non-null), but we don't want a memory leak. I'm still looking for guarantees in documentation (and we'd need to potentially compare guarantees for different platforms).
...
💬 kevkevinpal commented on pull request "log: LogError with FlatFilePos in UndoReadFromDisk":
(https://github.com/bitcoin/bitcoin/pull/30428#issuecomment-2224176117)
ACK [fa14e1d](https://github.com/bitcoin/bitcoin/pull/30428/commits/fa14e1d9d5c5dc44396a01583ae94480b7bc29ee)
I think this makes sense and mirroring `ReadBlockFromDisk` seems reasonable to me
(https://github.com/bitcoin/bitcoin/pull/30428#issuecomment-2224176117)
ACK [fa14e1d](https://github.com/bitcoin/bitcoin/pull/30428/commits/fa14e1d9d5c5dc44396a01583ae94480b7bc29ee)
I think this makes sense and mirroring `ReadBlockFromDisk` seems reasonable to me
💬 kevkevinpal commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2224232188)
> The coverage we do have comes from the process_messages harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Concept ACK [75fc22f](https://github.com/bitcoin/bitcoin/pull/30413/commits/75fc22f7729c6df779162edb620f986b5c811d8b)
I think it makes sense to fuzz the handshake itself
Maybe I'm just not too familiar with fuzz testing but this seems very similar to `process_messages`.
Would it make sense
...
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2224232188)
> The coverage we do have comes from the process_messages harness, but the harness doesn't fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Concept ACK [75fc22f](https://github.com/bitcoin/bitcoin/pull/30413/commits/75fc22f7729c6df779162edb620f986b5c811d8b)
I think it makes sense to fuzz the handshake itself
Maybe I'm just not too familiar with fuzz testing but this seems very similar to `process_messages`.
Would it make sense
...