💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2901391171)
> > I've restored the `shell` option in the `subprocess` API, and employed it on non-Windows systems.
>
> Hmm the shell support implemented in [eb16060](https://github.com/bitcoin/bitcoin/commit/eb160602a50bebcca3f53cdae741e1732738d51a) actually seems somewhat broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.

Thanks! Implemented as you suggested.

(no further clean up
...

⚠️ fanquake opened an issue: "seeds: seed.bitcoin.jonasschnelli.ch not returning results"
(https://github.com/bitcoin/bitcoin/issues/32590)
Also shown by [check-dnsseeds.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/check-dnsseeds.py):
```bash
./check-dnsseeds.py
* mainnet
OK seed.bitcoin.sipa.be (39 results)
OK dnsseed.bluematt.me (31 results)
OK dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us (35 results)
OK seed.bitcoinstats.com (24 results)
FAIL seed.bitcoin.jonasschnelli.ch
OK seed.btc.petertodd.net (37 results)
OK seed.bitcoin.sprovoost.nl (36 results)
OK dnsseed.emzy.de (40 results)
OK see
...

💬 hebasto commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2901416521)
My Guix build:
```
aarch64
7a5858e3364998675e06a2bec330a6fb9f619613a9228a568b5c98cba873460f guix-build-0275825f7ace/output/aarch64-linux-gnu/SHA256SUMS.part
a8b08f5f246ca97ae26bf9142355797f82d3f1e7eb2ffbc1b7d78f35c4c0cd4d guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu-debug.tar.gz
92cbdec0549173a3e3a90ac81ad2dac98d9758a76efe12a6d205cc1265f9f701 guix-build-0275825f7ace/output/aarch64-linux-gnu/bitcoin-0275825f7ace-aarch64-linux-gnu.tar.gz
c7915b68
...

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2860445285)
Code review up to 412a9e0e69ccf461554660b7c622b768cfb8ccb3

Also did some fuzzing and mutate the new code added and tests, craches occur as expected.

> If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.

In real-world scenarios it is unlikely we encounter those transactions that will exceed the cluster count limit, however when it occur I assume that the newly connected block will have similar transaction with the reor
...

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102062249)
This method's name aligns with its functionality.

However with the addition of this commit I think there is ambiguity currently between the size oversize, the count oversize and both. This makes it a bit confusing to follow.

maybe separate the two.

oversize should be for size.

overlimit or something like it for both

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102457782)
assert that we trim when oversized ?

```suggestion
assert(!removed.empty());
auto removed_set = top_sim.MakeSet(removed);
```

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102277142)
if there is nothing in the cluster? why should it be marked as needs_split_acceptable?
Also I see no crashes after removing the new or condition.

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102494391)
check both ways
```suggestion
}
// No element exist in set and cluster is oversized
if (is_oversized && !set.Overlaps(component)){
return false;
}
```

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2102571828)
Indicate that it should not be called when their is an instance of block builder.

📝 instagibbs opened a pull request: "test: fix and augment block tests of invalid_txs"
(https://github.com/bitcoin/bitcoin/pull/32591)
Issue discovered while reviewing https://github.com/bitcoin/bitcoin/pull/32533

🤔 rkrux reviewed a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2860781255)
Initial code review f3ab751206d4c3db8696ee541f9d253ce162f295

I'm in favour of this clean up: maintaining watch-only per output is cumbersome in the code and seems complicated to me from a user's point of view as well, great that the descriptor wallets provide watch-only behaviour on the wallet level.

The CI error seems unrelated as it's a timeout in the external signer test and it's happening in other PRs as well.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275537)
IMO this was quite complicated to reason about and have in the wallet.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102275063)
Nit:
```diff
- "and relation to the wallet (ismine)."},
+ "and relation to the wallet."},

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102613956)
In ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d wallet: Remove ISMINE_USED

While I'm in favour of cleaning up the `ismine` enum and type, I find the consistent passing of `avoid_reuse` bool in all the caller functions quite cognitively loaded; the related if/else checks in the functions of this struct adds more.

I did like `CachableAmount` previously storing the amounts in an array. Keeping the array but with a different enum `CachableAmountType` would get rid of the if/else blocks and `Cachab
...

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102670556)
```diff
- /* Set some defaults for depth, spendable, solvable,
+ /* Set some defaults for depth, solvable,
```

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102737979)
Note: this is okay because this is used only during wallet migration from legacy to descriptor.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102693407)
Would removing this property from the response categorise the diff as breaking change?
Looks a little odd to me to display this as `true` for watch only wallets as well because the wallet doesn't have private keys and thus I feel it should not be a decision maker on this.

💬 theStack commented on pull request "test: fix and augment block tests of invalid_txs":
(https://github.com/bitcoin/bitcoin/pull/32591#issuecomment-2901522359)
Concept ACK, good catch!

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2102766865)
Please ignore this suggestion. I had suggested because it felt a bit odd to me to have just 1 item here but leaving `ismine` is actually helpful to give context in the help section.

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2860857095)
Posting review midway. Reviewed up to and including 980a9cd3d38abd0e0da85363056a2be6098d3919 `http: read requests from connected clients`.