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

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583019336)
I agree that adding this makes sense as the bcc error message isn't clear. If I retouch, I'll add it.

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1583020605)
Ah I see, that makes sense now then, thanks.

🤔 furszy reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2028393990)
ACK 9552619961049d7673f84c40917b0385be70b782

💬 instagibbs commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1583032012)
```suggestion
fee_to_beat = max(int(tx_v3_child_2["fee"] * COIN), int(tx_unrelated_replacee["fee"] * COIN))
```
would you consider this instead? seems the more direct thing we're trying to accomplish

👍 hebasto approved a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028426491)
ACK cc09c165eb0bbb4b41a9132734bee55969e9d9a8.

I suggest to consider updating a help string in https://github.com/bitcoin/bitcoin/blob/a46065e36cf868265c909dc5edf29dc17be53c1f/configure.ac#L1133 to "Define to 1 if O_CLOEXEC flag is available, and to 0 if not."

As a follow up, the https://github.com/bitcoin/bitcoin/pull/16344 might be reconsidered.

👍 fanquake approved a pull request: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165#pullrequestreview-2028437917)
ACK fa1964c5b80ee28b0e06abdbd9a26e8e8c6f5acd - oss-fuzz LLVM will either be bumped globally tomorrow, or we'll land our own bump.

🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 15"
(https://github.com/bitcoin/bitcoin/pull/29165)

📝 willcl-ark opened a pull request: "gui: fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819)
As described in https://github.com/bitcoin/bitcoin/issues/10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.

Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.


...

💬 0xB10C commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2082675302)
I was made aware of https://bencher.dev/learn/engineering/sqlite-performance-tuning/ recently and the [dashboard](https://bencher.dev/perf/0xb10c-s-bitcoin-core?key=true&reports_per_page=4&branches_per_page=8&testbeds_per_page=8&benchmarks_per_page=8&reports_page=1&branches_page=1&testbeds_page=1&benchmarks_page=1&report=915c1007-33dd-4d30-b9c1-531b60af8a06&branches=0ab6caf7-716b-4a6e-8101-f2b9c891a9a2&testbeds=3c0c4d9d-b40e-4d9a-a462-2379355b0785&benchmarks=993b5444-b391-43dd-a4c1-c157cb1d7209%
...

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

Yes exactly. The correct interface for code that is doing no sanity checking would be `AcceptToMemoryPool()`. Since we're bypassing that in the fuzz tests, we should do something else to avoid putting the mempool in
...