💬 hebasto commented on pull request "depends: Fix build of Qt for 32-bit platforms with recent glibc":
(https://github.com/bitcoin/bitcoin/pull/29985#issuecomment-2082485060)
My Guix builds:
```
x86_64
7bd0bf18cb3b17ee5829ca24e7488d0d022e280d0f56ff04c20f9199f787178a guix-build-2e266f33b5d2/output/aarch64-linux-gnu/SHA256SUMS.part
9c07a926fbdafff0f189c454652ccf9028e4ba9c44826b7874affc69bc5b5ae9 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu-debug.tar.gz
8d9ce23406bbcc33f2cc4147f06c715c44c5b0cc18af6be296862c3e531f3f97 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu.tar.gz
34a37952
...
(https://github.com/bitcoin/bitcoin/pull/29985#issuecomment-2082485060)
My Guix builds:
```
x86_64
7bd0bf18cb3b17ee5829ca24e7488d0d022e280d0f56ff04c20f9199f787178a guix-build-2e266f33b5d2/output/aarch64-linux-gnu/SHA256SUMS.part
9c07a926fbdafff0f189c454652ccf9028e4ba9c44826b7874affc69bc5b5ae9 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu-debug.tar.gz
8d9ce23406bbcc33f2cc4147f06c715c44c5b0cc18af6be296862c3e531f3f97 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu.tar.gz
34a37952
...
💬 Sjors commented on pull request "[do not merge] validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2082492618)
I switched to the halving block! Also dropped all prerequisites but #26045 from the description, but see conceptual discussion in #29616.
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2082492618)
I switched to the halving block! Also dropped all prerequisites but #26045 from the description, but see conceptual discussion in #29616.
⚠️ Sjors opened an issue: "Pause IBD during AssumeUTXO snapshot load"
(https://github.com/bitcoin/bitcoin/issues/29993)
### Please describe the feature you'd like to see added.
The `loadtxoutset` RPC call should cause IBD to pause until the snapshot is loaded and the snapshot chain is activated.
### Is your feature related to a problem, if so please describe it.
When I start a fresh node and load a mainnet shapshot (#28553) any progress is completely buried in the log.
More importantly, I get the strong impression, though haven't properly tested this, that the IBD process slows down the snapshot load. This
...
(https://github.com/bitcoin/bitcoin/issues/29993)
### Please describe the feature you'd like to see added.
The `loadtxoutset` RPC call should cause IBD to pause until the snapshot is loaded and the snapshot chain is activated.
### Is your feature related to a problem, if so please describe it.
When I start a fresh node and load a mainnet shapshot (#28553) any progress is completely buried in the log.
More importantly, I get the strong impression, though haven't properly tested this, that the IBD process slows down the snapshot load. This
...
💬 Sjors commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2082496290)
Definitely not a blocker: #29993
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2082496290)
Definitely not a blocker: #29993
✅ willcl-ark closed an issue: "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf"
(https://github.com/bitcoin/bitcoin/issues/29139)
(https://github.com/bitcoin/bitcoin/issues/29139)
💬 willcl-ark commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2082504824)
I agree, going to close this issue and #29903 for now
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2082504824)
I agree, going to close this issue and #29903 for now
✅ willcl-ark closed a pull request: "rename bitcoin.conf in dist tarball"
(https://github.com/bitcoin/bitcoin/pull/29903)
(https://github.com/bitcoin/bitcoin/pull/29903)
🤔 BrandonOdiwuor reviewed a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2028283852)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2028283852)
Concept ACK
🤔 hebasto reviewed a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028305896)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028305896)
Concept ACK.
👍 willcl-ark approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2028133549)
ACK 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824
I think the macro simplication included in this change is a nice win for simplicity.
Ran all the updated examples in contrib/tracing, and played around with adding/removing various tracepoint arguments to check everything works as expected.
Left one q and one suggestion inline, but neither a blocker for this.
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2028133549)
ACK 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824
I think the macro simplication included in this change is a nice win for simplicity.
Ran all the updated examples in contrib/tracing, and played around with adding/removing various tracepoint arguments to check everything works as expected.
Left one q and one suggestion inline, but neither a blocker for this.
💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582972621)
In: 9343ee83ec9ca1ac32d9686f6aea6d755c623562
It's not clear to me how this differs from the semaphore gating being introduced. ISTM that the caller will need the semaphore in either case, so what difference does preparing the argument inside this `if` make? The `TRACEPOINT` macro appears to expand to an `if(TRACEPOINT_ACTIVE( ...`
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582972621)
In: 9343ee83ec9ca1ac32d9686f6aea6d755c623562
It's not clear to me how this differs from the semaphore gating being introduced. ISTM that the caller will need the semaphore in either case, so what difference does preparing the argument inside this `if` make? The `TRACEPOINT` macro appears to expand to an `if(TRACEPOINT_ACTIVE( ...`
💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582872788)
In: 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824
If you re-touch these, it might be nice (user-friendly) to add a simple check that we are running as root. Currently without root you see:
```log
Hooking into bitcoind with pid 4039799
Traceback (most recent call last):
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 258, in <module>
main(pid)
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 124, in main
bitcoind_with_usdts.enable_probe(
Fi
...
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582872788)
In: 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824
If you re-touch these, it might be nice (user-friendly) to add a simple check that we are running as root. Currently without root you see:
```log
Hooking into bitcoind with pid 4039799
Traceback (most recent call last):
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 258, in <module>
main(pid)
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 124, in main
bitcoind_with_usdts.enable_probe(
Fi
...
💬 paplorinc commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1582993565)
Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
Furthermore, since the proposed change affects how the `add_to_mempool` flag behaves, would it be more transparent to redefine it to directly reflect whether a transaction can be added to the mempool, i.e. `bool add_to_mempool = !pool.exists(GenTxid::Txid(tx->GetHash())) && fuzzed_data_provider.ConsumeBool();`?
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1582993565)
Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?
Furthermore, since the proposed change affects how the `add_to_mempool` flag behaves, would it be more transparent to redefine it to directly reflect whether a transaction can be added to the mempool, i.e. `bool add_to_mempool = !pool.exists(GenTxid::Txid(tx->GetHash())) && fuzzed_data_provider.ConsumeBool();`?
👍 brunoerg approved a pull request: "validation: don't clear cache on periodic flush: >2x block connection speed"
(https://github.com/bitcoin/bitcoin/pull/28233#pullrequestreview-2028345200)
crACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
(https://github.com/bitcoin/bitcoin/pull/28233#pullrequestreview-2028345200)
crACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
🤔 stickies-v reviewed a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2028288474)
nit: I think the last 3 commits should be squashed
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2028288474)
nit: I think the last 3 commits should be squashed
💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582961257)
Could add a `CHECK_NONFATAL(first_block->nHeight > 0);` here to put that assumption in code
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582961257)
Could add a `CHECK_NONFATAL(first_block->nHeight > 0);` here to put that assumption in code
💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582963779)
This block needs to be in the next commit - we're not passing `BLOCK_HAVE_MASK` yet in dd7696b97f20d756df1afa523a8f9e7bf3924c7d. This should fix the [failing CI](https://github.com/bitcoin/bitcoin/actions/runs/8870919761/job/24353246496?pr=29668)
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582963779)
This block needs to be in the next commit - we're not passing `BLOCK_HAVE_MASK` yet in dd7696b97f20d756df1afa523a8f9e7bf3924c7d. This should fix the [failing CI](https://github.com/bitcoin/bitcoin/actions/runs/8870919761/job/24353246496?pr=29668)
💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582984829)
nit
```suggestion
return GetPruneHeight(active_chainstate).value_or(-1);
```
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582984829)
nit
```suggestion
return GetPruneHeight(active_chainstate).value_or(-1);
```
💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582992037)
nit: more consistent with the (suggested) approach in `pruneblockchain`, but then "plus one (only present if pruning is enabled)" as per the docs in this RPC
```suggestion
const auto prune_height{GetPruneHeight(active_chainstate).value_or(-1)};
obj.pushKV("pruneheight", prune_height + 1);
```
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582992037)
nit: more consistent with the (suggested) approach in `pruneblockchain`, but then "plus one (only present if pruning is enabled)" as per the docs in this RPC
```suggestion
const auto prune_height{GetPruneHeight(active_chainstate).value_or(-1)};
obj.pushKV("pruneheight", prune_height + 1);
```
💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582979911)
```suggestion
//! Return height of highest block that has been pruned, or std::nullopt if no blocks have been pruned
```
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582979911)
```suggestion
//! Return height of highest block that has been pruned, or std::nullopt if no blocks have been pruned
```