💬 fjahr commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2879442591)
re-ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2879442591)
re-ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088470479)
Yes, that is it. I don't think this will actually matter in the grand scheme. I think I'll drop it again.
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088470479)
Yes, that is it. I don't think this will actually matter in the grand scheme. I think I'll drop it again.
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088486315)
Pushed the compiler issue on a separate branch here: https://github.com/TheCharlatan/bitcoin/actions/runs/15016860100/job/42196724477#step:10:198 .
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088486315)
Pushed the compiler issue on a separate branch here: https://github.com/TheCharlatan/bitcoin/actions/runs/15016860100/job/42196724477#step:10:198 .
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088492832)
> edit: I see this is already (partially) suggested and addressed in https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2078426205, I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span
I did implement a concept initially, but did not find it really that helpful, since the two usable template variants are concretely defined anyway.
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088492832)
> edit: I see this is already (partially) suggested and addressed in https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2078426205, I think adding the IsCoinRef concept could still be nice but less important than specifying the std::span
I did implement a concept initially, but did not find it really that helpful, since the two usable template variants are concretely defined anyway.
💬 polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088471060)
Nit: Will it be removed for sure? If there's no clear consensus on removal, maybe is worth deleting the last part of the sentence. I've seen a lot of controversy bc of that the last two days.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088471060)
Nit: Will it be removed for sure? If there's no clear consensus on removal, maybe is worth deleting the last part of the sentence. I've seen a lot of controversy bc of that the last two days.
🤔 polespinasa reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2839410351)
code review ACK 35bcd8eed3
left one small comment to avoid further discussions...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2839410351)
code review ACK 35bcd8eed3
left one small comment to avoid further discussions...
💬 maflcko commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879517285)
> drahtbot has categorized your comment as nack instead of ack
<details><summary>off-topic reply</summary>
When a comment contains both `ack` and `nack` (in uppercase), the bot just picks one. Obviously, it is off-topic to discuss here, but I think there is a misunderstanding what the goal of the summary comment by the bot is. The goal is *not* to have everyone "register" their "vote". It is simply a best-effort overview to have a clickable link to every reviewer's most recent review comme
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879517285)
> drahtbot has categorized your comment as nack instead of ack
<details><summary>off-topic reply</summary>
When a comment contains both `ack` and `nack` (in uppercase), the bot just picks one. Obviously, it is off-topic to discuss here, but I think there is a misunderstanding what the goal of the summary comment by the bot is. The goal is *not* to have everyone "register" their "vote". It is simply a best-effort overview to have a clickable link to every reviewer's most recent review comme
...
💬 maflcko commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2088538753)
Are there steps to reproduce on a fresh install? I tried Ubuntu 24.04, but it seems to pass:
```
1 export DEBIAN_FRONTEND=noninteractive && apt update && apt install git vim htop guix bash curl make -y && groupadd --system guixbuild && for i in `seq -w 1 10`; do useradd -g guixbuild -G guixbuild -d /var/empty -s `which nologin` -c "Guix build user $i" --system guixbuilder$i; done
2 git config --global merge.conflictstyle zdiff3
3 guix-daemon --build-users-group=guixbuild
...
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2088538753)
Are there steps to reproduce on a fresh install? I tried Ubuntu 24.04, but it seems to pass:
```
1 export DEBIAN_FRONTEND=noninteractive && apt update && apt install git vim htop guix bash curl make -y && groupadd --system guixbuild && for i in `seq -w 1 10`; do useradd -g guixbuild -G guixbuild -d /var/empty -s `which nologin` -c "Guix build user $i" --system guixbuilder$i; done
2 git config --global merge.conflictstyle zdiff3
3 guix-daemon --build-users-group=guixbuild
...
📝 fanquake opened a pull request: "test: add skip_if_running_under_valgrind()"
(https://github.com/bitcoin/bitcoin/pull/32492)
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
(https://github.com/bitcoin/bitcoin/pull/32492)
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
💬 fanquake commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2879582658)
Opened #32492 to add skipping (for now).
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2879582658)
Opened #32492 to add skipping (for now).
💬 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
...