🤔 rkrux reviewed a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298#pullrequestreview-3434253430)
Concept ACK a26e9fd5b5908853d37b81915f5e12a5ad822f86

Thanks for adding the fix.

💬 rkrux commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2503845007)
```diff
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -149,17 +149,16 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
assert_raises_rpc_error(-22, "TX decode failed", node2.combinerawtransaction, [rawtx2['hex'], rawtx3['hex'] + "00"])
assert_raises_rpc_error(-22, "Missing transactions", node2.combinerawtransaction, [])
assert_raises_rpc_error(-22, "Missing transactions", node2.combinerawtransaction, [rawtx2['hex']])
-
...

💬 rkrux commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2503860692)
Nit: Reading `Transaction 1` can be confusing for the user if they are not aware that 0-based indexing is being used.

💬 rkrux commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2503818549)
Nit: Can consider to be more verbose in the error message.

```diff
- throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
+ throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions. At least two transactions required.");
```

💬 rkrux commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2503802001)
> This is done to align the method with PSBT. It could be removed without breaking the change. But it also is just clearing data which should take minimal time. I think it's worth keeping to align with PSBT format.

I don't fully understand how it is aligning with PSBT, can you please explain? `GetHash()` used both here later and in `combinePSBT` RPC doesn't take the witness into account, making the removal of witness unnecessary.

https://github.com/bitcoin/bitcoin/blob/93e79181da8c952f270c4fa1
...

💬 kevkevinpal commented on pull request "test: Ignore error message give from python because of PYTHON_GIL":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2503882272)
Thanks, I updated in this commit [d848b8d](https://github.com/bitcoin/bitcoin/pull/33795/commits/d848b8dc7ecdc63927fab7706383e238e809fcba)

💬 kevkevinpal commented on pull request "test: Ignore error message give from python because of PYTHON_GIL":
(https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2503896536)
Thanks, I updated in this commit [d848b8d](https://github.com/bitcoin/bitcoin/pull/33795/commits/d848b8dc7ecdc63927fab7706383e238e809fcba)

I would mention that if any other test imports `capnp` in the future without adding the specific logic `interface_ipc.py`, then it will run into the same issue. But I think this makes sense for now

💬 Sjors commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3502947184)
@ismaelsadeeq I'm think about deprecating `getBlock()` in favour of a new `getTransactions()`. That would allow us to avoid shoehorning block templates in `CBlock`, which e.g. means we no longer need a dummy coinbase transaction.

💬 kevkevinpal commented on pull request "test: Ignore error message give from python because of PYTHON_GIL":
(https://github.com/bitcoin/bitcoin/pull/33795#issuecomment-3502952335)
Would it make sense to add a TODO to remove the warning suppression once GIL is supported in the capnp library?

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2503927416)
> Is a1a1bfa necessary?
Won't old versions be able to read the file even if estimations are sub 1sat/vb?

Yes it is necessary, old clients won't be able to read the file saved with new client, and new client won't also be able to read the file saved by old client see https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3242381132 comment with a linked PR that allows for that approach.

> just curiosity, how is this number chosen? Related to release number?

Yes but not necessary can
...

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2503896071)
What you mentioned is exactly what the comment is now.

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2503965416)
I am doing a breaking change here so I deleted it because it is indeed dummy and unused.
But you are right it is unrelated.
I add it in a separate commit.

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3503007280)
> Just some nits and questions :=)

I force pushed to address the comment, thanks for review @polespinasa

💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504009667)
Given [sv2-tp](https://github.com/stratum-mining/sv2-tp) and a few others are the users of this interface and they are still in development.

I'd rather have any un-useful methods pruned.
When we release a stable interface then we can start deprecating things to avoid breaking clients (As you mentioned in the tracking issue it is okay to make breaking changes so all these methods should be pruned and clients should update).

💬 maflcko commented on pull request "ci: Extend tidy job to cover kernel code":
(https://github.com/bitcoin/bitcoin/pull/33818#issuecomment-3503088429)
lgtm ACK 5d0a40d607d4d2902cc0372c2c9c6569dfe2d20c

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504094533)
There's a note in the tracking issue #33777 that we can drop these deprecated methods as soon as there's an important breaking change.

Even for testing it's nice that current clients works against both v30 and master.

💬 ismaelsadeeq commented on issue "Mining interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3503164270)
Why is this important?
Just because of shorhorning is not convincing imo, copying the txs is the most work which has to be done and then copy the header again.
Previously, they could access it with a single call, but after what you propose it will requires two calls.
Another downside is that they now have to calculate the block subsidy themselves, whereas previously it was included in the vout value of the first output.

🚀 fanquake merged a pull request: "ci: Extend tidy job to cover kernel code"
(https://github.com/bitcoin/bitcoin/pull/33818)

💬 maflcko commented on pull request "validation: reduce persisted UTXO set size by prioritizing positive lookups (RFC)":
(https://github.com/bitcoin/bitcoin/pull/33817#issuecomment-3503205706)
> Skipping BIP30 for those deeply buried blocks

Not sure about this. Wouldn't this mean someone can feed a `-nominimumchainwork` node a bogus chain, so that the node crashes or is stuck irrecoverably on the bogus chain?

Even if it wasn't, I am not sure if touching validation.cpp is worth it for basically a rounding error on overall IBD speed?

💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504171090)
Hmm, do you plan on deprecating it first and then removing it in a later release? On the contrary, I think it should be dropped as soon as we deem it unnecessary. Otherwise, we’ll end up with interface methods that conflict with each other. It would also be easier to review the changes together, rather than keeping them all here with deprecation comments and later deleting them it will save us review cycle.

> Even for testing it's nice that current clients works against both v30 and master.

...