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

💬 laanwj commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2446862162)
i also prefer mapping the enum value to a string, instead of repeating the log call.

💬 laanwj commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#issuecomment-3424937092)
Concept and code review ACK 07a926474b5a6fa1d3d4656362a0117611f6da2f. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.

💬 ismaelsadeeq commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2440265397)
In "Add test case for cluster size limits to TRUC logic" cf2f5211a8fab1438feb1cf1ccb86dccedd4b6c8

also add a test for cluster count

```suggestion
self.log.info("Test that a decreased limitclustercount also applies to TRUC cluster")
self.restart_node(0, extra_args=["-limitclustercount=1", "-acceptnonstdtxn=1"])
assert_raises_rpc_error(-26, "too-large-cluster", node.sendrawtransaction, tx_v3_child_large2["hex"])
self.check_mempool([tx_v3_parent_large2["tx
...