✅ willcl-ark closed an issue: "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf"
(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

✅ willcl-ark closed a pull request: "rename bitcoin.conf in dist tarball"
(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

🤔 hebasto reviewed a pull request: "build: add `-Wundef`"
(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.

💬 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( ...`

💬 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
...

💬 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();`?

👍 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

🤔 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

💬 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

💬 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)

💬 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);
```

💬 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);
```

💬 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
```

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1582976807)
`GetFirstBlock()` does not provide any guarantees that `pprev` is not `nullptr` (nor should it), so I think we need to assert that here

💬 brunoerg commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#issuecomment-2082585841)
Concept ACK

💬 brunoerg commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1583009692)
> 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?

In this case, it is calling `addUnchecked` directly that's why it would be proper to check whether the transaction is in mempool before. Note that, in practice, this function is used in `MemPoolAccept::Finalize` which removes conflicting transactions from the mempool before adding.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583018329)
In some cases, tracepoint argument preparation can require more than a single readable line in the `TRACEPOINT` macro. An example is the transaction serialization [here](https://github.com/0xB10C/bitcoin/blob/6464cc95ca97c4535de79e52ed174e58ea0bfb18/src/txmempool.cpp#L467-L478). For context, see https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1333832906. I don't currently plan on PRing something like this, but had this running for more than a year to collect data on full-rbf RBF repla
...