Use weight for block size function #2040
Conversation
yancyribbens
force-pushed
the
use-weight-for-block-size-function
branch
from
f0949f1
to
2544187
Compare
|
@tcharding no problem. Could changing the return type of size be done as a separate pr if it's all the same? |
|
|
||
| /// Returns the size of the block. | ||
| /// | ||
| /// size == size of header + size of encoded transaction count + total size of transactions. | ||
| pub fn size(&self) -> usize { | ||
| let txs_size: usize = self.txdata.iter().map(Transaction::size).sum(); | ||
| self.base_size() + txs_size | ||
| self.base_size().to_wu() as usize + txs_size |
There was a problem hiding this comment.
Perhaps add something like // base_size is maximum 80 + 9 so ok to cast
There was a problem hiding this comment.
Wouldn't that comment go on L:280?
80 + VarInt::from(self.txdata.len()).len()
| @@ -693,7 +693,7 @@ impl Transaction { | |||
|
|
|||
| /// Returns the regular byte-wise consensus-serialized size of this transaction. | |||
| #[inline] | |||
| pub fn size(&self) -> usize { self.scaled_size(1) } | |||
| pub fn size(&self) -> usize { self.scaled_size(1).to_wu() as usize } | |||
There was a problem hiding this comment.
Can you justify why this cast is ok in a code comment please.
There was a problem hiding this comment.
This is surrounded by tests that can justify the change is ok. For example, if this line is changed to:
pub fn size(&self) -> usize { (self.scaled_size(1).to_wu() + 1) as usize }
then the following failures are revealed:
failures:
---- blockdata::block::tests::block_test stdout ----
thread 'blockdata::block::tests::block_test' panicked at 'assertion failed: `(left == right)`
left: `645`,
right: `643`', bitcoin/src/blockdata/block.rs:493:9
---- blockdata::block::tests::segwit_block_test stdout ----
thread 'blockdata::block::tests::segwit_block_test' panicked at 'assertion failed: `(left == right)`
left: `4334`,
right: `4319`', bitcoin/src/blockdata/block.rs:535:9
---- blockdata::transaction::tests::nonsegwit_transaction stdout ----
thread 'blockdata::transaction::tests::nonsegwit_transaction' panicked at 'assertion failed: `(left == right)`
left: `194`,
right: `193`', bitcoin/src/blockdata/transaction.rs:1398:9
---- blockdata::transaction::tests::segwit_transaction stdout ----
thread 'blockdata::transaction::tests::segwit_transaction' panicked at 'assertion failed: `(left == right)`
left: `194`,
right: `193`', bitcoin/src/blockdata/transaction.rs:1440:9
There was a problem hiding this comment.
My point was that foo as usize silently looses data, on a 32 bit machine, when foo is a u64 if foo is bigger than what fits in a u32. So the cast makes devs have to think if/why it is safe, we try not to have these casts at all in rust-bitcoin. That is why I suggested before to change all the return types in a single PR but since you want to do that separately I asked for a code comment on this line.
There was a problem hiding this comment.
This PR does not change the code semantics. Before the set of values returned from scaled_size was truncated to usize. Now scaled_size returns a weight type which is not truncated depending on arch type. However since size() still returns usize, a cast to usize is required. I can base a PR off of this PR which changes the return type of size() as well.
There was a problem hiding this comment.
I'm OK ACKing even without an explanatory comment, but I would like to revisit this and maybe add some tests which attempt to get a Weight object to exceed 2^32 and see what happens. I believe both of you that (a) this cast is sketchy and (b) it matches the existing code.
Later on we should try to create an explicit internal invariant that we're never representing more than 2^31 wu using this type. Though no doubt we will have endless bikeshedding about the specific cap.
Having said that, I think the existing code is fine for basically all applications, which compute weights either for (a) things that the author's own code produced; and (b) things that came from the blockchain or p2p network. Neither of which can come close to overflowing 2^32.
There was a problem hiding this comment.
I'm OK ACKing even without an explanatory comment, but I would like to revisit this and maybe add some tests which attempt to get a Weight object to exceed 2^32 and see what happens.
If my understanding is right, I think what your asking would be hard to test since usize will be different on different arch type. I put in a PR here to change the return type to Weight instead of usize which should prevent any ambiguity in arch types. I'm not sure what else to test..
|
If you rebase on master the 1.48 job will go green. |
yancyribbens
force-pushed
the
use-weight-for-block-size-function
branch
from
2544187
to
a68c42e
Compare
Use Weight type for
base_sizein Transaction. Also a small re-factor to removetest_and_testsfrom the testname for transaction tests.