💬 janb84 commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063192800)
NIT: `precious_variables` is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2063192800)
NIT: `precious_variables` is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?
💬 fanquake commented on pull request "Crash fix, disconnect numBlocksChanged() signal during shutdown":
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2834519435)
Backported to 29.x in #32292.
(https://github.com/bitcoin-core/gui/pull/864#issuecomment-2834519435)
Backported to 29.x in #32292.
💬 fanquake commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834538457)
Not sure I understand your suggestion; are you saying we don't fix this? Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else? Note that this is a generic issue, and the PIE check is just the first check that fails in this way, if you remove the PIE check the threads check will fail the same.
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834538457)
Not sure I understand your suggestion; are you saying we don't fix this? Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else? Note that this is a generic issue, and the PIE check is just the first check that fails in this way, if you remove the PIE check the threads check will fail the same.
💬 vasild commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2063248425)
Does it make sense to advance `pindexLastCommonBlock` in that case? I am just curious, it is definitely not the aim of this PR.
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2063248425)
Does it make sense to advance `pindexLastCommonBlock` in that case? I am just curious, it is definitely not the aim of this PR.
💬 fanquake commented on pull request "doc: Fix fuzz test_runner.py path":
(https://github.com/bitcoin/bitcoin/pull/32353#issuecomment-2834572446)
Backported to 29.x in #32292.
(https://github.com/bitcoin/bitcoin/pull/32353#issuecomment-2834572446)
Backported to 29.x in #32292.
💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063264880)
Can you update zmq version used to 4.3.5 (https://github.com/bitcoin/bitcoin/pull/28627).
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063264880)
Can you update zmq version used to 4.3.5 (https://github.com/bitcoin/bitcoin/pull/28627).
💬 fanquake commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063265648)
Can you update sqlite version used to 3.46.1 (#29991).
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063265648)
Can you update sqlite version used to 3.46.1 (#29991).
💬 fanquake commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834586735)
> but was found by https://github.com/bitcoin/bitcoin/pull/32339 momentarily
It was actually first pointed out here, https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500175006, by @dergoegge.
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834586735)
> but was found by https://github.com/bitcoin/bitcoin/pull/32339 momentarily
It was actually first pointed out here, https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2500175006, by @dergoegge.
💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063286019)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063286019)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf
💬 letetrautre commented on pull request "test: Force named args in RPCOverloadWrapper optional args":
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063287118)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf
(https://github.com/bitcoin/bitcoin/pull/32360#discussion_r2063287118)
bc1qw4lvl39reqarm65hlq2rfnvr4s4hpr8cmdttaf
💬 hebasto commented on pull request "test: avoid stack overflow in `FindChallenges` via manual iteration":
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2834604584)
@theuni
Due to your https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2524210640, you might b e interested in reviewing this PR.
(https://github.com/bitcoin/bitcoin/pull/32351#issuecomment-2834604584)
@theuni
Due to your https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2524210640, you might b e interested in reviewing this PR.
🤔 Sjors requested changes to a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798630677)
Concept ACK.
bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:
1. The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.
2. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` to follow, both here on Github as well as a `git show --color-moved=dimmed-zebra`. I'm guessing you're moving stuff around, deleting and modifying?
And a few details:
- there's one more `-DWITH_BDB=ON` in `test-each-commit-exec.sh`
- `test_framework/bd
...
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798630677)
Concept ACK.
bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:
1. The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.
2. In that same commit, the churn in `wallet/scriptpubkeyman.cpp` to follow, both here on Github as well as a `git show --color-moved=dimmed-zebra`. I'm guessing you're moving stuff around, deleting and modifying?
And a few details:
- there's one more `-DWITH_BDB=ON` in `test-each-commit-exec.sh`
- `test_framework/bd
...
💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063255286)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": why doesn't this just return `false` like before? And if so, can't the entire helper be dropped?
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063255286)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": why doesn't this just return `false` like before? And if so, can't the entire helper be dropped?
💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063278522)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": this test (file) and the one below still exist.
(https://github.com/bitcoin/bitcoin/pull/28710#discussion_r2063278522)
In dc7bf5fd6a320c4528a28cef2a565366bbab3877 "wallet: Delete LegacySPKM": this test (file) and the one below still exist.
💬 maflcko commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834611665)
> I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
@hebasto If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834611665)
> I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
@hebasto If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.
💬 hebasto commented on issue "build: CMake caching failing PIE check":
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834619343)
> are you saying we don't fix this?
Correct.
> Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else?
Not "always" — only when configuration fails. Similar to https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378.
(https://github.com/bitcoin/bitcoin/issues/32323#issuecomment-2834619343)
> are you saying we don't fix this?
Correct.
> Are you saying we should add a recommendation to the build docs to always delete the build dir before running CMake? Something else?
Not "always" — only when configuration fails. Similar to https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378.
💬 tomasandroil commented on pull request "Fix missing error check in `set_clo_on_exec` for FD_CLOEXEC handling":
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2834621951)
@hebasto
I have upstreamed the changes as requested: [arun11299/cpp-subprocess#117](https://github.com/arun11299/cpp-subprocess/pull/117).
(https://github.com/bitcoin/bitcoin/pull/32342#issuecomment-2834621951)
@hebasto
I have upstreamed the changes as requested: [arun11299/cpp-subprocess#117](https://github.com/arun11299/cpp-subprocess/pull/117).
💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834635374)
> > I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
>
> [@hebasto](https://github.com/hebasto) If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.
Will do.
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2834635374)
> > I'd say for the CI we want to reset `/Ob0 /Od` back to `/O2 /Ob1` to avoid slowness.
>
> [@hebasto](https://github.com/hebasto) If you don't mind, could you update your CI branch with this. If not, that's fine also and I'll try it myself.
Will do.
⚠️ fanquake opened an issue: "windows: usage of deprecated `std:wstring_convert`"
(https://github.com/bitcoin/bitcoin/issues/32361)
`std:wstring_convert` was deprecated in C++17 & is removed in C++26.
Cross-compiling for Windows with GCC 15.1.0 is warning about this:
```bash
../../src/common/system.cpp: In function 'void runCommand(const std::string&)':
../../src/common/system.cpp:52:32: warning: 'template<class _Codecvt, class _Elem, class _Wide_alloc, class _Byte_alloc> class std::__cxx11::wstring_convert' is deprecated [-Wdeprecated-declarations]
52 | int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf
...
(https://github.com/bitcoin/bitcoin/issues/32361)
`std:wstring_convert` was deprecated in C++17 & is removed in C++26.
Cross-compiling for Windows with GCC 15.1.0 is warning about this:
```bash
../../src/common/system.cpp: In function 'void runCommand(const std::string&)':
../../src/common/system.cpp:52:32: warning: 'template<class _Codecvt, class _Elem, class _Wide_alloc, class _Byte_alloc> class std::__cxx11::wstring_convert' is deprecated [-Wdeprecated-declarations]
52 | int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf
...
👍 maflcko approved a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798181194)
lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇
<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: lgtm ACK bc3f07e384c2e145
...
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2798181194)
lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇
<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: lgtm ACK bc3f07e384c2e145
...