Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BIP152 (Compact Blocks) structures #249

Open
wants to merge 3 commits into
base: master
from

Conversation

@stevenroose
Copy link
Collaborator

stevenroose commented Mar 24, 2019

Adds the basic structures for BIP152 and a method to create a compact block from a full block.

Depends on rust-bitcoin/bitcoin_hashes#46.

@stevenroose stevenroose changed the title Implement BIP152 (Compact Blocks) structures Add BIP152 (Compact Blocks) structures Mar 24, 2019
@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from 990a31d to c8d0814 Apr 1, 2019
@stevenroose stevenroose requested review from tamasblummer and dongcarl Apr 1, 2019
Cargo.toml Outdated Show resolved Hide resolved
.map(|tx| {
ShortId::calculate_with_siphash_keys(
&match use_wtxid {
true => tx.bitcoin_hash(),

This comment has been minimized.

Copy link
@tamasblummer

tamasblummer Apr 1, 2019

Member

SendCmpct version and this should be better linked somehow.

This comment has been minimized.

Copy link
@nothingmuch

nothingmuch Apr 30, 2019

how does SendCmpct relate to this? assuming CmpctBlock instead of SendCmpct, isn't this only code path to constructing it in this PR -- CmpctBlock{ compact_block: HeaderAndShortIds::from_block(...) }? Or are you referring to the parallels between this function and BlockTransactions::from_request?

This comment has been minimized.

Copy link
@stevenroose

stevenroose May 30, 2019

Author Collaborator

I think he is referring to the fact that the use_wtxid should be linked to the version field (1 -> false; 2 -> true) somehow. I'll change that to passing the version number and return an Error if it's not 1 or 2.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 26, 2019

So I understand we don't want a siphasher dependency, but rather a bitcoin_hashes::siphash impl?

@tamasblummer

This comment has been minimized.

Copy link
Member

tamasblummer commented Apr 26, 2019

yes, siphash is also used in BIP158. It would be better included into bitcoin_hashes

.map(|tx| {
ShortId::calculate_with_siphash_keys(
&match use_wtxid {
true => tx.bitcoin_hash(),

This comment has been minimized.

Copy link
@nothingmuch

nothingmuch Apr 30, 2019

how does SendCmpct relate to this? assuming CmpctBlock instead of SendCmpct, isn't this only code path to constructing it in this PR -- CmpctBlock{ compact_block: HeaderAndShortIds::from_block(...) }? Or are you referring to the parallels between this function and BlockTransactions::from_request?

src/util/bip152.rs Outdated Show resolved Hide resolved
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented May 14, 2019

yes, siphash is also used in BIP158. It would be better included into bitcoin_hashes

@tamasblummer
I added a siphash24 module in bitcoin_hashes: rust-bitcoin/bitcoin_hashes#46

Could you check if that suits your need for BIP 158 usage? I'm actually very interested in BIP-158 implementation in rust-bitcoin. Are you planning to do them?

@tamasblummer

This comment has been minimized.

Copy link
Member

tamasblummer commented May 14, 2019

@stevenroose will review tomorrow. I already implemented and using BIP158 in murmel: https://github.com/rust-bitcoin/murmel/blob/master/src/blockfilter.rs

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from c8d0814 to 7a70807 May 17, 2019
@stevenroose stevenroose changed the title Add BIP152 (Compact Blocks) structures WIP: Add BIP152 (Compact Blocks) structures May 17, 2019
@stevenroose stevenroose changed the title WIP: Add BIP152 (Compact Blocks) structures BLOCKED: Add BIP152 (Compact Blocks) structures May 17, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented May 17, 2019

I already implemented and using BIP158 in murmel: https://github.com/rust-bitcoin/murmel/blob/master/src/blockfilter.rs

@tamasblummer would you mind upstreaming that to rust-bitcoin? Into src/util/bip185.rs f.e.?

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch 2 times, most recently from 3a12d50 to 50717b1 May 30, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented May 30, 2019

@tamasblummer I made 2 changes:

  • HeaderAndShortIds::from_block now takes a version number instead of the use_wtxid bool. It returns an error if the version is not 1 or 2. This is probably better for backwards compatibility when we add support for other versions.
  • HeaderAndShortIds::from_block now takes an additional &[bool] argument to indicate which transactions to prefill. It must be of length one less than the nb of txs in the block because the coinbase is always prefilled. This is quite rudimentary support for arbitrary fillings. I thought about a closure argument Fn(&sha256d::Hash) -> bool. One could pass a HashSet::contains or HashMap::contains_key into that argument, f.e., as well as a custom method. Would that be a better solution? Thought? @apoelstra @dongcarl
@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from 50717b1 to a6beaed May 30, 2019
@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from a6beaed to 2ed6915 Jul 6, 2019
@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from 2ed6915 to 805aec6 Jul 26, 2019
@stevenroose stevenroose changed the title BLOCKED: Add BIP152 (Compact Blocks) structures Add BIP152 (Compact Blocks) structures Jul 26, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Jul 26, 2019

This is rebased and ready to be reviewed. Uses the new bitcoin_hashes siphash impl.

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from 805aec6 to f394fe0 Jul 26, 2019
@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from f394fe0 to b6ff8bb Oct 10, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Oct 10, 2019

rebased

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from 0503d46 to eaccaf1 Oct 24, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Oct 24, 2019

fixed silly rebasing issue

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch from eaccaf1 to 913b271 Oct 28, 2019
@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Oct 28, 2019

This PR is passing the tests (except for the one that also fails on master).

/// A PrefilledTransaction structure is used in HeaderAndShortIDs to
/// provide a list of a few transactions explicitly.
#[derive(PartialEq, Eq, Clone, Debug)]
pub struct PrefilledTransaction(pub u64, pub Transaction);

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 5, 2019

Member

May be more efficient for users for this to hold a reference to the Transaction.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Nov 19, 2019

Author Collaborator

This optimization is only possible in the case where you're encoding a compact block, but (1) those are usually transported out of the context so they can't have lifetimes of the original block and (2) by default only the coinbase tx is prefilled and in most cases the nb of prefilled txs will be minimal.

block_hash: sha256d::Hash::consensus_decode(&mut d)?,
indexes: {
// Manually decode indexes because they are differentially encoded VarInts.
let differential: Vec<VarInt> = Decodable::consensus_decode(&mut d)?;

This comment has been minimized.

Copy link
@apoelstra

apoelstra Nov 5, 2019

Member

Not going to block this PR on it, but this could be done much more efficiently by avoiding the temporary Vec.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Nov 19, 2019

Author Collaborator

Done in separate commit.

Copy link
Member

apoelstra left a comment

utACK.

Willing to merge even with nits; this has been open for a while and I'd like to get it in. Efficiency improvements can come in later PRs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 19, 2019

Codecov Report

Merging #249 into master will decrease coverage by 1.13%.
The diff coverage is 40.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   81.89%   80.76%   -1.14%     
==========================================
  Files          38       40       +2     
  Lines        7169     7209      +40     
==========================================
- Hits         5871     5822      -49     
- Misses       1298     1387      +89
Impacted Files Coverage Δ
src/network/mod.rs 0% <ø> (ø) ⬆️
src/consensus/encode.rs 76.77% <ø> (ø) ⬆️
src/util/mod.rs 0% <ø> (ø) ⬆️
src/network/message_blockdata.rs 35.29% <0%> (-1.07%) ⬇️
src/network/message_compact_blocks.rs 0% <0%> (ø)
src/network/message.rs 58.84% <25%> (-9.75%) ⬇️
src/util/bip152.rs 45.33% <45.33%> (ø)
src/network/message_network.rs 25% <0%> (-10%) ⬇️
src/network/stream_reader.rs 95.62% <0%> (-2.06%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ac252...65cc14e. Read the comment docs.

@stevenroose stevenroose force-pushed the stevenroose:compact-blocks branch 2 times, most recently from 87636d5 to c67321e Dec 19, 2019
stevenroose added 3 commits Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.