Move `merkleblock` into `merkle_tree` #1374

tcharding · 2022-11-08T00:14:17Z

Re-done after review comments below. This is now PR 1 in the merkle_tree::block series :)

Move the merkleblock module into the merkle_tree module in a submodule called block. In order to do the minimum amount of changes in this patch DO NOT rename types to improved naming and reduce stutter.

Note:

block module is private
the three types are re-exported from merkle_block
the MerkleBlock re-export from the crate root is left in place.

This patch purposefully does the minimum amount of changes because there a whole bunch of improvements to the old "merkleblock" module that are coming next in a separate PR.

Kixunil

Concept ACK, not sure about naming.

Kixunil · 2022-11-09T11:58:39Z

bitcoin/src/network/message.rs

@@ -17,14 +17,13 @@ use io::Read as _;
 use crate::blockdata::block;
 use crate::blockdata::transaction;
 use crate::network::address::{Address, AddrV2Message};
-use crate::network::{message_network, message_bloom};
+use crate::network::{message_network, message_bloom, message_merkleblock};


Why is the module called message_merkleblock instead of just merkleblock?

To fit in with the names of all the others. FTR I think they all need renaming though.

I could rename them all as an initial patch if that is desired?

apoelstra · 2022-11-12T23:48:33Z

I understand merkleblock as providing the structure used by the gettxoutproof RPC call, which has nothing to do with the network layer. So I don't think this should be moved.

kcalvinalvin · 2022-11-13T07:20:24Z

I understand merkleblock as providing the structure used by the gettxoutproof RPC call, which has nothing to do with the network layer. So I don't think this should be moved.

I think it's used both for supporting bloom filter style spv in the network layer and for the gettxoutproof rpc call. I think the default for signaling bloom filter support was turned off but it's still supported by core and users can turn it on if they want.

Network related use for CMerkleBlock:
https://github.com/bitcoin/bitcoin/blob/7ef730ca84bd71a06f986ae7070e7b2ac8e47582/src/net_processing.cpp#L2162-L2183

rpc related use for CMerkleBlock:
https://github.com/bitcoin/bitcoin/blob/7ef730ca84bd71a06f986ae7070e7b2ac8e47582/src/rpc/txoutproof.cpp#L140-L143

Though since its use is for both rpc and network, maybe it's better to leave it in util?

tcharding · 2022-11-13T21:09:45Z

Thanks for bringing that to my attention, I missed the RPC usage when I did this.

Though since its use is for both rpc and network, maybe it's better to leave it in util?

We are trying to get rid of the util module altogether.

So I don't think this should be moved.

Any suggestion as to where it could live? Note we have src/merkle_tree.rs, this raises questions:

Inconsistent naming merkeblock.rs with no underscore merkle_tree.rs with underscore
Why merkle tree and merkle block - I guess improved documentation at the head of each file can help with this.

Kixunil · 2022-11-14T11:04:08Z

If we don't want to have it in network then merkle_tree sounds best. merkle_tree::Block seems good to me but internally we want merkle_tree/block.rs with pub use

apoelstra · 2022-11-14T14:13:14Z

I think eventually it should go in the crypto/ module. For now I'd be okay having it in the root. No matter what we do I think this type will be poorly discoverable because it doesn't have a "real" name other than maybe CMerkleBlock from the Bitcoin Core source :P. I didn't even know it was also used in the network layer.

We could maybe create an example in the top-level lib.rs of parsing/validating the output of gettxoutproof ... but that seems like a pretty niche use case, so I dunno.

But I'm happy with any names that you guys are. Agree that merkleblock is weird.

Kixunil · 2022-11-14T14:35:43Z

Inspired me to open #1386

tcharding · 2022-11-14T18:00:42Z

I think eventually it should go in the crypto/ module.

Drats, that is where all this started, I had moved it there and then second guessed myself.

tcharding · 2022-11-14T18:03:01Z

Another idea I had was to move the code into merlke_tree.rs but this makes the licensing stuff a bit bothersome, could create a merkle_tree/ directory and do

move merkle_tree.rs code to merkle_tree/mod.rs
move merbleblock.rs code to merkle_tree/merkle_block.rs and document its only there because the file author and license blurb are slightly different

And then move the whole thing to crypto/?

apoelstra · 2022-11-14T20:57:13Z

Nah, I think it's fine to just move the license blurb to a comment on the MerkleBlock data structure itself, and clarify that the "this was translated from Bitcoin Core MIT code" applies to the MerkleBlock structure and all methods implemented on it in this file.

We could also ask Pieter (who authored the original code in 4bedfa9223d3 bitcoin/bitcoin#1795 commit 14/17) to relicense it if we want to be totally sure of things.

tcharding · 2022-11-14T21:29:22Z

If we don't want to have it in network then merkle_tree sounds best. merkle_tree::Block seems good to me but internally we want merkle_tree/block.rs with pub use

Woops, some how I missed this comment this morning. With the license clarification from @apoelstra and this comment I'll re-spin the PR, thanks.

In preparation for moving `MerkleBlock` into the `merkle_tree` module; create a new directory for the module and move `merkle_tree.rs` to `merkle_tree/mod.rs`.

Move the `merkleblock` module into the `merkle_tree` module in a submodule called `block`. In order to do the minimum amount of changes in this patch DO NOT rename types to improved naming and reduce stutter. Note: - block module is private - the three types are re-exported from `merkle_block` This patch purposefully does the minimum amount of changes because there a whole bunch of improvements to the old "merkleblock" module that are coming next.

Kixunil

ACK 6131072

Will be happy to re-review if the path changes to relative.

Kixunil · 2022-11-15T09:35:13Z

bitcoin/src/merkle_tree/block.rs

@@ -53,7 +53,7 @@ use crate::blockdata::block::{self, Block};
 use crate::blockdata::transaction::Transaction;
 use crate::blockdata::constants::{MAX_BLOCK_WEIGHT, MIN_TRANSACTION_WEIGHT};
 use crate::consensus::encode::{self, Decodable, Encodable};
-use crate::util::merkleblock::MerkleBlockError::*;
+use crate::merkle_tree::MerkleBlockError::*;


WTF, this should've been self.

Hah! the number of times I changed this to self in the last few weeks. Eventually I tried to make the diff as unsurprising with minimal changes as possible.

My rule of thumb is to never use absolute path that starts with the equivalent of self. If this was self from the beginning this diff line wouldn't be here now.

Cool, I'll make that my own policy as well, cheers.

apoelstra

ACK 6131072

861fdd6 Put the `MerkleBlock` struct at the top of the file (Tobin C. Harding) f0d9681 Put error at the bottom of the file (Tobin C. Harding) 19e0947 Use self for Error variant imports (Tobin C. Harding) 83c2a55 Put helper function below where its called (Tobin C. Harding) 5076579 Fix indentation in pmt_tests macro (Tobin C. Harding) a7edbfb Move hex data to tests/data (Tobin C. Harding) Pull request description: PR 2 in the `merkle_tree::block` series, used to be on top of the now merged #1374. Do a bunch of refactorings in preparation for more invasive changes. This is a separate PR because, other than the first patch which moves hex strings to `tests/data/` the other patches are refactoring only patches, no logic changes. However the last patch is big and will be annoying to review - sorry about that. If you really oppose this basically stylistic patch putting important things first, the opposite of C code, please say and I'll try to stop doing it. ACKs for top commit: Kixunil: ACK 861fdd6 apoelstra: ACK 861fdd6 Tree-SHA512: 3da0a600898f490b602ab05a396061587d86ffef55697877885a8c611eff96e7382a2d816fe9594c100d378dc56fe7fdc88009a0343bc602b7f4c180836adbd3

tcharding mentioned this pull request Nov 8, 2022

Create crypto module #1260

Merged

tcharding force-pushed the 11-08-move-merkleblock-module branch from b48c7b5 to 6ff9d47 Compare November 8, 2022 00:19

Kixunil reviewed Nov 9, 2022

View reviewed changes

tcharding added 2 commits November 15, 2022 10:48

Move merkle_tree.rs to merkle_tree/mod.rs

c89d9c4

In preparation for moving `MerkleBlock` into the `merkle_tree` module; create a new directory for the module and move `merkle_tree.rs` to `merkle_tree/mod.rs`.

tcharding force-pushed the 11-08-move-merkleblock-module branch from 6ff9d47 to 6131072 Compare November 15, 2022 00:20

tcharding changed the title ~~Move merkleblock module to network~~ Move merkleblock into merkle_tree Nov 15, 2022

This was referenced Nov 15, 2022

Refactor merkle_tree::block #1388

Merged

Add PartialMerkleTree::calc_tree_height method #1389

Merged

Improve error handling in the merkle_tree::block module #1390

Closed

Kixunil approved these changes Nov 15, 2022

View reviewed changes

apoelstra approved these changes Nov 15, 2022

View reviewed changes

apoelstra merged commit 70eb92c into rust-bitcoin:master Nov 15, 2022

tcharding deleted the 11-08-move-merkleblock-module branch November 15, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `merkleblock` into `merkle_tree` #1374

Move `merkleblock` into `merkle_tree` #1374

tcharding commented Nov 8, 2022 •

edited

Kixunil left a comment

Kixunil Nov 9, 2022

tcharding Nov 9, 2022

tcharding Nov 9, 2022

apoelstra commented Nov 12, 2022

kcalvinalvin commented Nov 13, 2022

tcharding commented Nov 13, 2022 •

edited

Kixunil commented Nov 14, 2022

apoelstra commented Nov 14, 2022

Kixunil commented Nov 14, 2022

tcharding commented Nov 14, 2022

tcharding commented Nov 14, 2022 •

edited

apoelstra commented Nov 14, 2022

tcharding commented Nov 14, 2022

Kixunil left a comment

Kixunil Nov 15, 2022

tcharding Nov 15, 2022

Kixunil Nov 16, 2022

tcharding Nov 16, 2022

apoelstra left a comment

Move merkleblock into merkle_tree #1374

Move merkleblock into merkle_tree #1374

Conversation

tcharding commented Nov 8, 2022 • edited

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Nov 9, 2022

Choose a reason for hiding this comment

tcharding Nov 9, 2022

Choose a reason for hiding this comment

tcharding Nov 9, 2022

Choose a reason for hiding this comment

apoelstra commented Nov 12, 2022

kcalvinalvin commented Nov 13, 2022

tcharding commented Nov 13, 2022 • edited

Kixunil commented Nov 14, 2022

apoelstra commented Nov 14, 2022

Kixunil commented Nov 14, 2022

tcharding commented Nov 14, 2022

tcharding commented Nov 14, 2022 • edited

apoelstra commented Nov 14, 2022

tcharding commented Nov 14, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Nov 15, 2022

Choose a reason for hiding this comment

tcharding Nov 15, 2022

Choose a reason for hiding this comment

Kixunil Nov 16, 2022

Choose a reason for hiding this comment

tcharding Nov 16, 2022

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Move `merkleblock` into `merkle_tree` #1374

Move `merkleblock` into `merkle_tree` #1374

tcharding commented Nov 8, 2022 •

edited

tcharding commented Nov 13, 2022 •

edited

tcharding commented Nov 14, 2022 •

edited