💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142693766)
Thanks for taking my suggestion!
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2142693766)
Thanks for taking my suggestion!
💬 l0rinc commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142779638)
Regardless of the implementation, what I was documenting here was the safety of changing one value to the other, without having to understand the details of why. If I touch again, I don't mind changing this, if other reviewer agree it's overly pedantic. In other cases, I have explicitly removed asserts like this in the next commit, the signal that this value was explicitly checked before.
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2142779638)
Regardless of the implementation, what I was documenting here was the safety of changing one value to the other, without having to understand the details of why. If I touch again, I don't mind changing this, if other reviewer agree it's overly pedantic. In other cases, I have explicitly removed asserts like this in the next commit, the signal that this value was explicitly checked before.
📝 hodlinator opened a pull request: "wallet: Warn upon failing to scan directory"
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
💬 l0rinc commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142790766)
That's why I asked for !m_initial_sync_finished as well.
If the race is biased towards reenabling it too early (rather than later), I agree that it should be fine.
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142790766)
That's why I asked for !m_initial_sync_finished as well.
If the race is biased towards reenabling it too early (rather than later), I agree that it should be fine.
💬 Muniru0 commented on pull request "docs: add guidance on initialism capitalisation in PascalCase identifiers.":
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-2966852048)
To balance attribution with long-term stability, I recommend that we:
- Include the external link to the original rationale—but accompany it with a clear disclaimer (e.g., “Note: this resource may change or become unavailable over time”).
- Embed a succinct, self-contained summary of the policy’s reasoning directly in the developer notes, so that the purpose and benefits of the convention remain immediately accessible even if the external link breaks.
Also take into consideration the `n
...
(https://github.com/bitcoin/bitcoin/pull/32720#issuecomment-2966852048)
To balance attribution with long-term stability, I recommend that we:
- Include the external link to the original rationale—but accompany it with a clear disclaimer (e.g., “Note: this resource may change or become unavailable over time”).
- Embed a succinct, self-contained summary of the policy’s reasoning directly in the developer notes, so that the purpose and benefits of the convention remain immediately accessible even if the external link breaks.
Also take into consideration the `n
...
💬 fanquake commented on pull request "wallet: Warn upon failing to scan directory":
(https://github.com/bitcoin/bitcoin/pull/32736#issuecomment-2966893310)
https://cirrus-ci.com/task/6556243914915840?logs=ci#L4845:
```bash
[09:50:58.842] AssertionError: [node 0] Expected messages "['Error scanning directory entries under']" does not partially match log:
```
(https://github.com/bitcoin/bitcoin/pull/32736#issuecomment-2966893310)
https://cirrus-ci.com/task/6556243914915840?logs=ci#L4845:
```bash
[09:50:58.842] AssertionError: [node 0] Expected messages "['Error scanning directory entries under']" does not partially match log:
```
💬 TheCharlatan commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142866210)
furszy reminded me that BlockChecked is run synchronously, so I don't think that comparison makes sense after all. I guess this comment can be resolved.
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142866210)
furszy reminded me that BlockChecked is run synchronously, so I don't think that comparison makes sense after all. I guess this comment can be resolved.
💬 furszy commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142868289)
> Shouldn't checking `role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload()` be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the `BlockChecked` method.
The diff between `BlockConnected()` and `BlockChecked()` is that the former one runs async while the latter runs synchronously. So the idea was to avoid locking `cs_ma
...
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2142868289)
> Shouldn't checking `role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload()` be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the `BlockChecked` method.
The diff between `BlockConnected()` and `BlockChecked()` is that the former one runs async while the latter runs synchronously. So the idea was to avoid locking `cs_ma
...
💬 maflcko commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2966958527)
> docker run --platform linux/s390x -it ubuntu:latest /bin/bash
>
>
>
>
>
> and inside I did something like:
>
> export DEBIAN_FRONTEND=noninteractive && apt update && apt install -y git build-essential cmake ccache pkg-config libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qtbase5-dev qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev python3 \
> cd /mnt && git clone --depth=1 https://github.com/bitcoin/bitcoin.git && cd bitcoin && \
> cmake -B build && cmake --b
...
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2966958527)
> docker run --platform linux/s390x -it ubuntu:latest /bin/bash
>
>
>
>
>
> and inside I did something like:
>
> export DEBIAN_FRONTEND=noninteractive && apt update && apt install -y git build-essential cmake ccache pkg-config libevent-dev libboost-dev libzmq3-dev systemtap-sdt-dev qtbase5-dev qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev python3 \
> cd /mnt && git clone --depth=1 https://github.com/bitcoin/bitcoin.git && cd bitcoin && \
> cmake -B build && cmake --b
...
💬 yuvicc commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2966967169)
> I've been working on this same issue and my findings align well with @vasild feedback for a simpler approach.
>
> **Regarding the core fix**: @vasild suggested approach is exactly right - the fix can be much simpler without the code duplication issues. Here's what I implemented successfully: bitcoin/src/init.cpp
>
> ```c++
> // We should discover addresses in two cases:
> // 1. When no -bind or -whitebind is specified (bind_on_any is true)
> // 2. When any -bind address is 0.0.0.0 o
...
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2966967169)
> I've been working on this same issue and my findings align well with @vasild feedback for a simpler approach.
>
> **Regarding the core fix**: @vasild suggested approach is exactly right - the fix can be much simpler without the code duplication issues. Here's what I implemented successfully: bitcoin/src/init.cpp
>
> ```c++
> // We should discover addresses in two cases:
> // 1. When no -bind or -whitebind is specified (bind_on_any is true)
> // 2. When any -bind address is 0.0.0.0 o
...
💬 maflcko commented on pull request "wallet: Warn upon failing to scan directory":
(https://github.com/bitcoin/bitcoin/pull/32736#discussion_r2142884563)
I don't think chmod works under root. So this test will likely fail in CI and pass locally
(https://github.com/bitcoin/bitcoin/pull/32736#discussion_r2142884563)
I don't think chmod works under root. So this test will likely fail in CI and pass locally
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885418)
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in `ProcessMessage` and that's what's passed into `AddTxAnnouncement` and used throughout `txdownloadman_impl` and `txrequest`.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous `uint256` comparison is now suddenly a `GenTxid` co
...
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885418)
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in `ProcessMessage` and that's what's passed into `AddTxAnnouncement` and used throughout `txdownloadman_impl` and `txrequest`.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous `uint256` comparison is now suddenly a `GenTxid` co
...
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885949)
We erase the iterator soon after but still use the hash, so making a copy here is correct.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885949)
We erase the iterator soon after but still use the hash, so making a copy here is correct.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142887549)
I'll include this with my next push.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142887549)
I'll include this with my next push.
💬 l0rinc commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967059854)
So it does look like an incomplete virtualization in Docker of the underlying network adapter - or something similar I guess.
Is there anything we should do here (comment the code, open an issue anywhere)? If not, please close it.
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967059854)
So it does look like an incomplete virtualization in Docker of the underlying network adapter - or something similar I guess.
Is there anything we should do here (comment the code, open an issue anywhere)? If not, please close it.
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967109115)
@ryanofsky looks like Tidy got more strict again...
```
[10:33:51.654] /ci_container_base/src/ipc/capnp/protocol.cpp:20:10: error: inclusion of deprecated C++ header 'errno.h'; consider using 'cerrno' instead [modernize-deprecated-headers,-warnings-as-errors]
[10:33:51.654] 20 | #include <errno.h>
[10:33:51.654] | ^~~~~~~~~
[10:33:51.654] | <cerrno>
[10:33:51.654] 919 warnings generated.
```
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967109115)
@ryanofsky looks like Tidy got more strict again...
```
[10:33:51.654] /ci_container_base/src/ipc/capnp/protocol.cpp:20:10: error: inclusion of deprecated C++ header 'errno.h'; consider using 'cerrno' instead [modernize-deprecated-headers,-warnings-as-errors]
[10:33:51.654] 20 | #include <errno.h>
[10:33:51.654] | ^~~~~~~~~
[10:33:51.654] | <cerrno>
[10:33:51.654] 919 warnings generated.
```
💬 maflcko commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967112918)
It would be nice to exactly find the issue and fix it, but I won't be working on this for now 😅
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967112918)
It would be nice to exactly find the issue and fix it, but I won't be working on this for now 😅
💬 maflcko commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967130272)
Should be easy to fix by removing this line: https://github.com/bitcoin-core/libmultiprocess/blob/019839758085cb094f107e9ac354cbda79b388e2/.clang-tidy#L18
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967130272)
Should be easy to fix by removing this line: https://github.com/bitcoin-core/libmultiprocess/blob/019839758085cb094f107e9ac354cbda79b388e2/.clang-tidy#L18
📝 rkrux opened a pull request: "rpc, doc: clarify the response of listtransactions RPC"
(https://github.com/bitcoin/bitcoin/pull/32737)
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and
...
(https://github.com/bitcoin/bitcoin/pull/32737)
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and
...
💬 l0rinc commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143003197)
> The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).
I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143003197)
> The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).
I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2967247693)
I have abstracted out the `std::next_permutation` logic into a separate `ExhaustiveLinearize()` function, and also added a comment that explains how the variable functions/classes/tests are related.
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2967247693)
I have abstracted out the `std::next_permutation` logic into a separate `ExhaustiveLinearize()` function, and also added a comment that explains how the variable functions/classes/tests are related.