💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1681594420)
Thank you for the correction! Applied with some minor adjustments.

🤔 paplorinc reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2183858096)
Very nice separation into commits, left some questions

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681620128)
nit: typo in commit message: `his allows keeps the secret`,`allows for passing`, `the logic for for applying`, `and the finally move`

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681626093)
nit: would it make sense to use `IsValid()` here?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681668029)
```suggestion
* expecting a `secp256k1_keypair`. This avoids the need to create a `secp256k1_keypair` object and
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681648449)
Might make the code more uniform (and maybe the diff simpler, if we'd extract it as suggested above) if instead of not making this a const cast, like the rest, we rather added the const to the method's signature:
```patch
diff --git a/src/secp256k1/include/secp256k1_extrakeys.h b/src/secp256k1/include/secp256k1_extrakeys.h
--- a/src/secp256k1/include/secp256k1_extrakeys.h (revision 21d031e7867ef97c07cee691c115d59b3aebfd19)
+++ b/src/secp256k1/include/secp256k1_extrakeys.h (date 1721245901582
...

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681661927)
```suggestion
* - Otherwise: tweak the internal key with H_TapTweak(pubkey || *merkle_root)
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681636478)
maybe it would simplify the diff if we extracted the keypair.data() here
```C++
KeyPair keypair = ComputeKeyPair();
if (!keypair.IsValid()) return false;
const secp256k1_keypair *keypairData = reinterpret_cast<const secp256k1_keypair *>(keypair.data());
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681662778)
```suggestion
* (this is used for key path spending with a specific
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681669713)
nit: typo in commit message: `Instead of calling ... instead invalidate`

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1681672646)
Might be a naive question, but is this a behavior change, does it have any security implications (e.g. could it leave keys in the memory or something)?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681676666)
this isn't making the trivial linear graph

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681678886)
Why is this benchmark "LIMO" but not `BenchLinearizeNoItersWorstCaseAnc`?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681682373)
re: `clusterlin: optimize rechunking in LinearizationChunking`

benchmark doesn't seem to show much improvement for this change. I understood this as purely an efficiency improvement which should be reflected in bench results?

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681705464)
I have incorporated something like this into the DepGraphFormatter comment.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681705729)
I have added a comment to explain this briefly.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681706300)
I have moved the connected-component logic, together with its (now optional) usage to #30286.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681706478)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681706944)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1681707363)
I don't understand this comment.