Ready for Review: Clean up `network::encodable` and `network::serialize` by dongcarl · Pull Request #156 · rust-bitcoin/rust-bitcoin

dongcarl · 2018-08-27T09:59:22Z

Closes: #155

dongcarl · 2018-09-08T00:17:19Z

Rebased.

dongcarl · 2018-09-13T18:19:46Z

Rebased.

apoelstra · 2018-09-16T15:52:59Z

src/util/hash.rs

Is this used anywhere? It's pretty surprising that the "bitcoin hash" of a Vec does not include its length prefix.

Used here but doesn't seem to be needed?

Like, the merkle_root() method calls .bitcoin_hash() on a Vec<u8>? Yeah, that should be made more explicit.

Can you add a commit fixing merkle_root to use Sha256dEncoder and then fix this commit so it doesn't add useless code?

Addressed in: ba84548c8e13a4fd83dde417970cd5f2e9e995c6

Not sure which commit you mean?

apoelstra · 2018-09-16T15:54:10Z

src/consensus/mod.rs

Can you add a pub use params::Params here so that people can use this as bitcoin::consensus::Params?

Can you change this commit to address this?

Addressed in: cccbb057bc88462284430b91ea333ea13ecbf7b0

apoelstra · 2018-09-16T16:01:51Z

src/consensus/encode.rs

Should be renamed to Encoder. Ditto for SimpleDecoder.

Addressed in: 58b66001fdfa538dae9448c05ac1f307a8225d97

apoelstra · 2018-09-16T16:02:19Z

src/consensus/encode.rs

We should drop RawEncoder and just do an impl<T: Write> Encoder for T, the way ReadExtBytes in byteorder works. Similar for RawDecoder.

Addressed in: 58b66001fdfa538dae9448c05ac1f307a8225d97

apoelstra · 2018-09-16T16:02:48Z

src/consensus/mod.rs

Should also pub use Encoder and Decoder (suggested renames of SimpleEncoder and SimpleDecoder)

and deserialize and serialize

Addressed in: cccbb057bc88462284430b91ea333ea13ecbf7b0

apoelstra · 2018-09-16T16:05:30Z

src/consensus/encode.rs

We should just unwrap in here, not return an error, and document in the Encodable trait that you should only ever error if the underlying Encoder errors.

Addressed in: efb85a1f5d74ba1ef8b5d7c4779b467cb2831488

apoelstra · 2018-09-16T16:08:25Z

src/util/hash.rs

We should impl Write for this rather than SimpleEncoder, then the default impl for all Writes can take over.

Addressed in: 4f76787dcf6c1a673c08bed7bdcb9ed2fe5864b3

dongcarl · 2018-09-16T19:52:49Z

Done.

apoelstra · 2018-09-16T20:30:11Z

Can you rebase everything so I can just review the whole changeset again?

dongcarl · 2018-09-16T20:31:18Z

@apoelstra rebase over...? Confused.

apoelstra · 2018-09-16T20:33:31Z

@dongcarl use rebase -i to make a readable history that does not have a bunch of commits undoing other commits.

dongcarl · 2018-09-16T20:58:48Z

@apoelstra Squashed commits together that only mvs/rms stuff

dongcarl · 2018-09-16T21:08:07Z

Retriggering Travis.

dongcarl · 2018-09-16T21:13:37Z

Does anyone know why it's not testing the latest commit?

dongcarl · 2018-09-16T22:34:30Z

Looks like it's working now.

apoelstra · 2018-09-19T22:16:44Z

@dongcarl I need a history where my nits are addressed. I can't tell where or if you've fixed things when they're all spread around.

dongcarl · 2018-09-20T10:37:08Z

@apoelstra I've split the commits up a little more so it's obvious what's addressing what, I've also commented the relevant commits on your reviews, 58b66001fdfa538dae9448c05ac1f307a8225d97 is a little too convoluted to split up in reasonable time 🙁

- Modify VersionMessage constructor to take in parameters directly that would have otherwise been extracted from a Socket (now removed)

- Use Sha256dEncoder for calculating merkle root - Remove BitcoinHash implementation for Vec<u8>

- Move network::encodable::* to consensus::encode::* - Rename Consensus{En,De}codable to {En,De}codable (now under consensus::encode) - Move network::serialize::Error to consensus::encode::Error - Remove Raw{En,De}coder, implement {En,De}coder for T: {Write,Read} instead - Move network::serialize::Simple{En,De}coder to consensus::encode::{En,De}coder - Rename util::Error::Serialize to util::Error::Encode - Modify comments to refer to new names - Modify files to refer to new names - Expose {En,De}cod{able,er}, {de,}serialize, Params - Do not return Result for serialize{,_hex} as serializing to a Vec should never fail

apoelstra

ack

Bump bitcoin dependency to 0.26

Changed miri tests passing

…rust-bitcoin#156) * fix: add a BIP32 path to Default wallet creation options * fix: increase gap limits to match Android and iOS defaults * chore: fix special gap limit comment * fix: add BIP32 account 0 to create_accounts_with_passphrase_from_options * style: format to pass fmt * tests: update tests for adding BIP32 to Default wallet * style: add missing , * chore: remove unused import * chore: fix comment in test * fix: lower gap limits based on testing with one old wallet from 2019 * style: fix format

dongcarl force-pushed the 2018-8-network-cleanup branch from 307ec2e to ed5a358 Compare September 3, 2018 05:53

dongcarl changed the title ~~WIP: Clean up network::encodable and network::serialize~~ Ready for Review: Clean up network::encodable and network::serialize Sep 3, 2018

apoelstra added this to the 0.15 milestone Sep 5, 2018

apoelstra self-requested a review September 5, 2018 16:11

dongcarl force-pushed the 2018-8-network-cleanup branch from ed5a358 to 8f982ff Compare September 8, 2018 00:17

dongcarl force-pushed the 2018-8-network-cleanup branch 2 times, most recently from 2949f70 to a3a8b74 Compare September 9, 2018 10:23

dongcarl mentioned this pull request Sep 9, 2018

BIP174 (de)serialization support #103

Merged

dongcarl force-pushed the 2018-8-network-cleanup branch from a3a8b74 to 5c4ca24 Compare September 13, 2018 18:18

apoelstra reviewed Sep 16, 2018

View reviewed changes

dongcarl force-pushed the 2018-8-network-cleanup branch 2 times, most recently from 2ba4074 to d66a34b Compare September 16, 2018 19:25

dongcarl force-pushed the 2018-8-network-cleanup branch from d66a34b to c4f4d5f Compare September 16, 2018 20:55

dongcarl closed this Sep 16, 2018

dongcarl reopened this Sep 16, 2018

dongcarl force-pushed the 2018-8-network-cleanup branch from c4f4d5f to bac7a9d Compare September 16, 2018 21:37

dongcarl force-pushed the 2018-8-network-cleanup branch from bac7a9d to 4f76787 Compare September 20, 2018 10:31

dongcarl added 6 commits September 25, 2018 21:13

Remove nu_select macro

7f11766

Remove low-level networking support

7e9d393

- Modify VersionMessage constructor to take in parameters directly that would have otherwise been extracted from a Socket (now removed)

Move network::consensus_params to consensus::params

97937b1

Move serialize::BitcoinHash to util::hash::BitcoinHash

8e0e4eb

- Use Sha256dEncoder for calculating merkle root - Remove BitcoinHash implementation for Vec<u8>

Use default impl for Encoder for Sha256dEncoder

c42252c

dongcarl force-pushed the 2018-8-network-cleanup branch from 4f76787 to c42252c Compare September 25, 2018 13:21

apoelstra approved these changes Sep 25, 2018

View reviewed changes

apoelstra merged commit 45140a3 into rust-bitcoin:master Sep 25, 2018

apoelstra mentioned this pull request Nov 3, 2018

transaction: make 0-input de/serialization always use Segwit #153

Merged

casey pushed a commit to casey/rust-bitcoin that referenced this pull request Nov 28, 2023

Merge pull request rust-bitcoin#156 from romanz/bump-bitcoin

db723c2

Bump bitcoin dependency to 0.26

yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024

Merge pull request rust-bitcoin#156 from sanket1729/miri_fix

360d40a

Changed miri tests passing

Conversation

dongcarl commented Aug 27, 2018

Uh oh!

dongcarl commented Sep 8, 2018

Uh oh!

dongcarl commented Sep 13, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

apoelstra commented Sep 16, 2018

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

apoelstra commented Sep 16, 2018

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

dongcarl commented Sep 16, 2018

Uh oh!

apoelstra commented Sep 19, 2018

Uh oh!

dongcarl commented Sep 20, 2018

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants