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

💬 maflcko commented on pull request "test: add unit test coverage for the empty leaves path in MerkleComputation":
(https://github.com/bitcoin/bitcoin/pull/33858#issuecomment-3520585217)
> Coverage before adding test: <img alt="before" width="2000" height="66" src="https://github.com/user-attachments/assets/28cb35b1-5127-4f82-ba74-5891bd36553c">
>
> Coverage after adding test: <img alt="after" width="2000" height="66" src="https://github.com/user-attachments/assets/41ebbd66-872e-4ba4-b550-84ff9bc61752">

I don't think those links work. Also, this is redundant to the corecheck result anyway?

👋 maflcko's pull request is ready for review: "ci: Enable experimental kernel stuff in most CI tasks"
(https://github.com/bitcoin/bitcoin/pull/33824)

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3520618909)
> we do getBlock and that provides everything we need

Ok, that's not really how you're supposed to use it :-)

It does make a breaking change potentially unpainful.

However there's another gotcha if we remove a method from the interface: there's no gaps allowed in the `.capnp` method numbering. This could be worked around as follows:

```capnp
interface BlockTemplate $Proxy.wrap("interfaces::BlockTemplate") {
destroy @0 (context :Proxy.Context) -> ();
getBlockHeader @1 (cont
...

👍 TheCharlatan approved a pull request: "build: Bump g++ minimum supported version to 12"
(https://github.com/bitcoin/bitcoin/pull/33842#pullrequestreview-3452188347)
Re-ACK fa9f29a4a79944f6ffbb58eab0ac41e243fbeb97

🤔 hodlinator reviewed a pull request: "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState"
(https://github.com/bitcoin/bitcoin/pull/33856#pullrequestreview-3452156315)
Concept ACK moving out-reference-parameters to return value

Not familiar with the kernel API but had a brief look at net_processing and validation.

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517333716)
Should remove the shadowing `state` on line 4515 IMO.

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517330536)
Why not reuse `state` here?

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2517354777)
Worth adding an `Assume(!headers.empty())` before this, as `state` will be unset if we don't process any, whereas before we would return `true` regardless?

💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517387755)
I don't think there was ever a `signtransaction`, it was just a typo in the docs. The external signer feature was introduced in v22. The docs from that time mentions `signtransaction` and not `signtx`: https://github.com/bitcoin/bitcoin/blob/v22.0/doc/external-signer.md

Looking at the code from v22.0 however, the actual command was already `signtx`.

💬 Sjors commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2517395290)
`signtx` is not inherently stdin

All (required) commands should work with or without `--stdin`. Bitcoin Core currently only uses it for `signtx`, and that's worth mentioning, but it can change.