Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 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.
💬 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
...