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

Count bytes read in encoding #594

Merged
merged 1 commit into from May 1, 2021
Merged

Conversation

RCasatta
Copy link
Collaborator

This allows counting the byte read during encoding

src/consensus/encode.rs Outdated Show resolved Hide resolved
src/consensus/encode.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not achieve the same thing using https://doc.rust-lang.org/std/io/trait.Read.html#method.take? From the docs:

Creates an adaptor which will read at most limit bytes from it.

This function returns a new instance of Read which will read at most limit bytes, after which it will always return EOF(Ok(0)). Any read errors will not count towards the number of bytes read and future calls to read() may succeed.

@apoelstra
Copy link
Member

lol! I had been talking to @RCasatta about different approaches to this for the last couple of days and I totally forgot that take existed.

@apoelstra
Copy link
Member

concept ACK.

I think we should also (a) rename MAX_VEC_SIZE to MAX_OBJECT_SIZE or something, and (b) remove the existing Vec-limiting logic.

@apoelstra
Copy link
Member

Oh, no, ignore part (b). It is still necessary to prevent massive allocations.

apoelstra
apoelstra previously approved these changes Apr 21, 2021
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack d419c82

I would prefer the variable be renamed but I'm fine either way.

@RCasatta
Copy link
Collaborator Author

MAX_VEC_SIZE renamed to MAX_OBJECT_SIZE

@stevenroose
Copy link
Collaborator

Hmm, MAX_OBJECT_SIZE is only really true if we use it on every object that somehow could have a total size like that. So all objects that have vectors in them. So anything that has txs in them, etc etc.

I think the MAX_VEC_SIZE was more honest because we (though incorrectly) enforced that on vectors without making any commitments to other limitations.

dr-orlovsky
dr-orlovsky previously approved these changes Apr 22, 2021
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

sgeisler
sgeisler previously approved these changes Apr 24, 2021
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 8413706

Test log:
Apr 24 20:08:22.452  INFO testinator: Installing rust toolchain 'nightly'
Apr 24 20:08:22.919  INFO testinator: Installing rust toolchain 'stable'
Apr 24 20:08:23.233  INFO testinator: Installing rust toolchain '1.29.0'
Apr 24 20:08:23.663  INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Apr 24 20:08:23.663  INFO testinator: Preparing environment for rust stable tests (8 configurations)
Apr 24 20:08:23.663  INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Apr 24 20:10:16.056  INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.UQDUAzB58CcT/rust-bitcoin
Apr 24 20:10:16.056 DEBUG testinator: Generating lock file with rust=1.29.0
Apr 24 20:10:16.059  INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.jALZUYuvQcFn/rust-bitcoin
Apr 24 20:10:16.061  INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.a60S1I6Gmfwt/rust-bitcoin
Apr 24 20:10:17.389 DEBUG testinator: Pinning cc to 1.0.41
Apr 24 20:10:17.544 DEBUG testinator: Pinning serde to 1.0.98
Apr 24 20:10:17.763 DEBUG testinator: Pinning serde_derive to 1.0.98
Apr 24 20:10:18.053 DEBUG testinator: Pinning byteorder to 1.3.4
Apr 24 20:11:05.324  INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Apr 24 20:11:12.142  INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Apr 24 20:11:48.408  INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Apr 24 20:11:50.941  INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Apr 24 20:12:13.876  INFO testinator: Test rust=nightly, features=[base64] succeeded!
Apr 24 20:12:19.666  INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Apr 24 20:12:39.296  INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Apr 24 20:12:53.508  INFO testinator: Test rust=stable, features=[base64] succeeded!
Apr 24 20:13:06.385  INFO testinator: Test rust=nightly, features=[rand] succeeded!
Apr 24 20:13:37.570  INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Apr 24 20:13:45.885  INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Apr 24 20:13:48.154  INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Apr 24 20:14:14.275  INFO testinator: Test rust=stable, features=[rand] succeeded!
Apr 24 20:14:30.606  INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Apr 24 20:14:45.510  INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Apr 24 20:15:10.721  INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!                                                                                                                     
Apr 24 20:15:22.544  INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Apr 24 20:15:33.270  INFO testinator: Test rust=nightly, features=[] succeeded!
Apr 24 20:15:45.860  INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Apr 24 20:16:00.124  INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                                               
Apr 24 20:16:31.652  INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Apr 24 20:16:40.528  INFO testinator: Test rust=stable, features=[] succeeded!
Apr 24 20:17:45.277  INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Apr 24 20:18:40.238  INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!                                                                                                                               
Apr 24 20:19:25.394  INFO testinator: Test rust=1.29.0, features=[] succeeded!
Apr 24 20:19:27.749  INFO testinator: Fuzzing deserialize_script
Apr 24 20:20:38.237  INFO testinator: Successfully fuzzed deserialize_script
Apr 24 20:20:38.237  INFO testinator: Fuzzing uint128_fuzz
Apr 24 20:21:40.169  INFO testinator: Successfully fuzzed uint128_fuzz
Apr 24 20:21:40.169  INFO testinator: Fuzzing deserialize_amount
Apr 24 20:22:41.179  INFO testinator: Successfully fuzzed deserialize_amount
Apr 24 20:22:41.179  INFO testinator: Fuzzing deserialize_transaction
Apr 24 20:23:43.256  INFO testinator: Successfully fuzzed deserialize_transaction
Apr 24 20:23:43.256  INFO testinator: Fuzzing deser_net_msg
Apr 24 20:24:46.209  INFO testinator: Successfully fuzzed deser_net_msg
Apr 24 20:24:46.209  INFO testinator: Fuzzing deserialize_address
Apr 24 20:25:47.179  INFO testinator: Successfully fuzzed deserialize_address
Apr 24 20:25:47.179  INFO testinator: Fuzzing deserialize_block
Apr 24 20:26:49.090  INFO testinator: Successfully fuzzed deserialize_block
Apr 24 20:26:49.090  INFO testinator: Fuzzing outpoint_string
Apr 24 20:27:50.200  INFO testinator: Successfully fuzzed outpoint_string
Apr 24 20:27:50.200  INFO testinator: Fuzzing deserialize_psbt
Apr 24 20:28:53.090  INFO testinator: Successfully fuzzed deserialize_psbt

Caveat: I don't know if this PR covers all places where the check should be applied.

@apoelstra
Copy link
Member

derp, I realize that renaming the variable makes this a breaking change.

If we undo the rename we can get this in 0.26.1. Otherwise it has to wait for 0.27.

@RCasatta
Copy link
Collaborator Author

RCasatta commented Apr 28, 2021

renamed back to the original MAX_VEC_SIZE (forgetting one obviously... Now fixed...)

@dr-orlovsky
Copy link
Collaborator

This is non-API breaking, so adding label

@dr-orlovsky dr-orlovsky added the minor API Change This PR should get a minor version bump label May 1, 2021
@dr-orlovsky dr-orlovsky added this to the 0.26.1 milestone May 1, 2021
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK f692c4a

@dr-orlovsky
Copy link
Collaborator

I will try to do my first merge – since we have ACK from @apoelstra and 2 more ACKs on top and the change is quite trivial :)

@dr-orlovsky dr-orlovsky merged commit 6809624 into rust-bitcoin:master May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor API Change This PR should get a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants