💬 rahmatshahfarooqee1122-dev commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-3519084572)
257427052021105841

💬 kevkevinpal commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3519093351)
> An alternative could be to use an lvalue. However, I am not sure if this is worth it:

I think it might make most sense to continue with https://github.com/bitcoin/bitcoin/pull/33842 and to bump the min version of g++ to 12 and above as you mentioned

💬 ajtowns commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516214890)
> I don't think a virtual move assignment would actually help

Yeah. What about dropping it entirely? I think if the fuzz test is changed something like this:

```diff
--- a/src/test/fuzz/txgraph.cpp
+++ b/src/test/fuzz/txgraph.cpp
@@ -138,14 +138,14 @@ struct SimTxGraph
}

/** Add a new transaction to the simulation. */
- TxGraph::Ref* AddTransaction(const FeePerWeight& feerate)
+ TxGraph::Ref* AddTransaction(TxGraph::Ref&& ref, const FeePerWeight& feerate)
{

...

📝 frankomosh opened a pull request: "test: add unit test coverage for the empty leaves path in MerkleComputation"
(https://github.com/bitcoin/bitcoin/pull/33858)
As noted in [#32243 (comment)](https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2988854482), the early return inside `MerkleComputation` when `leaves.size() == 0` was only exercised by fuzz tests.

The existing `merkle_test_empty_block` calls `BlockMerkleRoot`, which uses `ComputeMerkleRoot`, but does not exercise the `TransactionMerklePath` → `ComputeMerklePath` → `MerkleComputation` code path.

Coverage before adding test:
<img width="2459" height="66" alt="before" src="https:
...

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516439828)
Fixed in 607b61c8b186

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516441074)
Done.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516441785)
Done in da5ddd2002bfe46f56c45b260ebda226c2fd45ee

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516442549)
Done in da5ddd2002bf

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516444966)
Incorporated in 017b21286a30374fc8ed21c3b8b92239c6cff55a

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516450486)
Done, and I also fixed `feature_dbcrash.py`.

Note that in `wallet_basic.py`, the usage of `-limitancestorcount` does have an effect on how the wallet will select inputs when constructing transactions.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516451323)
I took this change, thanks.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516463003)
I think generally you know what the txid is already, either because you would have queried for the mempool entry by txid, or because in the output of `getrawmempool` the results are keyed on txid? Is there a situation where we'd output a mempool entry and the user wouldn't know the txid?

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516469332)
Fixed, thanks for catching!

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516471068)
After further discussion I opted to make the new mempool RPC's output all chunk data in fee-per-adjusted-weight, rather than fee-per-vsize.

💬 saikiran57 commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-3520094789)
Hi, I've provided fix for this issues kindly validate pr and give your valuable comments. https://github.com/bitcoin/bitcoin/pull/31668

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2516895815)
Correct, Thanks.

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3520287169)
Thank you @TheCharlatan and @stickies-v for the review.

- Adressed [@TheCharlatan comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2507927303) to keep the variable name as state
- Adressed [@TheCharlatan comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508027260) and [@stickies-v comment](https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2514692129) on adding empty header check and entire block header interface tests.
- Addressed [@TheCharlatan
...

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2517091076)
For now I have kept a `try-catch block` and #33856 can be a potential solution. I will rebase when there's potential interest for the change. @TheCharlatan

💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2517123474)
Is theere any rteason to keep this line?

💬 yuvicc commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#issuecomment-3520360967)
Code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2

Logic correctly handles null pointers from empty spans.