Implement `Block.get_strippedsize()` and `Transaction.get_vsize()` by visvirial · Pull Request #626 · rust-bitcoin/rust-bitcoin

visvirial · 2021-06-28T11:13:42Z

I implemented the following methods:

block.get_strippedsize(): gets the "strippedsize" of a block.
tx.get_vsize(): gets the "virtual" size of a transaction.

The test values added for this code match the values comes from Bitcoin Core.

Edit: block.get_strippedsize() will call tx.get_scaled_size() twice for every transactions in a block, which cannot be prevented without exposing an extra function in Transaction which produces the non-segwit data bytes. This will impact the performance, while the exposed API kept simple.

Edit 2: If you prefer performance over smaller API change, let me know. I will modify my PR (will add tx.get_strippedsize()).

TheBlueMatt

What's the specific use-case for this? I honestly can't think of a use for the stripped size of anything now that basically every part of bitcoin supports segwit.

TheBlueMatt · 2021-06-28T13:41:35Z

src/blockdata/transaction.rs


+    /// Gets the "vsize" of this transaction. Will be `ceil(weight / 4.0)`.
+    #[inline]
+    pub fn get_vsize(&self) -> usize {


Why do we need to expose this? Users can just divide-rounding-up themselves, and I'm not sure how much users should/do have a use for this?

tx.get_vsize() is readable and handy. Any reason for the user not to use such this instead of comming up with their own version?

TheBlueMatt · 2021-06-28T13:42:15Z

src/blockdata/transaction.rs

+    #[inline]
+    pub fn get_vsize(&self) -> usize {
+        let weight = self.get_weight();
+        (weight / WITNESS_SCALE_FACTOR) + if weight % WITNESS_SCALE_FACTOR == 0 { 0 } else { 1 }


The usual (and more efficient) way to divide-rounding-up is to add one minus the divisor and then divide, instead of branching.

Thanks for a comment. I fixed this.

TheBlueMatt · 2021-06-28T13:44:13Z

src/blockdata/block.rs

+
+    /// Get the strippedsize of the block
+    pub fn get_strippedsize(&self) -> usize {
+        let txs_size: usize = self.txdata.iter().map(|tx| {


Yes, I think we should expose a utility in transaction to get the stripped size, it would simplify this and may have value itself.

sgeisler

utACK 2085dc3

The tests seem good, I checked the stripped size and vsize that differ from the normal size against bitcoind.

@TheBlueMatt if it's in bitcoin core RPC replies then it's probably also ok to include it in rust-bitcoin imo.

visvirial · 2021-06-28T22:05:55Z

What's the specific use-case for this? I honestly can't think of a use for the stripped size of anything now that basically every part of bitcoin supports segwit.

Since Bitcoin Core emits strippedsize, rust-bitcoin also should be capable of computing this.
For use case, we should ask Bitcoin Core devs for why they are exposing strippedsize. I'm implemented this for building a blockexplorer which is compatible with Bitcoin Core's JSON API.

`Block.get_strippedsize()` is also simplified and optimized.

TheBlueMatt · 2021-06-28T22:41:51Z

Since Bitcoin Core emits strippedsize, rust-bitcoin also should be capable of computing this.

I don't think this is a good argument for doing anything. Bitcoin Core has lots of backwards compatibility concerns and ultimately are just developers, like us. Whether they decided to add anything or not isn't something above question.

I'm implemented this for building a blockexplorer which is compatible with Bitcoin Core's JSON API.

Now that's a use-case! Given this is quite simple it seems reasonable to add it for something like this.

TheBlueMatt · 2021-06-28T22:43:32Z

src/blockdata/transaction.rs

+    }
+
+    /// Gets the size of this transaction excluding the witness data.
+    pub fn get_strippedsize(&self) -> usize {


Can you add a test with a few sample transactions where you call get_strippedsize on the original transaction, then remove the witness, then check that the new vsize/weight/size methods all return the same as the original strippedsize (mod multipliers).

I don't think it is needed because test_nonsegwit_transaction() covers this, but I added this in cdf7be4.

apoelstra

ack cdf7be4

I'd prefer the fixups appear in the commits that they fix, but these commits are all small enough that it's fine.

dr-orlovsky

utACK cdf7be4

dr-orlovsky · 2021-09-10T22:01:16Z

src/blockdata/block.rs

+        80 + VarInt(self.txdata.len() as u64).len()
+    }
+
    /// Get the size of the block


Nit: this may be #[inline] (but do not discard ACKs b/c of just this)

dr-orlovsky · 2021-09-10T22:01:45Z

src/blockdata/block.rs

+    }
+
+    /// Get the strippedsize of the block
+    pub fn get_strippedsize(&self) -> usize {


Suggested change

pub fn get_strippedsize(&self) -> usize {

pub fn get_stripped_size(&self) -> usize {

just to be consistent with other function names?

dr-orlovsky · 2021-09-10T22:04:34Z

src/blockdata/transaction.rs

+    }
+
+    /// Gets the size of this transaction excluding the witness data.
+    pub fn get_strippedsize(&self) -> usize {


Ibid on naming

dr-orlovsky · 2021-09-10T22:09:18Z

I do not think my nits worth discarding existing reviews. Merging now (3 ACKs) and feel free to propose post-merge PR with these nit fixed, if you'd like to.

visvirial · 2021-09-12T11:09:24Z

Thank you for merging my PR!

darosior

Sorry for seeing this only now. The get_vsize transaction isn't correct as per Bitcoin Core's definition [0]. We already have a correct one in src/policy:

rust-bitcoin/src/policy/mod.rs

Lines 57 to 61 in 72dbe1d

    
           /// The virtual transaction size, as computed by default by bitcoind node. 
        
           pub fn get_virtual_tx_size(weight: i64, n_sigops: i64) -> i64 { 
        
               (cmp::max(weight, n_sigops * DEFAULT_BYTES_PER_SIGOP as i64) + WITNESS_SCALE_FACTOR as i64 - 1) 
        
                   / WITNESS_SCALE_FACTOR as i64 
        
           }

Sure, we could assume that users don't actually need the sigop-aware version if they are not going to craft clumsy transactions but:

get_vsize could just call get_virtual_tx_size
we could at least document the divergence

[0] Arguably there is both a BIP141 and a Core definition, and it's correct as per the former

dr-orlovsky · 2021-09-22T16:39:15Z

@darosior thanks for spotting. Probably we need a dedicated issue?

darosior · 2021-09-24T17:50:37Z

@dr-orlovsky opened a PR directly going for the simplest alternative: just document the behaviour divergence

visvirial added 2 commits June 28, 2021 20:03

Implement Block.get_strippedsize() and Transaction.get_vsize().

4ac9cef

Refactoring: define Block.get_base_size().

2085dc3

TheBlueMatt reviewed Jun 28, 2021

View reviewed changes

sgeisler previously approved these changes Jun 28, 2021

View reviewed changes

Optimize Transaction.get_vsize() (thanks @TheBlueMatt).

1bf9147

visvirial dismissed sgeisler’s stale review via 1bf9147 June 28, 2021 22:14

visvirial added 2 commits June 29, 2021 07:34

Implement Transaction.get_strippedsize().

c9dead4

`Block.get_strippedsize()` is also simplified and optimized.

Remove #[inline] from Transaction.get_strippedsize().

2bda871

TheBlueMatt reviewed Jun 28, 2021

View reviewed changes

Add extra checks for test_segwit_transaction().

cdf7be4

apoelstra approved these changes Aug 25, 2021

View reviewed changes

dr-orlovsky approved these changes Sep 10, 2021

View reviewed changes

dr-orlovsky merged commit 697e8f5 into rust-bitcoin:master Sep 10, 2021

visvirial deleted the impl-vsize branch September 12, 2021 11:09

darosior reviewed Sep 22, 2021

View reviewed changes

dr-orlovsky added this to the 0.28.0 milestone Sep 25, 2021

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

	pub fn get_strippedsize(&self) -> usize {
	pub fn get_stripped_size(&self) -> usize {

	/// The virtual transaction size, as computed by default by bitcoind node.
	pub fn get_virtual_tx_size(weight: i64, n_sigops: i64) -> i64 {
	(cmp::max(weight, n_sigops * DEFAULT_BYTES_PER_SIGOP as i64) + WITNESS_SCALE_FACTOR as i64 - 1)
	/ WITNESS_SCALE_FACTOR as i64
	}

Conversation

visvirial commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

visvirial commented Jun 28, 2021

Uh oh!

TheBlueMatt commented Jun 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Sep 10, 2021

Uh oh!

visvirial commented Sep 12, 2021

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Sep 22, 2021

Uh oh!

darosior commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

visvirial commented Jun 28, 2021 •

edited

Loading