💬 i-am-yuvi commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2909841408)
Concept ACK

💬 janb84 commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2107410288)
I was under the impression that forking the bitcoin repository automatically setup GHA and enabled it. That was not correct, thanks for the clarification.

🤔 furszy reviewed a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592#pullrequestreview-2868540710)
Code review ACK ee98ad2e9d2be9a72c15c5c1ef93bfd3d5ed015c

🤔 rkrux reviewed a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2866249442)
I'm sensing that, if feasible, maybe the first 4 commits can be a separate PR that removes the watch only stuff separately. Otherwise this is a bit too much to unpack in one shot - from data consistency POV.
The e6fd00372951bdc06462b74a9d078c33a5f75e78 commit contains few comment formatting related changes as well that are slightly distracting.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2105777713)
No filter passed now.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2105777223)
There is no filter passed anymore.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2105777838)
Same here.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107326256)
Same point of using logical `and` here.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107283500)
Not a bug but would prefer to have a logical `and` here instead of a bitwise one because `all_from_me` is strictly a boolean in cpp.

```
all_from_me = all_from_me && mine;
```

Ref: https://stackoverflow.com/a/24560

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107393628)
Bitwise `and` for booleans is fine in Python I guess but I find it odd to read it.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107371939)
```
enum isminetype : unsigned int {
ISMINE_NO = 0,
ISMINE_WATCH_ONLY = 1 << 0,
ISMINE_SPENDABLE = 1 << 1,
ISMINE_USED = 1 << 2,
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ALL_USED = ISMINE_ALL | ISMINE_USED,
ISMINE_ENUM_ELEMENTS,
};
```

`ISMINE_ALL` didn't include `ISMINE_USED`, i.e., it avoided used ones probably because it wanted to avoid reuse. Should it not be replaced with `/*avoid_reuse=*/true` otherwise it en
...

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107373819)
The ccf618bf3b88a95a01463e6a96fc8d9bc52fe57d message can include the boolean param name.

> This isminetype is not a real isminetype as it is never returned by
IsMine. This is only used for isminefilters in one function, which can
be better represented with a bool parameter.

💬 rkrux commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107486198)
As per this earlier statement, `avoid_reuse: true` does translate to "don't use ISMINE_USED".

https://github.com/bitcoin/bitcoin/pull/32523/files#diff-69473389a98be9232528ccdef04f9fa51ce8c5558e64994e15589be924eebae3L296

```
isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED;
```

💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107241475)
nit in 1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: The RPC result removals should probably have a release note?

💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107252018)
nit in https://github.com/bitcoin/bitcoin/commit/1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: Why is the backward-compat guard for `options.isObject()` needed? Seems better to remove `&& options.isObject()`?

💬 maflcko commented on pull request "wallet: Remove watchonly behavior and isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2107489803)
1ec04d4c4efe9e12f02f561d3c9fd23972c4a1bd: I don't think this is correct. Descriptor wallets can't mix watchonly and non-watchonly, but they still need this for size estimation.

At least locally when I try `fundrawtransaction "020000000001002d310100000000160014a3a5a66c36a754cb95d32f5489a6f856aa30586000000000" '{"fee_rate":3.331}'`, I get now a smaller fee result for a watch-only wallet.

I guess it would be good to add a test for this on current master or earlier in this pull, then fixup any
...

💬 TheCharlatan commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2107503689)
Would it be useful if we'd have a `ReadRawBlockUndo`? Similarly to `ReadRawBlock` it could just read the data without any hash integrity checks and without an extra serialization roundtrip. Our internal serialization just skips over the coinbase, but I don't feel like that would be surfacing too much detail either.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2107515289)
I can only see 5 reported errors, however, there should be 8 in total, no?

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2868754066)
light code review ACK 538e9ff804f8dfba6f6a808e83572fdeab145ab8

💬 ajtowns commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#issuecomment-2910192822)
ACK f66b14d2ecb0db991d255be49d6ff6ec361569b5