π€ mzumsande reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2908751605)
Concept ACK.
I wonder if it might be better to add the additional tests to `p2p_initial_headers_sync.py` - after all, this is not a particularly long test.
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2908751605)
Concept ACK.
I wonder if it might be better to add the additional tests to `p2p_initial_headers_sync.py` - after all, this is not a particularly long test.
π¬ mzumsande commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2134912428)
this seems unnecessary, `add_outbound_p2p_connection` already has a built-in wait for the `verack`.
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2134912428)
this seems unnecessary, `add_outbound_p2p_connection` already has a built-in wait for the `verack`.
π¬ yuvicc commented on issue "avoid using invalidateblock to directly test reorg behavior":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2954644351)
I've opened a PR [#32587](https://github.com/bitcoin/bitcoin/pull/32587) to update the reorg pattern to the real one mentioned above β it's ready for review! If anyoneβs interested in taking a look, your feedback would be most welcome and appreciated!
@instagibbs @mzumsande
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2954644351)
I've opened a PR [#32587](https://github.com/bitcoin/bitcoin/pull/32587) to update the reorg pattern to the real one mentioned above β it's ready for review! If anyoneβs interested in taking a look, your feedback would be most welcome and appreciated!
@instagibbs @mzumsande
π¬ hebasto commented on pull request "test: Turn util/test_runner into functional test":
(https://github.com/bitcoin/bitcoin/pull/32697#issuecomment-2955009919)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/32697#issuecomment-2955009919)
Concept ACK.
π¬ yuvicc commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2955049712)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2955049712)
Concept ACK
π€ shahsb reviewed a pull request: "wallet: Allow read-only database access for info and dump commands"
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2909362789)
Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.
This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2909362789)
Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.
This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.
π€ shahsb reviewed a pull request: "wallet: Allow read-only database access for info and dump commands"
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2909366676)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32685#pullrequestreview-2909366676)
Concept ACK
π€ mabu44 reviewed a pull request: "docs: adds correct updated documentation links"
(https://github.com/bitcoin/bitcoin/pull/32699#pullrequestreview-2909378292)
This makes it necessary to manually update the links with every release, or at least with every new version that contains changes to those documentation files.
(https://github.com/bitcoin/bitcoin/pull/32699#pullrequestreview-2909378292)
This makes it necessary to manually update the links with every release, or at least with every new version that contains changes to those documentation files.
π¬ yuvicc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2955103592)
Concept ACK
Will benchmark this soon!
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2955103592)
Concept ACK
Will benchmark this soon!
β οΈ fanquake opened an issue: "depends: Windows libevent build broken against mingw-w64 13.x"
(https://github.com/bitcoin/bitcoin/issues/32707)
#### make -C depends HOST=x86_64-w64-mingw32
```bash
/opt/homebrew/Cellar/mingw-w64/13.0.0/toolchain-x86_64/x86_64-w64-mingw32/include/iphlpapi.h:51:44: error: unknown type name 'PMIB_TCP6ROW'; did you mean 'PMIB_TCPROW'?
51 | ULONG WINAPI SetPerTcp6ConnectionEStats (PMIB_TCP6ROW Row, TCP_ESTATS_TYPE EstatsType, PUCHAR Rw, ULONG RwVersion, ULONG RwSize, ULONG Offset);
| ^~~~~~~~~~~~
| PMIB_TCPR
...
(https://github.com/bitcoin/bitcoin/issues/32707)
#### make -C depends HOST=x86_64-w64-mingw32
```bash
/opt/homebrew/Cellar/mingw-w64/13.0.0/toolchain-x86_64/x86_64-w64-mingw32/include/iphlpapi.h:51:44: error: unknown type name 'PMIB_TCP6ROW'; did you mean 'PMIB_TCPROW'?
51 | ULONG WINAPI SetPerTcp6ConnectionEStats (PMIB_TCP6ROW Row, TCP_ESTATS_TYPE EstatsType, PUCHAR Rw, ULONG RwVersion, ULONG RwSize, ULONG Offset);
| ^~~~~~~~~~~~
| PMIB_TCPR
...
π rkrux opened a pull request: "rpc, doc: update `listdescriptors` RCP help"
(https://github.com/bitcoin/bitcoin/pull/32708)
This RPC lists all the descriptors present in the wallet, not only the ones that were imported, but also the ones generated when a new wallet is created.
It can be verified by creating a new wallet and calling the `listdescriptors` RPC, which will contain 8 ranged descriptors that are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled" because this is the only wallet type available now after removal of legacy wallets.
<!--
*** Please remove the
...
(https://github.com/bitcoin/bitcoin/pull/32708)
This RPC lists all the descriptors present in the wallet, not only the ones that were imported, but also the ones generated when a new wallet is created.
It can be verified by creating a new wallet and calling the `listdescriptors` RPC, which will contain 8 ranged descriptors that are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled" because this is the only wallet type available now after removal of legacy wallets.
<!--
*** Please remove the
...
π¬ Sjors commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2955317432)
The wrapper binary has been merged with #31375. Actually shipping the binaries would happen if #31802 lands.
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2955317432)
The wrapper binary has been merged with #31375. Actually shipping the binaries would happen if #31802 lands.
π¬ PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2955355673)
> Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.
>
> This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.
Good idea. I could check it and work on it.
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-2955355673)
> Could we also guard higher-level wallet operations (e.g., in LoadWallet, TopUpKeyPool, UpgradeWallet, etc.) against unintended writes in read-only mode?.
>
> This helps future-proof against accidental changes, even if developers mistakenly invoke write operations upstream.
Good idea. I could check it and work on it.
π¬ hebasto commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2955357844)
cc @theuni
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2955357844)
cc @theuni
π¬ saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2135490336)
Hi @achow101 could you please share your thoughts with the latest changes.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2135490336)
Hi @achow101 could you please share your thoughts with the latest changes.
π¬ stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135515163)
> but I'm actually not seeing where this was happening
There's a `InvalidChainFound(to_mark_failed)` call near the end of `InvalidateBlock()`, which in turn calls `RecalculateBestHeader()`.
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2135515163)
> but I'm actually not seeing where this was happening
There's a `InvalidChainFound(to_mark_failed)` call near the end of `InvalidateBlock()`, which in turn calls `RecalculateBestHeader()`.
π¬ stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2955469784)
re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
No changes except minor ones addressing review feedback wrt documentation updates and inlining.
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2955469784)
re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
No changes except minor ones addressing review feedback wrt documentation updates and inlining.
π¬ stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2135540635)
Oh no I agree, debug logs should be exempt from ratelimiting. My point is just that I think it's bad to have different logging behaviour depending on whether `LogInfo()` or `LogPrintLevel(..., BCLog::Info)` is used, so I think something like the below would work well?
<details>
<summary>git diff on 911ee520c8</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index d588ef86dc..5f298405e7 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -349,17 +349,13 @@ inline void LogPri
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2135540635)
Oh no I agree, debug logs should be exempt from ratelimiting. My point is just that I think it's bad to have different logging behaviour depending on whether `LogInfo()` or `LogPrintLevel(..., BCLog::Info)` is used, so I think something like the below would work well?
<details>
<summary>git diff on 911ee520c8</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index d588ef86dc..5f298405e7 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -349,17 +349,13 @@ inline void LogPri
...
π hebasto opened a pull request: "cmake: Fix `FindQt` module"
(https://github.com/bitcoin/bitcoin/pull/32709)
This PR improves the `FindQt` module in two ways:
1. Ensures that the `find_package(Qt .. MODULE REQUIRED COMPONENTS ...)` call treats any missing component as a fatal error. In the master branch, only a warning is currently issued.
2. Cleans up the user-facing part of the CMake cache following the migration to Qt 6. An exception (`Qt6_DIR`) is documented.
(https://github.com/bitcoin/bitcoin/pull/32709)
This PR improves the `FindQt` module in two ways:
1. Ensures that the `find_package(Qt .. MODULE REQUIRED COMPONENTS ...)` call treats any missing component as a fatal error. In the master branch, only a warning is currently issued.
2. Cleans up the user-facing part of the CMake cache following the migration to Qt 6. An exception (`Qt6_DIR`) is documented.
π¬ Sjors commented on issue "multiprocess: `ipc_tests` fail on *BSD":
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2955561573)
I was able to run the IPC tests on an aarch64 OpenBSD VM using the patch in https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949888947 and #32690 and they passed on the first try.
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2955561573)
I was able to run the IPC tests on an aarch64 OpenBSD VM using the patch in https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949888947 and #32690 and they passed on the first try.