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

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

💬 fjahr commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2004525996)
You're right, this wasn't a complete solution. I have improved this now by using buckets of signatures for each thread which are all added and only then verified by the master thread at the end. This could be further improved if we could add the signatures to a batch per thread right away but this would require that we could merge batch objects which the secp api currently doesn't make possible. I'll add that to my wish list because I think it should be even better than the current approach but
...

🤔 furszy reviewed a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-2700676883)
Could you decouple this test into its own isolated function?
The current monolithic approach makes it harder to review, even when the changes seem simple.
See for example how [wallet_migration.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_migration.py) is structured.