💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166292409)
An empty line after all headers keeps the style consistent with other markdown files in the project:
```suggestion
## Weight, Virtual Size, and Sigop‑Adjusted Size

**Transaction weight** is defined in BIP‑141 as the sum of four times the stripped transaction size plus the witness data
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166516812)
Should this value be scaled down and and rounded up as is done here?

https://github.com/bitcoin/bitcoin/blob/c8da5c1576636e31acb3422e52b8cb685dbbba64/src/policy/policy.cpp#L307

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166285774)
Whitespace change in final commit should be moved to the commit introducing this block.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166210405)
PR description nits:

---

Suggest to use of automatic GitHub linking of issues instead of making an explicit markdown link. PR description is used in the merge-commit, so good to tidy up:
```diff
- Fixes [32775](explicit link to PR)
+ Fixes #32775
```

---

Could add back-ticks for code: ``
`CTxMemPoolEntry::GetTxSize()`
``

---

Could also mention that this is somewhat of a re-take of #27591.

---

```diff
- To resolve this, all mempool-related RPCs now return two separa
...

🤔 hodlinator reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-2957038300)
Reviewed c8da5c1576636e31acb3422e52b8cb685dbbba64

👍 rkrux approved a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2957389841)
ACK 5fe7915c865a8e7c0a95ec376d0f1ff737f5d1c2
TYVM for patiently addressing all the comments in this big PR!

In the `tests: Test musig() parsing` commit, I have looked at all the unit test vectors but I have only glanced through the changes in the `DoCheck` function in the `descriptor_tests` for now.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166251339)
Thanks for using a more elaborate name, using `first` variable name here was not resonating with me.
This previous suggestion (https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2161706514) also suggested to inverse the boolean values, otherwise this block now reads "if any key is parsed, then set error", which is not right.
I can fix this in follow-up though.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166288334)
Interesting case fuzzer caught - the error message made me curious because it didn't contain the `pk` prefix and that made me debug the flow a bit. This case triggered the Miniscript parsing flow in `ParseScript` function and the error is coming from this line in `KeyParser`.
```cpp
auto pk = ParsePubkey(exp_index, {&*begin, &*end}, ParseContext(), *m_out, m_key_parsing_error);
```

I don't think this requires any change in this PR.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166272447)
Fine to use `Assume` but we still end up with an uncovered code block because it is unreachable.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166303215)
```diff
+ * @param[out] has_hardened updated if hardened derivation is found
```

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166263326)
`span` variable is used only here, I tried to pass `sp` instead and got this error.

```
note: candidate function not viable: 2nd argument ('const std::span<const char>') would lose const qualifier
19 | bool Const(const std::string& str, std::span<const char>& sp, bool skip = true);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
```
I wish there was a way to inform the compiler that the 2nd argument here is not updated if `skip` is false (which
...

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166544023)
Review note: Besides the internal use within `GetKeyIDs`, I see this function is actually used in the Musig signing PR.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166469077)
> The internal keys

I assume it's referring to the taproot internal key(s).

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2166549418)
I see that I had misunderstood your previous comment, this^ looks fine to me.

> The Sign algorithm must not be executed twice with the same secnonce. Otherwise, it is possible to extract the secret signing key from the two partial signatures output by the two executions of Sign.

This^ is the only precaution I read in BIP 327 related to reusing, and I don't think anything in the above comment goes against this precaution. Specially since using the same partial sig doesn't translate to signi
...

💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004555912)
@glozow

> Alternatively, we could add a `set` of reconsiderable wtxids

I think this is the best option. An extra index does not seem worth it, as it's 3 pointers extra per announcement, while a set of reconsiderable wtxids is just 3-4 pointers per reconsiderable wtxid. Another alternative is making the `ByWtxid` index be `(Wtxid, bool, NodeId)`, but that means that some queries (those which want to check for a specific Wtxid/NodeId announcement) need two lookups. The separate set seems mo
...

💬 fanquake commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-3004606908)
Should get a release note, and likely also going to be backported to `29.x`.

💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2166635000)
`100'000` isn't sufficient for my machine with an empty destructor. But `200'000` was for both, decreased.

💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#issuecomment-3004688762)
Thanks for highlighting.
I'll take a look at #31423 soon, it had fallen off my radar for some reason.

💬 fanquake commented on pull request "cmake: Use `HINTS` instead of `PATHS` in `find_*` commands":
(https://github.com/bitcoin/bitcoin/pull/32805#issuecomment-3004717577)
cc @purpleKarrot

💬 fanquake commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2166689603)
> If enough participants have joined the effort (5 or more is recommended)

Just noting that of the last three additions to the repo (https://github.com/asmap/asmap-data/pull/23, https://github.com/asmap/asmap-data/pull/21, https://github.com/asmap/asmap-data/pull/19), it seems like none of them reached 5: I can see 3, 3, and 3 participants.