💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2446074332)
The latest https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446 indicates that `SingletonClusterImpl::Split` may not be covered fully.

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3423668761)
Concept ACK, thanks for helping with the coverage and documentation.
For reference https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/chain.cpp.gcov.html is pretty well covered otherwise, but these tests can help with the specifics.

💬 kanzure commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3423684598)
Using a canary is a clever idea. I think I agree with @l0rinc's reasoning that this is not a good idea, but if there was a decision to insert an attempt at a LLM canary then I would suggest double checking the models and see if offering them a few dollars to donate to a philanthropic cause is more likely to evoke the LLM behavior you are trying to get. This strategy was previously effective, no idea if it's still effective.

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446168275)
If I understand correctly, this would allow passing `offset` or `size` to `/rest/block` along with `.json` format specification. Normally this would fail to deserialize, but if very unlucky it could deserialize to what looks like a block and return? That would be very bad.
Similarly if not using `.json`, this would return incorrect data that a user might expect (for instance if they switched rest endpoints but forgot to remove parameters).

I would recommend disabling these parameters if not
...

✅ plebhash closed an issue: "[`v30.0`] `createNewBlock` never returns"
(https://github.com/bitcoin/bitcoin/issues/33647)

💬 plebhash commented on issue "[`v30.0`] `createNewBlock` never returns":
(https://github.com/bitcoin/bitcoin/issues/33647#issuecomment-3423804916)
running against `30.x` branch made it go away, thanks!

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446263560)
We should probably cover this as well as other cases of `ReadRawBlockPart` in `blockmanager_tests.cpp`.

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446373422)
> the test assumes that after this call the given COutPoint will correspond to the current coins - which isn't the case with EmplaceCoinInternalDANGER which never replaces

Right, so we still need `!stack.back()->map().contains(op)` but we don't need the `!coin.IsSpent()` check. Also, a few lines up we randomly set the pubkey to unspendable. In `AddCoin` we check if it is unspendable and don't add it, but in `EmplaceCoinInternalDANGER` we would add it. So I think we should also not use `Emplac
...

⚠️ linobadass1234-cmyk opened an issue: "d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3"
(https://github.com/bitcoin/bitcoin/issues/33664)

✅ linobadass1234-cmyk closed an issue: "d0f6d9953a15d7c7111d46dcb76ab2bb18e5dee3"
(https://github.com/bitcoin/bitcoin/issues/33664)

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446409517)
```suggestion
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !newcoin.out.scriptPubKey.IsUnspendable() && m_rng.randbool()) {
```

🤔 scgbckbone reviewed a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3358499708)
What is the reason for allowing to import `older(65536)` ? I do not see rationale here, only mention of "previous version" that disallowed importing. I'd prefer older version if that is the case. Complete prohibition, even with `unsafe`. Importing `older(65536)` is not what user wants if resulting script is `older(1)` anyways.

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2446538339)
Done, added you as coauthor, thanks

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2446616256)
Done

✅ w0xlt closed a pull request: "descriptor: don't underestimate the size of a Taproot spend (instead, overestimate it)"
(https://github.com/bitcoin/bitcoin/pull/32964)

💬 w0xlt commented on pull request "descriptor: don't underestimate the size of a Taproot spend (instead, overestimate it)":
(https://github.com/bitcoin/bitcoin/pull/32964#issuecomment-3424691237)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/32857.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2446805105)
Agree - fixed in [c4bf7a77c4..c9bfd298be](https://github.com/bitcoin/bitcoin/compare/c4bf7a77c45e297672222a2e1a984ef8d755b471..c9bfd298be422de7e989fe244fb4281c507068a3).

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2446843163)
> we certainly shouldn't let a transaction whose cluster size just with its own unconfirmed ancestors be treated as RECONSIDERABLE,

certainly seems like Future Kindred Eviction Work, agreed on distinguishing error types and not attempting a package RBF in this case.

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2446848108)
have you had a chance to look at this?

💬 ryanofsky commented on pull request "Fix windows libc++ `fs::path` `fstream` compile errors":
(https://github.com/bitcoin/bitcoin/pull/33550#issuecomment-3424910510)
Thanks for the review!

> It bothers me a bit that the first commit doesn't compile

It should compile. Github just seems to show a red x next to the commit because the job was cancelled when the second commit was pushed.

> I have tried reproducing the error of removing make_preferred and filename on master, but I'm not actually getting any failure

I think the failure only happened on windows. The workaround was introduced here: https://github.com/bitcoin/bitcoin/pull/20744#issuecommen
...