💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3580359812)
Thanks for your reviews.
After the merge of cluster mempool, there is a better approach.
I will work on it soon, in the meantime this can tagged up for grabs.

🤔 maflcko reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3509833324)
lgtm, but i think the error handling can be improved?

lgtm ff5c3feceaba496ff25efd8420cfcc32e0864bcc 🌽

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=

...

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564089445)
```suggestion
Responds with 404 if the block or the byte range doesn't exist.
```

nit

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564233993)
I also don't see the code path here. `rest_block` is called in the scope and line below, so why would pruning have any effect here?

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2564184625)
the rest interface should be fine to expose to remote untrusted parties, no? So i am not sure about allowing them to unconditionally log confusing errors to the debug log.

i think it would be better if the caller could decide if there really was a storage corruption and an error should be logged, or if the user just had a typo in the rest url.

One way to achieve this, would be by returning a failure value.

Unfortunately, `util::Result` does not yet allow this: #25665. So maybe for now, the r
...

💬 Sjors commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3580478570)
The node log messages should contain a `destroy` entry every time the client releases a template. It's probably also a good idea to have the client emit a log message when it discards a template. If the client waits until disconnect you'll see a flood of destroy methods at that point.

If the latter isn't happening, that could suggest a bug where Bitcoin Core doesn't release the memory after the client disconnects. As suggested by @ryanofsky adding logging to `BlockTemplateImpl` and `~BlockTempl
...

📝 GarmashAlex opened a pull request: "docs: sync external-signer.md with current external signer flow"
(https://github.com/bitcoin/bitcoin/pull/33947)
Updated external-signer.md to match the current implementation. The signer network selection is passed as --chain rather than --testnet, and transaction signing is performed via stdin using the signtx command with JSON output containing a psbt field or error. Wallet creation for external signers requires external_signer=true with private keys disabled, so the example now uses named arguments. Spending with an external signer goes through the new send/sendall RPCs which build a PSBT, call the sig
...

✅ fanquake closed a pull request: "docs: sync external-signer.md with current external signer flow"
(https://github.com/bitcoin/bitcoin/pull/33947)

💬 fanquake commented on pull request "docs: sync external-signer.md with current external signer flow":
(https://github.com/bitcoin/bitcoin/pull/33947#issuecomment-3580498625)
Thanks, however there is an existing PR at #33765. I'd suggest reviewing there first.

🤔 maflcko reviewed a pull request: "fuzz: wallet: add target for `TransactionCanBeBumped`"
(https://github.com/bitcoin/bitcoin/pull/33916#pullrequestreview-3510093058)
needs rebase

💬 maflcko commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564296509)
missing trailing comma?

💬 maflcko commented on pull request "fuzz: wallet: add target for `TransactionCanBeBumped`":
(https://github.com/bitcoin/bitcoin/pull/33916#discussion_r2564297647)
clang format new code?

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3580622759)
`7826148b12...ade7fcb6a0`: in the internals of the `PrivateBroadcast` class - drop the 2 indexes "by node" and "by priority" and use just one container. When a search by node or by priority is to be done, then full scan that single container. That is `O(n)` but as suggested above we will not store huge amount of entries. Searching by priority is actually finding the one with the highest priority.

So, organize the data in a map with a key=transaction and value=sending stats per node. The map c
...

💬 maflcko commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3580635290)
review ACK c0bfe72f6e1f63e05772eda959137b3d0bbbf6c3 🍨

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c0bfe72f6e1f
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564421629)
Removed the usage of `SaltedTxidHasher` altogether. Thanks!

💬 fanquake commented on pull request "depends: Boost 1.90.0":
(https://github.com/bitcoin/bitcoin/pull/33428#issuecomment-3580642994)
Pushed this to the `1.90.0.beta1`.

💬 fanquake commented on pull request "Change Parse descriptor argument to string_view":
(https://github.com/bitcoin/bitcoin/pull/33914#issuecomment-3580645314)
cc @stickies-v

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2564432684)
I switched to own-implemented operator `<=>`. "Resolve"ing this because in the latest version a `= default`ed operator `<=>` is not suitable.

💬 TheCharlatan commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3580676738)
I tend to agree with stickies-v here. I think when it comes to setting up these options structs we should support dynamic settings. In my view that is the point of them. You get this "toggle" behaviour through the options structs, but not when passing arguments when creating the actual underlying object.

💬 TheCharlatan commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#issuecomment-3580678291)
This looks reasonable, Concept ACK