Add BIP152 (Compact Blocks) structures #1088

0xB10C · 2022-07-06T15:39:49Z

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

This is a rebase of #249 by stevenroose (see #249 (comment)) with a milestone for 29.0. I've added deserialization and serialization tests for the network messages and fixed an off-by-one bug in the deserialization of the differentially encoded varints in the getblocktxn message (see #249 (comment)).

Closes #249.

0xB10C · 2022-07-06T15:46:02Z

@tcharding you had two outstanding comments in #249: #249 (comment) and #249 (comment). If they are still relevant, feel free to re-add them here and I'll address them.

apoelstra

ACK ea50200

tcharding

I can't remember right now why I suggested adding AsRef, I'll do a more full review and see if it comes out of my brain.

src/util/bip152.rs

0xB10C · 2022-07-11T15:32:58Z

Added impl_bytes_newtype!() (and impl_array_newtype!()) for ShortId as discussed in #1088 (comment). Invalidated ACK from @apoelstra.

tcharding

Two more small things, then looks good to go! Thanks for doing this man.

src/util/bip152.rs

0xB10C · 2022-07-12T16:39:20Z

Addressed #1088 (comment) and #1088 (comment). Thank you for reviewing and helping to move this forward.

apoelstra

ACK 9acd3de

tcharding

ACK 9acd3de

apoelstra · 2022-07-12T23:01:30Z

This is by far our oldest PR (if you consider it to be the same as #249) ... I feel a bit weird hitting Merge on it :)

src/util/bip152.rs

0xB10C · 2022-07-13T09:30:15Z

src/util/bip152.rs

+        let mut last_idx = 0;
+        for idx in &self.indexes {
+            len += VarInt(*idx - last_idx).consensus_encode(&mut s)?;
+            last_idx = *idx + 1;


also panics here.

I'm not sure how to handle a error during serialization. I've added a overflow check here and return an io:ErrorKind::InvalidData but this still panics: panicked at 'in-memory writers don't error: Custom { kind: InvalidData, error: "block index overflow" }', src/consensus/encode.rs:145:51...

maybe leaving as is and documenting the panic is a better solution

src/util/bip152.rs

RCasatta · 2022-07-13T09:46:12Z

src/util/bip152.rs

+        let mut prefilled = vec![];
+        let mut short_ids = vec![];


I think we can initialize the capacity of this vectors by using prefill.len() and block.txdata.len()

Specifically:

let mut prefilled = Vec::with_capacity(prefill.len() + 1); // +1 for coinbase tx let mut short_ids = Vec::with_capacity(block.txdata.len() - prefill.len() - 1);

I'm not sure why you subtract one in the short_ids capacity. This would result in a negative capacity when all transactions are pre-filled (catched by testcase 2 in test_header_and_short_ids_from_block).

I think short_ids should be:

let mut short_ids = Vec::with_capacity(block.txdata.len() - prefill.len());

hmmm, so it's possible for the coinbase tx to be inside the prefill vec?

I was judging from the logic where you were prefilled.push() when index == 0, that coinbase tx would never be passed by the prefill vec and would always be fetched from block.txdata.

Is this behavior documented somewhere?

prefill can contain coinbase tx or not, but either way, the coinbase tx will be treated as if it is prefilled etc.

I think the coinbase transaction is always pre-filled with BIP152. The receiving peer has no way of knowing the coinbase transaction before it knows the block.

From BIP 125 in the purpose column of prefilledtxn:

Used to provide the coinbase transaction and a select few which we expect a peer may be missing

https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#headerandshortids

src/util/bip152.rs

junderw · 2022-07-18T06:07:27Z

LGTM, besides 1 nit and a +1 for all of the unresolved comments.

0xB10C · 2022-07-19T11:18:58Z

addressed Add BIP152 (Compact Blocks) structures #1088 (comment): no longer panic in deserialization of BlockTransactionRequest. Also added two testcases for this.
as mentioned in Add BIP152 (Compact Blocks) structures #1088 (comment), documented that serialization can panic if the indexes vector includes u64::MAX. Docs on both the BlockTransactionRequest::indexes field and the consensus_encode() impl. Lmk if that's OK.
implemented the TODO mentioned in Add BIP152 (Compact Blocks) structures #1088 (comment)
addressed Add BIP152 (Compact Blocks) structures #1088 (comment) based on @junderw's suggestion but used Add BIP152 (Compact Blocks) structures #1088 (comment) instead
ran rustfmt on src/util/bip152.rs to address Add BIP152 (Compact Blocks) structures #1088 (comment) and to ease Introduce rustfmt in a non-invasive manner #1040
rebased on lastest master which meant:
- Upgrade bitcoin_hashes dependency #1065: replaced the BlockHash::default() occurrences
- Add new type for sequence #1093: use Sequence()

apoelstra

ACK 8d06fd1

tcharding

ACK 8d06fd1

tcharding · 2022-07-22T03:09:49Z

Super thorough, thanks @0xB10C!

apoelstra · 2022-07-22T20:27:08Z

This needs a rebase to use crate::hashes -- currently it does not build after merging.

tcharding · 2022-07-23T20:39:44Z

Ha that's funny, I couldn't for the life of me work out why it built locally without crate in the import line of hashes. I just thought I must have been being brain dead.

This adds tests for serialization of BIP152 network messages and tests specifically for the differential encoding of the transaction indicies of 'getblocktx'. Previously, this code contained an off-by-one error.

0xB10C · 2022-07-24T11:28:50Z

Rebased.

Added crate to use hashes and added the internal_macros imports required since 21a1cc7. Reran rustfmt on bip152.rs with the lastest rustfmt.toml.

tcharding

ACK ee29910

apoelstra

ACK ee29910

apoelstra · 2022-07-25T17:35:35Z

I think 3.5 years is long enough for this PR to get in :)

…uctures ee29910 BIP152: Test net msg ser/der and diff encoding (0xb10c) cd1aaaf BIP152: Add Compact Block unit test based on Elements (Steven Roose) d4d92a8 BIP152: Add Compact Blocks network messages (Steven Roose) f2fcdc8 BIP152: Add basic Compact Block structures (Steven Roose) a9a39c4 blockdata: Derive PartialOrd, Ord and Hash for BlockHeader (Steven Roose) Pull request description: > Adds the basic structures for BIP152 and a method to create a compact block from a full block. This is a rebase of #249 by stevenroose (see rust-bitcoin/rust-bitcoin#249 (comment)) with a milestone for 29.0. I've added deserialization and serialization tests for the network messages and fixed an off-by-one bug in the deserialization of the differentially encoded varints in the `getblocktxn` message (see rust-bitcoin/rust-bitcoin#249 (comment)). Closes #249. ACKs for top commit: tcharding: ACK ee29910 apoelstra: ACK ee29910 Tree-SHA512: 462a91576281f5a2ffdc2610769ea93970b60dac75a150c827966c48daec7cf93f526f9f202e7ba1dbb1410b49148579880260a3c3df298b98330c0d891a4cca

0xB10C mentioned this pull request Jul 6, 2022

Add BIP152 (Compact Blocks) structures #249

Closed

apoelstra previously approved these changes Jul 7, 2022

View reviewed changes

tcharding reviewed Jul 10, 2022

View reviewed changes

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

0xB10C dismissed apoelstra’s stale review via 6139bfc July 11, 2022 15:17

0xB10C force-pushed the compact-blocks branch from ea50200 to 6139bfc Compare July 11, 2022 15:17

tcharding reviewed Jul 12, 2022

View reviewed changes

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

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

0xB10C force-pushed the compact-blocks branch from 6139bfc to 9acd3de Compare July 12, 2022 16:37

apoelstra previously approved these changes Jul 12, 2022

View reviewed changes

tcharding previously approved these changes Jul 12, 2022

View reviewed changes

0xB10C commented Jul 13, 2022

View reviewed changes

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

0xB10C commented Jul 13, 2022

View reviewed changes

RCasatta reviewed Jul 13, 2022

View reviewed changes

tcharding added this to the 0.29.0 milestone Jul 18, 2022

junderw reviewed Jul 18, 2022

View reviewed changes

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

0xB10C dismissed stale reviews from tcharding and apoelstra via 8f705d5 July 19, 2022 09:42

0xB10C force-pushed the compact-blocks branch 3 times, most recently from dd54400 to 8d06fd1 Compare July 19, 2022 11:09

apoelstra previously approved these changes Jul 19, 2022

View reviewed changes

This was referenced Jul 20, 2022

WIP: Add fuzz test for PrefilledTransaction #873

Closed

Release tracking PR - v0.29.0 #1109

Merged

tcharding previously approved these changes Jul 22, 2022

View reviewed changes

tcharding added the release notes mention label Jul 22, 2022

stevenroose and others added 5 commits July 24, 2022 10:32

blockdata: Derive PartialOrd, Ord and Hash for BlockHeader

a9a39c4

BIP152: Add basic Compact Block structures

f2fcdc8

BIP152: Add Compact Blocks network messages

d4d92a8

BIP152: Add Compact Block unit test based on Elements

cd1aaaf

BIP152: Test net msg ser/der and diff encoding

ee29910

This adds tests for serialization of BIP152 network messages and tests specifically for the differential encoding of the transaction indicies of 'getblocktx'. Previously, this code contained an off-by-one error.

0xB10C dismissed stale reviews from tcharding and apoelstra via ee29910 July 24, 2022 11:26

0xB10C force-pushed the compact-blocks branch from 8d06fd1 to ee29910 Compare July 24, 2022 11:26

tcharding approved these changes Jul 25, 2022

View reviewed changes

apoelstra approved these changes Jul 25, 2022

View reviewed changes

apoelstra merged commit 5b70706 into rust-bitcoin:master Jul 25, 2022

0xB10C deleted the compact-blocks branch August 3, 2022 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BIP152 (Compact Blocks) structures #1088

Add BIP152 (Compact Blocks) structures #1088

0xB10C commented Jul 6, 2022 •

edited

0xB10C commented Jul 6, 2022

apoelstra left a comment

tcharding left a comment

0xB10C commented Jul 11, 2022

tcharding left a comment

0xB10C commented Jul 12, 2022

apoelstra left a comment

tcharding left a comment

apoelstra commented Jul 12, 2022

0xB10C Jul 13, 2022

0xB10C Jul 18, 2022

0xB10C Jul 18, 2022

RCasatta Jul 13, 2022

junderw Jul 18, 2022

0xB10C Jul 19, 2022 •

edited

junderw Jul 19, 2022

0xB10C Jul 19, 2022

junderw commented Jul 18, 2022

0xB10C commented Jul 19, 2022

apoelstra left a comment

tcharding left a comment

tcharding commented Jul 22, 2022

apoelstra commented Jul 22, 2022

tcharding commented Jul 23, 2022

0xB10C commented Jul 24, 2022 •

edited

tcharding left a comment

apoelstra left a comment

apoelstra commented Jul 25, 2022

Add BIP152 (Compact Blocks) structures #1088

Add BIP152 (Compact Blocks) structures #1088

Conversation

0xB10C commented Jul 6, 2022 • edited

0xB10C commented Jul 6, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

0xB10C commented Jul 11, 2022

tcharding left a comment

Choose a reason for hiding this comment

0xB10C commented Jul 12, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 12, 2022

0xB10C Jul 13, 2022

Choose a reason for hiding this comment

0xB10C Jul 18, 2022

Choose a reason for hiding this comment

0xB10C Jul 18, 2022

Choose a reason for hiding this comment

RCasatta Jul 13, 2022

Choose a reason for hiding this comment

junderw Jul 18, 2022

Choose a reason for hiding this comment

0xB10C Jul 19, 2022 • edited

Choose a reason for hiding this comment

junderw Jul 19, 2022

Choose a reason for hiding this comment

0xB10C Jul 19, 2022

Choose a reason for hiding this comment

junderw commented Jul 18, 2022

0xB10C commented Jul 19, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Jul 22, 2022

apoelstra commented Jul 22, 2022

tcharding commented Jul 23, 2022

0xB10C commented Jul 24, 2022 • edited

tcharding left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 25, 2022

0xB10C commented Jul 6, 2022 •

edited

0xB10C Jul 19, 2022 •

edited

0xB10C commented Jul 24, 2022 •

edited