💬 josibake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004472779)
> or is this issue MULTIPROCESS=1 capnp specific?

I don't think the issue is specific to multiprocess, but rather I'm running into the issue when trying to build multiprocess. I think the root cause is as @vasild mentions, i.e., hardcoding gcc/g++. Admittedly, it _is_ weird that I'm hitting this with multiprocess but its because I am specifically trying to build in an environment without gcc/g++.

I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't w
...

🤔 hodlinator reviewed a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564#pullrequestreview-2957841876)
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004501987)
@glozow this sounds right. I wonder if we'd want to have LimitOrphans deterministically called inside `AddTx/AddAnnouncer` such that we can assert what the max amount is being trimmed, in count/size for coverage?

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166253286)
Would be good to verify the value as well:
```suggestion
assert_equal(raw['sigopsize'], <calculation>)
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166299222)
Missing punctuation (also found by Drahtbot LLM):
```suggestion
**BIP‑141 virtual size** (sometimes referred to as `vsize_bip141`) is simply the block weight divided by four, rounded up.
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166355936)
Q1: Why do we want to only conditionally report `sigopsize`, is there some comment to that affect from the old PR? I don't get that impression from https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1376980882.

Q2: In the above linked comment, do you understand what is meant by "(Or, even better, do the sigop adjustment in cases where we can -- getrawtransaction of txs in blocks could pull up the utxo being spent via rev*.dat and calculate the sigop adjustment?)"? I must admit I don't
...

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166305252)
nit: Would suggest wrapping the line width at 80 or 100 if you want to wrap it, or something closer to what other docs do in this folder.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166360506)
nit: https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1378636536 mentions adding `sigopsize` to other RPCs as well. Not sure if necessary.

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