💬 willcl-ark commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2738172847)
I also cannot reproduce OP:

```bash
will@ubuntu in …/src/core/bitcoin on  master [$!?] via △ v3.31.6 : 🐍 (core)
$ git diff -p | cat
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index a35306b6935..d2904fb3f14 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1480,7 +1480,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
uint8_t ext_flag, key_version;
switch (sigversion) {
case SigVersion::TAPR
...

🤔 sipa reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2700118245)
Big change: `std::vector<ClusterSet>` is gone, replaced by `m_main_clusterset` and `m_staging_clusterset`.
* New helper functions `GetTopLevel()`, `GetSpecifiedLevel()`, `GetClusterSet()` made this fairly easy to do.
* Simplifications resulted in `ClearLocator`, `GetConflicts`, `PullIn`, `MakeTransactionsMissing`, `StartStaging`, `CommitStaging`, `ExtractCluster`.
* `CopyTo` was renamed to `CopyToStaging`.
* `LevelDown` was renamed to `MoveToMain`.
* `MakeTransactionsMissing` was renamed to
...

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004315650)
`NEEDS_SPLIT_OPTIMAL` is gone now after further discussion. Marking resolved.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004306838)
@ajtowns That comment was put in the wrong commit. I've fixed it.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004287728)
Late update (discussed elsewhere): this property actually did not hold. Added comments, and partially rewritten.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004304219)
Done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004308947)
I added the following comment:

```
// This is a big simulation test for TxGraph, which performs a fuzz-derived sequence of valid
// operations on a TxGraph instance, as well as on a simpler (mostly) reimplementation (see
// SimTxGraph above), comparing the outcome of functions that return a result, and finally
// performing a full comparison between the two.
```

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004286166)
See my comment here: https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2734449766 about how `Assume()` gives you compiler-guaranteed side-effect-equivalence, largely without runtime cost.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004312349)
Together with a lot of changes (getting rid of the `ClusterSet` vector), done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004316851)
Done.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004313231)
This code is gone now, resolving.

💬 sipa commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2738185787)
@Chand-ra The taproot script unit tests are largely, and don't live in this repository. You need to clone the QA assets repository (https://github.com/bitcoin-core/qa-assets) and then specify the location of its `unit_test_data` directory in the `DIR_UNIT_TEST_DATA` environment variable when running the unit test binary.

🤔 mzumsande reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2699856438)
Concept ACK
only reviewed up to 199a2eb87c3ad9382f2fe4ae97524099224a7b04 so far.

💬 mzumsande commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004115942)
df99f19d81537ecfbf66f6662bc9814b594ef8d2:
I was trying to understand why this fix is correct / the height suddenly changes from 109 to 110, and I find the entire `chainstatemanager_snapshot_init` subtest very confusing:
First, there is a comment saying "Test that simulating a shutdown (...) we end up with a single chainstate that is at tip", followed by a check that there are still 2 chainstates - so the test contradicts itself.

Then the test calls the test-only function `LoadVerifyActiva
...

💬 mzumsande commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2004288056)
a1fd5a886252ebd813ee69029dcf17de6883e047:
Why is `m_target_utxohash` a `std::optional<AssumeutxoHash>` if it's only ever used as a bool? The hash, once set , is never read anywhere as far as I can see.

nit: it should be `m_target_utxohash` instead of `m_target_blockhash` in the commit message.

👍 theuni approved a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2700353780)
Thanks for addressing my comment.

I agree, this will be a fun one to get merged. I have several local branches with fun changes on top of this one.

ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.

💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004380489)
Can the `m_best_header` update logic be merged into the while loop that handles the `nStatus` updates? Not sure if it is necessarily better; but I was thinking of something like:

Adding this just before the while loop:
```c++
const bool best_header_needs_update = m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip;
if (best_header_needs_update) {
m_chainman.m_best_header = invalid_walk_tip->pprev;
}
```

And then here:
```suggestion
if
...

💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004381740)
Shouldn't we mark the candidate dirty after this?

💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004383630)
Small typo at the end of the comment

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2738349197)
> It could enable other approaches to building bindings and using kernel code in external projects.

I have done some experimentation with using c++ bindings directly.

> [SWIG](https://www.swig.org/)

I tried creating python bindings through swig without looping through the C bindings and I could not get it to work within a reasonable amount of time. While likely a skill issue, it does seem to be struggling with some of our c++20 features and heavily templated code.

> [pybind](https:
...

💬 tdb3 commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2004449128)
Updated. Thanks.