💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993398098)
Let's use `uint32_t` because that is what is used in both `CTransaction` and `CMutableTransaction`.
https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L308

https://github.com/bitcoin/bitcoin/blob/72c150dfe7619c2b6e2fd06a1fa431b545a3a225/src/primitives/transaction.h#L381

Those docs might be outdated because this change was done recently: https://github.com/bitcoin/bitcoin/pull/29325

💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993472740)
See: https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1993416704

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1993480984)
Doc nit: Maybe add some docstrings here, just like currently done with BIP9Stats?

💬 l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2721197918)
> time on the x-axis rather than height

If we do that we don't even need to filter out the first 400k blocks since they're so insignificant.
What do you think of this?

<img width="1000" alt="image" src="https://github.com/user-attachments/assets/1b3fb11a-f4f4-4277-99ba-decd2c60da6a" />

💬 hodlinator commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1993519848)
My previous measurement was on Clang 19.1.7. My GCC 14.2.1 results show a similar 2.5% improvement.

<details><summary>GCC Benchmarks</summary>

#### PR

```
₿ build/src/bench/bench_bitcoin -filter=CheckBlockBench -min-time=30000

| ns/block | block/s | err% | ins/block | cyc/block | IPC | bra/block | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------
...

📝 Chand-ra opened a pull request: "test: replace hardcoded fee with node relay fee based calculation"
(https://github.com/bitcoin/bitcoin/pull/32058)
Replace the hardcoded fee of 1000 satoshis with a dynamic calculation that retrieves the node’s current minimum relay fee (minrelaytxfee) via RPC, as directed by the TODO tag.

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r1993472154)
```suggestion
void Cluster::AppendChunkFeerates(std::vector<FeeFrac>& ret) const noexcept
{
auto chunk_feerates = ChunkLinearization(m_depgraph, m_linearization);
ret.reserve(ret.size() + chunk_feerates.size());
ret.insert(ret.end(), chunk_feerates.begin(), chunk_feerates.end());
}
```

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r1993479930)
Shouldn't this be <= instead.
```suggestion
Assume(m_clustersets.size() <= 2);

```

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