🤔 furszy reviewed a pull request: "Including exception what() in Runaway dialog box"
(https://github.com/bitcoin-core/gui/pull/876#pullrequestreview-2868183402)
> If an exception happens, including e.what() in the text passed to the dialog
box, so that the user can see it. This is in addition to any text from
getWarnings() that might already be present, separated by newlines if so.

Have you verified that we won't output the same error message twice?

💬 furszy commented on pull request "Including exception what() in Runaway dialog box":
(https://github.com/bitcoin-core/gui/pull/876#discussion_r2107175788)
const std::string&

💬 furszy commented on pull request "Including exception what() in Runaway dialog box":
(https://github.com/bitcoin-core/gui/pull/876#discussion_r2107175290)
`const std::string&`

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