After participating in a deep-dive on libbitcoinkernel, I noticed that the BlockHeader API did not provide a simple way to access the Merkle root.
This PR adds a method to the BlockHeader API that retrieves the Merkle root from a block header into a 32-byte output buffer.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36194.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, sedited, nervana21 |
| Concept ACK | jjamming |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK
Concept ACK.
I think this should be a block_header instead of a block attribute, though. I'm also not sure we need a separate btck_BlockMerkleRoot type for this? Would a simple void btck_block_header_get_merkle_root(const btck_BlockHeader* header, unsigned char output[32]) not suffice?
Would a simple void btck_block_header_get_merkle_root(const btck_BlockHeader* header, unsigned char output[32]) not suffice?
I think I'd prefer that too.
Concept ACK
Not opposed. Worth flagging generally that we shouldn't grow the API surface without a clear use, though here it's arguably completing an existing set of header accessors.
Agree with @stickies-v on approach.
Concept ACK
1316 | + 1317 | +void btck_block_merkle_root_to_bytes(const btck_BlockMerkleRoot* merkle_root, unsigned char output[32]) 1318 | +{ 1319 | + std::memcpy(output, btck_BlockMerkleRoot::get(merkle_root).begin(), 32); 1320 | +} 1321 | +
42a6ab2d74267c1aa57c7d2d14bb9b85a6592966: kernel: Expose block header Merkle root
void btck_block_merkle_root_to_bytes(const btck_BlockMerkleRoot* merkle_root, unsigned char output[32])
{
std::memcpy(output, btck_BlockMerkleRoot::get(merkle_root).begin(), 32);
}
int btck_block_merkle_root_equals(const btck_BlockMerkleRoot* merkle_root1, const btck_BlockMerkleRoot* merkle_root2)
{
return btck_BlockMerkleRoot::get(merkle_root1) == btck_BlockMerkleRoot::get(merkle_root2);
}
nit: Consider mirroring the order of the above btck_block_hash_ which flips these and instead reads _copy, _to_bytes, _equals, _destroy. The same in bitcoinkernel.h
Add an accessor that writes the block header's Merkle root to a 32-byte output buffer, and expose it through the wrapper as a fixed-size byte array
42a6ab2d74267c1aa57c7d2d14bb9b85a6592966: kernel: Expose block header Merkle root
// merkle_root sits at offset 36: version(4) + prev_hash(32)
check_equal(std::span{mainnet_block_1_header}.subspan(36, 32), merkle_root.ToBytes());
Thanks @stickies-v for the feedback, and @nervana21 for the feedback and code review.
In the current implementation, I followed the approach suggested by @stickies-v and I removed btck_BlockMerkleRoot and simplified the API so that only btck_block_header_get_merkle_root is needed to access the Merkle root.
After removing the dedicated btck_BlockMerkleRoot type and the BlockMerkleRoot class, I needed to change the return type of the MerkleRoot() method. The best approach I found was to use std::array<std::byte, 32>.
IMHO, although the current implementation is simpler, I would prefer MerkleRoot() to return a dedicated Merkle root type rather than directly std::array<std::byte, 32>.
ACK 16278c28166b0ddff1a96fdb6545850d7c921ea6
The PR description needs to be updated to reflect the current state before merging.
I would prefer
MerkleRoot()to return a dedicated Merkle root type rather than directlystd::array<std::byte, 32>.
Can you elaborate on why you'd prefer that? I personally can't see any meaningful benefits, only costs.
ACK 16278c28166b0ddff1a96fdb6545850d7c921ea6
Would also prefer this directly going to bytes as done currently. Can you correct the description? I think the last sentence can just be dropped.
Can you elaborate on why you'd prefer that? I personally can't see any meaningful benefits, only costs.
I agree that introducing it would add some extra cost. My preference was based mainly on consistency with the coding style I observed in the API. That said, I agree with keeping std::array<std::byte, 32> as the direct return type.
tACK 16278c28166b0ddff1a96fdb6545850d7c921ea6