💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r1993495206)
why are we iterating the clusters for all quality levels for staging?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1989793614)
I think this whole block is from a previous version and should now be removed?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1991236489)
nit: I don't think it makes a functional difference, but it's a bit weird using the const cast here (+ for `ChainParameters`, `BlockUndo`)?

🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-2675490600)
I've been looking at thread-safety, and left some comments on it (as well as some unrelated ones).

I think the API is pretty close to being thread-safe. Would be nice if we can make some guarantees on it and document it as such?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1991290582)
`kernel_context_destroy()` and `kernel_context_interrupt()` are the only places that take a non-`const` `kernel_Context`. I think we `kernel_Context`'s is no different to all other `*_destroy()` functions - in that they should never be called twice, regardless of the thread. And it seems to me that `kernel_context_interrupt()` is actually thread-safe. So, I think "but functions taking..." can be removed?

(also nit: s/non-cost/non-const/)

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1991230564)
I think this shouldn't be `const`?

```suggestion
BITCOINKERNEL_API void kernel_chain_parameters_destroy(kernel_ChainParameters* chain_parameters);
```

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1991274124)
This doesn't seem thread-safe (+ for ~all other setters). Since it seems we can't use std::atomic for most of these, adding a per-struct lock might be a good alternative?

I can't think of a sane scenario where someone would _want_ to call the same setter from multiple threads, but... it's probably better to offer the guarantees anyway?

👍 rkrux approved a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2681875100)
Concept and utACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc

> the only exceptions I could think of is PoW-verification of block hashes with the less-than (<) operator

As soon as I read the PR title, I immediately thought of this^ use case but I didn't know there were not more such use cases like this.

> But that would of course need larger, potentially more controversial changes, and its questionable if its really worth the effort.

Agree, good decision to clean up only in the tests fi
...

💬 rkrux commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993503185)
bytes -> int and then int -> bytes

type conversion is distracting anyway, all the more if not necessary

going away with this change, +1

💬 ismaelsadeeq commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2721308708)
re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e [08477fd..8ceaec](https://github.com/bitcoin/bitcoin/compare/f4344220d7195324f921dcf001c1a117008477fd..8ceaecc89e20129f9c11727e4c7795633cfcdd2e)

🤔 ismaelsadeeq reviewed a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2681978893)
ACK 40ac07ef45f6de0ac6a13e34797387a6ee140de9

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993642671)
It depends on how much effort you want to put on it.
Ideally, the function signature should clearly indicate if the procedure can fail or not. Exceptions should be reserved for unexpected situations. In this case, we're throwing an specific exception in an internal wallet function solely to notify the user about an invalid parameter provided by the RPC interface. This creates unintended coupling between components (the RPC code needs to knows about the internal wallet function throwing this spe
...

💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993586159)
Pass `onfirmed_only` to ensure that the transaction descends from a different coinbase.
Additionally, you can go further by ensuring that you have at least two confirmed UTXOs at the beginning; otherwise, split the first coinbase.
```suggestion
tx_d = self.wallet.send_self_transfer(from_node=node,
fee_rate=Decimal("0.00100"), confirmed_only=True)

```

💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993657612)
> where we're checking the raw n value

That is to check user input.

I think the abstraction level here is fine, but happy to review if you propose a further abstraction in a follow-up.

💬 theStack commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993663031)
Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn't make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.

💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993676044)
Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for `return i`. Doesn't seem worth it.

🤔 rkrux reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2682207939)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0

I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483, this https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182 has a few interesting ideas as well.

Re: https://github.com/bitcoin/bitcoin/pull/31340#pul
...

💬 mabu44 commented on pull request "contrib: Fix `gen-bitcoin-conf.sh`":
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2721511568)
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471

💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993689355)
Yes, IIRC `std::optional` wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.

💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993710323)
If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also [stackoverflow.com/a/46339933](https://stackoverflow.com/a/46339933). Struct inheritance is also fine (see discussions like [stackoverflow.com/questio
...