Refactor address byte swapping #968
Conversation
|
Your observation about naming looks correct, the comment was talking about Anyway, I think the whole code looks over-complicated and confusing - why not just write to the writer in a for loop, converting individual |
I used the same name on purpose, it's the same method implemented on a different type.
The method is used when decoding as well, in multiple places. |
|
The reason I don't like I think in a tricky code like this it may be worth a bit of code repetition (decoding should still be a single function) if it increases clarity. But maybe it's just me who has harder time tracking endianess. 🤷♂️ I wouldn't block the PR for any of my objections, it's just for consideration. |
|
Force push is total re-write, based on review by @Kixunil. PR description has also been re-written. |
src/network/address.rs
Outdated
| let mut buf = [0u8; 2]; | ||
|
|
||
| for i in 0..8 { | ||
| if io::Read::read(&mut r, &mut buf)? != 2 { |
There was a problem hiding this comment.
This should be read_exact. read returning less bytes is not an error.
There was a problem hiding this comment.
Thanks man, I learned a bit more about readers.
src/network/address.rs
Outdated
| if io::Read::read(&mut r, &mut buf)? != 2 { | ||
| return Err(encode::Error::ParseFailed("missing address bytes")); | ||
| } | ||
| address[i] = u16::from_be_bytes(buf); |
There was a problem hiding this comment.
Why not for word in &mut address and then *word = u16::from_be_bytes(buf);? (feel free to come up with better var name)
There was a problem hiding this comment.
Ah nice, thanks. Implemented as suggested.
|
Changes in force push:
|
|
I have no idea why I did this PR other at this moment in time, converting to draft to let the edition bump stuff go in. |
src/network/address.rs
Outdated
| let mut len = self.services.consensus_encode(&mut s)?; | ||
|
|
||
| for word in &self.address { | ||
| len += io::Write::write(&mut s, &word.to_be_bytes())?; |
There was a problem hiding this comment.
Just realized we have the same issue as with read_exact, we need write_all() here. Also while changing you could use s.write_all() instead, no need to repeat the whole trait since it can't get confused anyway thanks to generics.
There was a problem hiding this comment.
Legend man, late is better than never :) Will fix as suggested.
|
Changes in force push: Use |
|
Looks good now, will ACK when it's un-drafted. |
When encoding a `network::Address` two of the fields are encoded big-endian instead of little-endian as is done by `consensus_encode`. In order to achieve this we have a helper function `addr_to_be` that swaps the bytes. This function is miss-named because it is not converting to a specific endian-ness (which implies different behaviour on machines with different endian-ness) but is reversing the byte order irrespective of the underlying architecture. - Remove function `addr_to_be` - Inline the endian-ness code when encoding an address - Remove TODO and use `to_be_bytes` when encoding port - Add a function for reading big-endian bytes `read_be_address` - Use `read_be_address` when decoding `Address` and `Addrv2` Refactor only, no logic changes. Code path is already covered by unit tests.
|
Good to have you back @Kixunil! |
07c7530 Refactor address byte swapping (Tobin C. Harding) Pull request description: Refactor address byte swapping When encoding a `network::Address` two of the fields are encoded big-endian instead of little-endian as is done by `consensus_encode`. In order to achieve this we have a helper function `addr_to_be` that swaps the bytes. This function is miss-named because it is not converting to a specific endian-ness (which implies different behaviour on machines with different endian-ness) but is reversing the byte order irrespective of the underlying architecture. - Remove function `addr_to_be` - Inline the endian-ness code when encoding an address - Remove TODO and use `to_be_bytes` when encoding port - Add a function for reading big-endian bytes `read_be_address` - Use `read_be_address` when decoding `Address` and `Addrv2` Refactor only, no logic changes. Code path is already covered by unit tests. ACKs for top commit: apoelstra: ACK 07c7530 Kixunil: ACK 07c7530 Tree-SHA512: 186bc86512e264a7b306f3bc2e18d1619f3cd84fc54412148cfc2663e8d6e9616ea9e2fe19eafec72d76cc11367a9b39cac2b73210d9e43eb8f453bd253b33de
Refactor address byte swapping
When encoding a
network::Addresstwo of the fields are encodedbig-endian instead of little-endian as is done by
consensus_encode. Inorder to achieve this we have a helper function
addr_to_bethat swapsthe bytes. This function is miss-named because it is not converting to a
specific endian-ness (which implies different behaviour on machines with
different endian-ness) but is reversing the byte order irrespective of
the underlying architecture.
addr_to_beto_be_byteswhen encoding portread_be_addressread_be_addresswhen decodingAddressandAddrv2Refactor only, no logic changes. Code path is already covered by
unit tests.