📝 Sjors converted_to_draft a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135)
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/explorin
...
(https://github.com/bitcoin/bitcoin/pull/33135)
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/explorin
...
💬 maflcko commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3556353881)
lgtm ACK 2578e6fc0f4af35f389cd8ff59825c874e0b72ac
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3556353881)
lgtm ACK 2578e6fc0f4af35f389cd8ff59825c874e0b72ac
💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3556397690)
Rebased and improved the test error logging:
```cpp
// A safe boundary value should yield no warnings.
{
FlatSigningProvider keys;
std::string err;
auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(65535)))", keys, err, /*require_checksum=*/false);
BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
```
This fails with:
```
test/descriptor_tests.cpp:1265: Entering test case "descriptor_old
...
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3556397690)
Rebased and improved the test error logging:
```cpp
// A safe boundary value should yield no warnings.
{
FlatSigningProvider keys;
std::string err;
auto descs = Parse("wsh(and_v(v:pk(0379e45b3cf75f9c5f9befd8e9506fb962f6a9d185ac87001ec44a8d3df8d4a9e3),older(65535)))", keys, err, /*require_checksum=*/false);
BOOST_REQUIRE_MESSAGE(!descs.empty(), err);
```
This fails with:
```
test/descriptor_tests.cpp:1265: Entering test case "descriptor_old
...
💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3556555996)
Rebased on master. I updated the Miniscript implementation slightly due to changes from https://github.com/bitcoin/bitcoin/pull/31734
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3556555996)
Rebased on master. I updated the Miniscript implementation slightly due to changes from https://github.com/bitcoin/bitcoin/pull/31734
💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3556680990)
`6e0f3a4a58...47f4f65d0c`: rebase due to conflicts
Is the local failure you observe reproducible or sporadic? If it is reproducible maybe you can nail down which test is making the traffic? Previously to find the offending test I bisected the list of tests (used to nail down the traffic from `node_init_tests/init_test` which is fixed in this PR). Here is a write-only script to list all tests (681 currently) and run only e.g. from 1 to 340:
```sh
BUILD/bin/test_bitcoin $(BUILD/bin/test_bit
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3556680990)
`6e0f3a4a58...47f4f65d0c`: rebase due to conflicts
Is the local failure you observe reproducible or sporadic? If it is reproducible maybe you can nail down which test is making the traffic? Previously to find the offending test I bisected the list of tests (used to nail down the traffic from `node_init_tests/init_test` which is fixed in this PR). Here is a write-only script to list all tests (681 currently) and run only e.g. from 1 to 340:
```sh
BUILD/bin/test_bitcoin $(BUILD/bin/test_bit
...
🤔 yuvicc reviewed a pull request: "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags"
(https://github.com/bitcoin/bitcoin/pull/33908#pullrequestreview-3486594405)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/33908#pullrequestreview-3486594405)
Approach ACK
💬 yuvicc commented on pull request "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags":
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545072096)
And then use `Handle` instead of `UniqueHandle` here:
```suggestion
class BlockValidationState : public Handle<btck_BlockValidationState, btck_block_validation_state_copy, btck_block_validation_state_destroy>, public BlockValidationStateApi<BlockValidationState>
```
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545072096)
And then use `Handle` instead of `UniqueHandle` here:
```suggestion
class BlockValidationState : public Handle<btck_BlockValidationState, btck_block_validation_state_copy, btck_block_validation_state_destroy>, public BlockValidationStateApi<BlockValidationState>
```
💬 yuvicc commented on pull request "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags":
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545063204)
I think we also expose `_copy` API here:
```suggestion
/**
* @brief Copies the block validation state.
*
* @param[in] block_validation_state Non-null.
* @return The copied block validation state.
*/
BITCOINKERNEL_API btck_BlockValidationState* BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_validation_state_copy(
const btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1);
```
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545063204)
I think we also expose `_copy` API here:
```suggestion
/**
* @brief Copies the block validation state.
*
* @param[in] block_validation_state Non-null.
* @return The copied block validation state.
*/
BITCOINKERNEL_API btck_BlockValidationState* BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_validation_state_copy(
const btck_BlockValidationState* block_validation_state) BITCOINKERNEL_ARG_NONNULL(1);
```
💬 yuvicc commented on pull request "kernel: add context‑free block validation API (`btck_check_block_context_free`) with POW/Merkle flags":
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545075148)
Also here:
```suggestion
explicit BlockValidationState() : Handle{btck_block_validation_state_create()} {}
```
(https://github.com/bitcoin/bitcoin/pull/33908#discussion_r2545075148)
Also here:
```suggestion
explicit BlockValidationState() : Handle{btck_block_validation_state_create()} {}
```
💬 fanquake commented on pull request "depends: update xcb-util packages to latest versions":
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2545130316)
I think the issue was updating the packages in lockstep. I'll cherry-pick the two commits, with some adjustments, i.e to remove `--without-launchd`.
(https://github.com/bitcoin/bitcoin/pull/33851#discussion_r2545130316)
I think the issue was updating the packages in lockstep. I'll cherry-pick the two commits, with some adjustments, i.e to remove `--without-launchd`.
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2545150228)
There's a related report to libxcb here: https://gitlab.freedesktop.org/xorg/lib/libxcb/-/merge_requests/25.
(https://github.com/bitcoin/bitcoin/pull/33537#discussion_r2545150228)
There's a related report to libxcb here: https://gitlab.freedesktop.org/xorg/lib/libxcb/-/merge_requests/25.
👍 vasild approved a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3486800310)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Reviewing is learning, I love it! Thanks!
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3486800310)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Reviewing is learning, I love it! Thanks!
📝 maflcko opened a pull request: "clang-format: Set PackConstructorInitializers: CurrentLine"
(https://github.com/bitcoin/bitcoin/pull/33912)
Now that the minimum supported clang version is 17, the `PackConstructorInitializers` setting can be set to `CurrentLine` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
The `CurrentLine` option will either put all constructor initializers on the current line if they fit. Otherwise, it will put each one on its own line.
The `CurrentLine` option is desirable over the current `BinPack` option, because:
...
(https://github.com/bitcoin/bitcoin/pull/33912)
Now that the minimum supported clang version is 17, the `PackConstructorInitializers` setting can be set to `CurrentLine` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
The `CurrentLine` option will either put all constructor initializers on the current line if they fit. Otherwise, it will put each one on its own line.
The `CurrentLine` option is desirable over the current `BinPack` option, because:
...
💬 maflcko commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545299017)
I don't think the tip can be nullptr in rpc code. This can just be a `CHECK_NONFATAL`, but this seems unrelated to lifetimebound stuff?
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545299017)
I don't think the tip can be nullptr in rpc code. This can just be a `CHECK_NONFATAL`, but this seems unrelated to lifetimebound stuff?
💬 l0rinc commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545337568)
> this seems unrelated to lifetimebound stuff
Yes, he started with:
> An off-topic nit
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2545337568)
> this seems unrelated to lifetimebound stuff
Yes, he started with:
> An off-topic nit
💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545357404)
reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545357404)
reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3557161480)
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Only a [const removal](https://github.com/bitcoin/bitcoin/compare/9a387e9175d34d1eaf83b9008f5e857d1eb04b8e..de4242f47476769d0a7f3e79e8297ed2dd60d9a4?diff=unified&w) since my last ACK
(https://github.com/bitcoin/bitcoin/pull/32740#issuecomment-3557161480)
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Only a [const removal](https://github.com/bitcoin/bitcoin/compare/9a387e9175d34d1eaf83b9008f5e857d1eb04b8e..de4242f47476769d0a7f3e79e8297ed2dd60d9a4?diff=unified&w) since my last ACK
💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545381694)
maybe unrelated, because I think it was working at some point, but using `podman` on various distros (Debian/Ubuntu/OpenSuse) to try to run the s390x task fails with a segfault in the depends compilation:
```
...
[58/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qfsfileengine_iterator.cpp.o
[59/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qloggingcategory.cpp.o
FAILED: qtbase/src/tools/bootstrap/C
...
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2545381694)
maybe unrelated, because I think it was working at some point, but using `podman` on various distros (Debian/Ubuntu/OpenSuse) to try to run the s390x task fails with a segfault in the depends compilation:
```
...
[58/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qfsfileengine_iterator.cpp.o
[59/817] Building CXX object qtbase/src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/io/qloggingcategory.cpp.o
FAILED: qtbase/src/tools/bootstrap/C
...
👍 l0rinc approved a pull request: "clang-format: Set PackConstructorInitializers: CurrentLine"
(https://github.com/bitcoin/bitcoin/pull/33912#pullrequestreview-3487068101)
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d
I agree that this is our best option now, mostly because it will minimize diffs
(https://github.com/bitcoin/bitcoin/pull/33912#pullrequestreview-3487068101)
ACK fad0c76d0a109ab7b063f0d405588cf1e6c15e4d
I agree that this is our best option now, mostly because it will minimize diffs
💬 l0rinc commented on pull request "clang-format: Set PackConstructorInitializers: CurrentLine":
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545431725)
> Now that the minimum supported clang version is 17, the PackConstructorInitializers setting can be set to CurrentLine in the clang-format file.
Nit: `CurrentLine` was already available in [16](https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
<img width="967" height="703" alt="Image" src="https://github.com/user-attachments/assets/4e1fb719-9f19-496d-b184-9d869cc27a33" />
The new addition in [17](https://releases.llvm.org/17.0.1/tools/
...
(https://github.com/bitcoin/bitcoin/pull/33912#discussion_r2545431725)
> Now that the minimum supported clang version is 17, the PackConstructorInitializers setting can be set to CurrentLine in the clang-format file.
Nit: `CurrentLine` was already available in [16](https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#packconstructorinitializers)
<img width="967" height="703" alt="Image" src="https://github.com/user-attachments/assets/4e1fb719-9f19-496d-b184-9d869cc27a33" />
The new addition in [17](https://releases.llvm.org/17.0.1/tools/
...