💬 fjahr commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2879584074)
> I tested by using other blocks and not necessarily changing the node that is requesting blocks from node1.
> my personal preference would be to make node2 request block 773 (or some other valid block) from node1. but yes, this works :)
I don't think this is a good idea because it's not testing the same behavior that the test currently does. The test intends to check that the prune height doesn't change when the block data is available, but not the undo data of these blocks. To do this,
...
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2879584074)
> I tested by using other blocks and not necessarily changing the node that is requesting blocks from node1.
> my personal preference would be to make node2 request block 773 (or some other valid block) from node1. but yes, this works :)
I don't think this is a good idea because it's not testing the same behavior that the test currently does. The test intends to check that the prune height doesn't change when the block data is available, but not the undo data of these blocks. To do this,
...
💬 maflcko commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2879587037)
> So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() execution.
Can you explain why this is needed? Seems a lot of shuffling and code changes for a trivial change that will later cast everything to `std::string` anyway?
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2879587037)
> So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() execution.
Can you explain why this is needed? Seems a lot of shuffling and code changes for a trivial change that will later cast everything to `std::string` anyway?
👍 hebasto approved a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488#pullrequestreview-2839582440)
ACK fa427ffceeefd368a1ade273501ce4b01133ad4d, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
@maflcko
Thank you for picking this up!
(https://github.com/bitcoin/bitcoin/pull/32488#pullrequestreview-2839582440)
ACK fa427ffceeefd368a1ade273501ce4b01133ad4d, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
@maflcko
Thank you for picking this up!
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088577925)
Imo, there are 2 main advantages:
- discoverability: grepping for callsites, template instantiations etc is a lot more cumbersome (and easy to miss) than just inspecting the canonical template definition
- type safety: explicitly narrowing down the actual types that `T` can be makes reviewing the function easier, because you have to think about edge cases less, e.g. will the function still be safe when a new callsite is added
This is more a general style preference, and not particularly imp
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088577925)
Imo, there are 2 main advantages:
- discoverability: grepping for callsites, template instantiations etc is a lot more cumbersome (and easy to miss) than just inspecting the canonical template definition
- type safety: explicitly narrowing down the actual types that `T` can be makes reviewing the function easier, because you have to think about edge cases less, e.g. will the function still be safe when a new callsite is added
This is more a general style preference, and not particularly imp
...
⚠️ fanquake opened an issue: "test: wallet_reorgsrestore.py failure under valgrind"
(https://github.com/bitcoin/bitcoin/issues/32493)
I see this consistently when running the `native_valgrind` CI:
```bash
node0 2025-05-14T10:02:50.888348Z [init] [wallet/wallet.h:924] [WalletLogPrintf] [reorg_crash] Wallet completed loading in 61ms
node0 2025-05-14T10:02:50.889127Z [init] [wallet/sqlite.cpp:55] [TraceSqlCallback] [walletdb:trace] [/tmp/test_runner_₿_🏃_20250514_100134/wallet_reorgsrestore_0/node0/regtest/reorg_crash/wallet.dat] SQLite Statement: BEGIN TRANSACTION
node0 2025-05-14T10:02:50.889588Z [init] [wallet
...
(https://github.com/bitcoin/bitcoin/issues/32493)
I see this consistently when running the `native_valgrind` CI:
```bash
node0 2025-05-14T10:02:50.888348Z [init] [wallet/wallet.h:924] [WalletLogPrintf] [reorg_crash] Wallet completed loading in 61ms
node0 2025-05-14T10:02:50.889127Z [init] [wallet/sqlite.cpp:55] [TraceSqlCallback] [walletdb:trace] [/tmp/test_runner_₿_🏃_20250514_100134/wallet_reorgsrestore_0/node0/regtest/reorg_crash/wallet.dat] SQLite Statement: BEGIN TRANSACTION
node0 2025-05-14T10:02:50.889588Z [init] [wallet
...
🤔 marcofleon reviewed a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488#pullrequestreview-2839644907)
Code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
(https://github.com/bitcoin/bitcoin/pull/32488#pullrequestreview-2839644907)
Code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088614496)
> There are also other checks in ConnectBlock before the !state.IsValid() check that we may have skipped because of the early return when SpendBlock fails - including transaction input validation, script verification, fee range checks, ...; Is that correct?
Good catch! Actually I think the commit message description is misleading, if not wrong. The checks you mention are skipped in any case, because we check `if (!state.IsValid()) break;`, but similarly the coinbase check won't override the v
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088614496)
> There are also other checks in ConnectBlock before the !state.IsValid() check that we may have skipped because of the early return when SpendBlock fails - including transaction input validation, script verification, fee range checks, ...; Is that correct?
Good catch! Actually I think the commit message description is misleading, if not wrong. The checks you mention are skipped in any case, because we check `if (!state.IsValid()) break;`, but similarly the coinbase check won't override the v
...
🚀 fanquake merged a pull request: "fuzz: Properly setup wallet in wallet_fees target"
(https://github.com/bitcoin/bitcoin/pull/32488)
(https://github.com/bitcoin/bitcoin/pull/32488)
⚠️ fanquake opened an issue: "build: (initial?) failure to build xproto on Alpine"
(https://github.com/bitcoin/bitcoin/issues/32494)
```bash
# podman run -it alpine
apk update && apk add pkgconf patch python3 cmake g++ bash curl gcc git make && \
git clone https://github.com/bitcoin/bitcoin && \
cd bitcoin && \
make -C depends qt -j13
```
This will fail with
```bash
Staging xproto...
make[1]: Entering directory '/bitcoin/depends/work/build/aarch64-unknown-linux-musl/xproto/7.0.31-7e4871430ec'
Making install in specs
make[2]: Entering directory '/bitcoin/depends/work/build/aarch64-unknown-linux-musl/xproto/7.0.31-7e4871430
...
(https://github.com/bitcoin/bitcoin/issues/32494)
```bash
# podman run -it alpine
apk update && apk add pkgconf patch python3 cmake g++ bash curl gcc git make && \
git clone https://github.com/bitcoin/bitcoin && \
cd bitcoin && \
make -C depends qt -j13
```
This will fail with
```bash
Staging xproto...
make[1]: Entering directory '/bitcoin/depends/work/build/aarch64-unknown-linux-musl/xproto/7.0.31-7e4871430ec'
Making install in specs
make[2]: Entering directory '/bitcoin/depends/work/build/aarch64-unknown-linux-musl/xproto/7.0.31-7e4871430
...
💬 jsarenik commented on issue "Allow for rescan using block filters with pruned node":
(https://github.com/bitcoin/bitcoin/issues/21267#issuecomment-2879773571)
Just what I noticed and it seems related to this issue: After using `removeprunedfunds` followed by `rescanblockchain start stop` I see spent transactions in my descriptor wallet's `listunspent` output. 100% reproducible on regtest.
What I found so far is that after `removeprunedfunds` a `rescanblockchain` called without parameters results in a usable wallet but when called with some block range like `rescanblockchain 1026 1146` (with my tip height 1146) I end up with lots of already-spent outp
...
(https://github.com/bitcoin/bitcoin/issues/21267#issuecomment-2879773571)
Just what I noticed and it seems related to this issue: After using `removeprunedfunds` followed by `rescanblockchain start stop` I see spent transactions in my descriptor wallet's `listunspent` output. 100% reproducible on regtest.
What I found so far is that after `removeprunedfunds` a `rescanblockchain` called without parameters results in a usable wallet but when called with some block range like `rescanblockchain 1026 1146` (with my tip height 1146) I end up with lots of already-spent outp
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088705047)
It seems like list initializing `coin` is triggering MSVC to copy-initialize a `Coin` temporary, causing a second user-defined conversion and subsequent error. I don't really understand why direct initialization rules are different in this regard, but using it does seem to make MSVC happy: https://github.com/stickies-v/bitcoin/actions/runs/15018875180/job/42203094843
Fix: https://github.com/stickies-v/bitcoin/commit/6f1bc11c336db6dcea2954a03520b7064f0f0e6d
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088705047)
It seems like list initializing `coin` is triggering MSVC to copy-initialize a `Coin` temporary, causing a second user-defined conversion and subsequent error. I don't really understand why direct initialization rules are different in this regard, but using it does seem to make MSVC happy: https://github.com/stickies-v/bitcoin/actions/runs/15018875180/job/42203094843
Fix: https://github.com/stickies-v/bitcoin/commit/6f1bc11c336db6dcea2954a03520b7064f0f0e6d
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088714491)
`m_node_mutex` is a recursive mutex, allowing multiple nested locks from a single thread. But see https://github.com/bitcoin/bitcoin/pull/32326 which changes it to a non-recursive mutex which is easier to reason about.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088714491)
`m_node_mutex` is a recursive mutex, allowing multiple nested locks from a single thread. But see https://github.com/bitcoin/bitcoin/pull/32326 which changes it to a non-recursive mutex which is easier to reason about.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879829734)
`8b93e0894f...bd6c57f387`: address suggestions
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879829734)
`8b93e0894f...bd6c57f387`: address suggestions
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088718421)
Because the only caller of `GenerateWaitSockets()` has a vector so I decided to pass a const reference to a vector. However, that was the case as well before this PR and the change from raw pointer to `shared_ptr` (the aim of this PR) is unrelated to that. Changed back to `std::span`.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088718421)
Because the only caller of `GenerateWaitSockets()` has a vector so I decided to pass a const reference to a vector. However, that was the case as well before this PR and the change from raw pointer to `shared_ptr` (the aim of this PR) is unrelated to that. Changed back to `std::span`.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088722206)
Yes, I meant that the location of the deletes is unchanged. Before we had `DeleteNode()` which did `FinalizeNode()` + `delete` and after that commit, `DeleteNode()` is expanded into the callers that do `FinalizeNode()` directly instead of calling `DeleteNode()` + `clear()` on the vector which is equivalent to the `delete` that was in `DeleteNode()`.
Elaborated a little bit in the commit message.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088722206)
Yes, I meant that the location of the deletes is unchanged. Before we had `DeleteNode()` which did `FinalizeNode()` + `delete` and after that commit, `DeleteNode()` is expanded into the callers that do `FinalizeNode()` directly instead of calling `DeleteNode()` + `clear()` on the vector which is equivalent to the `delete` that was in `DeleteNode()`.
Elaborated a little bit in the commit message.
💬 maflcko commented on pull request "test: add skip_if_running_under_valgrind()":
(https://github.com/bitcoin/bitcoin/pull/32492#issuecomment-2879855860)
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
I haven't tested or confirmed this, but the diff looks plausible.
(https://github.com/bitcoin/bitcoin/pull/32492#issuecomment-2879855860)
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
I haven't tested or confirmed this, but the diff looks plausible.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088731770)
I did not want to bloat this PR with changes to the interface of `ProcessMessages()` and `SendMessages()`. They take a raw pointer. Since they do not take ownership and the object is going to be alive for the duration of the call and there will always be an object (will never pass `nullptr`), then those functions can take `CNode&`.
Here is a patch to do that on top of this PR. Could be a followup. I do not want to bloat this PR with this unless reviewers want it.
<details>
<summary>[patch
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2088731770)
I did not want to bloat this PR with changes to the interface of `ProcessMessages()` and `SendMessages()`. They take a raw pointer. Since they do not take ownership and the object is going to be alive for the duration of the call and there will always be an object (will never pass `nullptr`), then those functions can take `CNode&`.
Here is a patch to do that on top of this PR. Could be a followup. I do not want to bloat this PR with this unless reviewers want it.
<details>
<summary>[patch
...
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879863257)
`bd6c57f387...20657a7c8e`: rebase
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2879863257)
`bd6c57f387...20657a7c8e`: rebase
⚠️ fanquake opened an issue: "test: consider Clang `type` sanitizer"
(https://github.com/bitcoin/bitcoin/issues/32495)
Clang 20 introduced an experimental type sanitizer that "detects violations of C/C++ type-based aliasing rules." See here for more info: https://clang.llvm.org/docs/TypeSanitizer.html. Someone might want to run this against our code, to see if it works, or detects real issues. This could likely be done with:
```bash
make -C depends/ CC=clang CXX=clang++ CXXFLAGS="-stdlib=libc++" NM=llvm-nm AR=llvm-ar RANLIB=llvm-ranlib STRIP=llvm-strip
cmake -B build --toolchain /bitcoin/depends/x86_64-pc-linux-
...
(https://github.com/bitcoin/bitcoin/issues/32495)
Clang 20 introduced an experimental type sanitizer that "detects violations of C/C++ type-based aliasing rules." See here for more info: https://clang.llvm.org/docs/TypeSanitizer.html. Someone might want to run this against our code, to see if it works, or detects real issues. This could likely be done with:
```bash
make -C depends/ CC=clang CXX=clang++ CXXFLAGS="-stdlib=libc++" NM=llvm-nm AR=llvm-ar RANLIB=llvm-ranlib STRIP=llvm-strip
cmake -B build --toolchain /bitcoin/depends/x86_64-pc-linux-
...
💬 maflcko commented on pull request "test: fix two intermittent failures in wallet_basic.py":
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2879883748)
review ACK e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 🎩
<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: review ACK e7ad86e1ca3b
...
(https://github.com/bitcoin/bitcoin/pull/32483#issuecomment-2879883748)
review ACK e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 🎩
<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: review ACK e7ad86e1ca3b
...